@diosmosis opened this Pull Request on December 14th 2020 Member

Description:

FYI @tsteur

Currently the segment that gets archived can have a different hash than what is requested in the browser. Encoding the segment before sending it to RequestCommand seems to fix this, but I'm not exactly sure why.

Review

  • [ ] Functional review done
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@tsteur commented on December 14th 2020 Member

@diosmosis is there an issue or comment where there's more information about this? It be unexpected this is suddenly needed or was this an issue in 3.X as well?

@diosmosis commented on December 14th 2020 Member

@tsteur it's to solve the problem reported by this user: https://github.com/matomo-org/matomo/issues/16842#issuecomment-742466061

there's a small discussion there. It wasn't required in 3.x, I guess in 3.x the segment was already encoded once in core:archive, then the single urlencode() double encoded it properly? It's strange, but the hash is only correct on both UI requests and climulti requests, if it is double encoded here.

@tsteur commented on December 14th 2020 Member

👍 I suppose it's not really trivial to add a test here? Or a test would probably archive the data when browser archiving is enabled. This would also explain why some archives aren't showing any data (while some others do) for our web account.

Eg this segment is showing data

image

while such a segment isn't showing data since Matomo 4

image

Can we make sure it won't cause any problems with other segments like that it's not falsely double encoded when using ActionsInVisit>1 for example?

On our demo for matomo website we can reproduce this as well by the looks. I'll patch it there already as well so we can see if the data eventually comes alright. Good find!

@diosmosis commented on December 14th 2020 Member

Might be possible to add a test... will see.

This Pull Request was closed on December 17th 2020
Powered by GitHub Issue Mirror