Actually shouldn't merge just yet, forgot to mention the latest change will require changes to premium plugins, since the
'rows' query param appears to be handled only in plugins not in core.
Actually shouldn't merge just yet, forgot to mention the latest change will require changes to premium plugins, since the 'rows' query param appears to be handled only in plugins not in core.
Haven't looked at code or anything... will it be possible to keep BC? Or at least update plugins to be compatible with all Matomo versions?
I'm not to sure about BC. The change was to encode the values in the rows parameter so a row could have a comma in it (eg,
Tue, Jan 28 or whatever). Otherwise the rows param would interpret it as 2 rows,
'Tue', 'Jan 28'. The problem is plugins use
'rows' to to set the
rows_to_display ViewDataTable config, it's not done in core, so the corresponding urldecode has to be done in the plugins.
I guess we could modify the 'rows' query parameter in FrontController or something like it to provide BC (ie, urldecode it there before it gets to the evolution graph controller methods)... but I'm not sure how good a solution that is... We can certainly update premium plugins. Shouldn't be an issue for previous matomo versions since I wouldn't expect there to be urlencoded data in a row label (at least for plugins that support
@tsteur finally got the tests to pass, I think this could use another review. One big change: turns out the code always copied the last log's properties, even if the last log was part of a different visit. Since we only looked back the visit standard length time, this wasn't an issue, but now since we can look further back, I had to modify the VisitRecognizer code more.
Looks good to merge for me if tests pass 👍
@tsteur looks like the silent fail might be a feature: https://github.com/matomo-org/matomo/blob/3.x-dev/tests/PHPUnit/Fixtures/TwoSitesEcommerceOrderWithItems.php#L142-L147
I guess that means I should remove the new exception... not sure how else we could report the issue, maybe a tracker failure in the UI, but then those "receipt views" might result in a lot of tracker failures...
@diosmosis maybe we could update the test to expect an error instead of
self::checkResponse()? In general it be great to fail there so they have a chance to notice there was possibly something wrong. We could of course also just keep to silently fail as it does nothing in the end.
@tsteur I thought about that, but the specific case linked is refreshing a "receipt" page after the order is tracked, which seems like it could happen regularly. Ie, this might be a common occurrence and not just an error. Actually I guess it wouldn't be a big issue if we gave a 400... users would likely know that it's fine for them.
Have no preference really, I reckon either is fine. I was thinking if there could be edge cases where it's maybe a problem to send the same request twice but can't really think of anything right now.
@tsteur will just update the tests, think it should be ok if in these cases there's an error response.