Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apparently the segment needs to be double encoded when sent in the climulti:request command. #16947

Merged
merged 5 commits into from Dec 17, 2020

Conversation

diosmosis
Copy link
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

@diosmosis diosmosis added Needs Review PRs that need a code review Regression Indicates a feature used to work in a certain way but it no longer does even though it should. labels Dec 14, 2020
@tsteur
Copy link
Member

tsteur commented Dec 14, 2020

@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
Copy link
Member Author

@tsteur it's to solve the problem reported by this user: #16842 (comment)

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
Copy link
Member

tsteur commented Dec 14, 2020

👍 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
Copy link
Member Author

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

@diosmosis diosmosis merged commit 7edc2c9 into 4.x-dev Dec 17, 2020
@diosmosis diosmosis deleted the correct-double-encoding-segment branch December 17, 2020 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants