@mattab opened this Pull Request on March 10th 2016 Owner
@tsteur commented on March 10th 2016 Owner

I'm not sure if we should do too much validation here in Tracker API. In this case the user was using an old version of Piwik and it would have been fixed by updating. I'd rather have the raw data in DB whatever was sent and format if needed later in the report etc. Ideally the code was written well enough that we need to format output only only in one place wherever resolution is used. This gives developers to track whatever they want as the Tracking API is kind of generic and might not only relate to screen resolutions but possibly also resolutions. Now, if we think of pixels, they will be likely always an integer. But if we think of other resolutions such as tracking it in mm (millimeter), on Android you could technically use 0.5dp or so etc.

Also re check for width > 9999. There are already screens with 7680×4320 and likely there are at some point soon, if not already, screens > 9999. Something like this should definitely not be done in Tracking API. The tracking API has to be "stupid" and not do such things like checking whether the width is greater than something.

@tsteur commented on March 10th 2016 Owner

Similar problem where API tried to be smart is here https://github.com/piwik/piwik/issues/9842#issue-136043409 .

@tsteur commented on March 10th 2016 Owner

noticed https://github.com/piwik/piwik/issues/9913 re 9999px. I would still remove it here meanwhile. And ideally also not use round as a generic tracking API should not make such assumptions about what the resolution might be

@mattab commented on March 14th 2016 Owner

I agree generally Tracking API shouldn't validate data. in this case I thought it makes some sense, because the config_resolution DB table column is by design meant to store a screen resolution. Since we store resolution in one string column it is important to validate that the screen resolution is correctly formed... Resolution must include a width and a height. we validate the width and height to prevent storing an invalid resolution.

In general if it's not good to validate data in tracking API, an alternative solution would be that the Archiver validates the resolution labels and removes those that are invalid... is this maybe this you would prefer?

@tsteur commented on March 14th 2016 Owner

In general if it's not good to validate data in tracking API, an alternative solution would be that the Archiver validates the resolution labels and removes those that are invalid... is this maybe this you would prefer?

Yes that's something I would prefer. This let's us later reuse the same field for tracking some "resolution" in "mm". In some cases it might even make sense to track screen resolution in mm instead of px. We'd need to create a custom report / archiver for this but that's fine.

@tsteur commented on March 14th 2016 Owner

because the config_resolution DB table column is by design meant to store a screen resolution

In general we need to rethink some of the meanings while we're going generic. Don't have to change the meaning everywhere but could make sense for some columns.

As for Piwik 2.16.0 the bug should be fixed already. For Piwik 3+ we could issue an alter update to increase the size of this field and track all kinda values. If they track some values wrong, they still can make at least something out of the data instead of having them all tracked as unknown. Eg when only width was tracked they know at least the width. It seems not so important for resolution but I'm more thinking globally how the Tracking API should behave. We can always remove invalid data afterwards but we will never be able to restore not tracked values (or values set to unknown)

@mattab commented on March 15th 2016 Owner

OK I understand your point of view. We don't really need this change for now.

As for Piwik 2.16.0 the bug should be fixed already. For Piwik 3+ we could issue an alter update to increase the size of this field and track all kinda values.

Covered in https://github.com/piwik/piwik/issues/9913

This Pull Request was closed on March 15th 2016
Powered by GitHub Issue Mirror