Encode segmentValue for Actions reports. This extra encoding is required because of how many times a segment is urldecoded:
This means the value needs to be triple encoded for values like plus signs to be used properly in the segment.
http://example.org/with+plus/will display like that, instead of
I am not sure if this fix will have repercussions so I haven't modified the tests. Will do so after someone else gives their opinion.
If it fixes your issue and tests pass probably all good to go. It's almost impossible to review it since the segments are just a mess and we never know where they are urlencoded or urldecoded etc.
It will change a lot of the test output I think (another encoding in the segment property in API output). Think this won't be a BC break? at least not a potentially huge one.
Think this won't be a BC break?
I really don't know. As long as we can still handle the segments urlencoded and urldecoded it should be fine.
Applied the patch locally and it resolves the issue for me.
Don't have a fix yet, but found the cause of the error. Actions are stored urlencoded in the log_action table, but the value in the segment when opening the segmented visitor log/transitions modal/etc. is not.
I'm not sure this is the case? That might have been the case quite a while ago but shouldn't be the case anymore? at least for me it was stored normally
or you mean the URL is encoded after tracking?
I've looked at the PR for quite a while but the PR is quite complex and not trivial to understand especially re side effects etc. Left some comments though. Do you think there would be otherwise a solution where we don't need to change anything except for maybe the segment expression?
I'm not sure this is the case? That might have been the case quite a while ago but shouldn't be the case anymore?
It's not the case, it looked like it was, but I think I tracked something wrong.
Looking forward to merging this :+1:
This change turned out to be rather complex, I doubt I'll be able to finish it soon, and it's not as high value as other tickets. Would vote to put it in the priority backlog.
@diosmosis :ok_hand: But be good to finish this later (before Matomo 4 ideally...). Moving it to 3.12.0 milestone for now.
Nevermind, now all the other tests fail.
works 👍 code looks good too 👍