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
Call setVisitorIdCookie later so disableCookie config can be set first #18135
Conversation
…t and avoid rogue cookie creation when creating tracker instances
build js |
If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'. |
If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'. |
ping @justinvelluppillai there's still one of my comments left |
If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'. |
This PR was last updated more than one month ago, maybe it's time to close it. Please check if there is anything we still can do or close this PR. ping @matomo-org/core-reviewers |
ping @justinvelluppillai |
build js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works for me, did quite some testing around it ( but didn't check the tests). @sgiehl can you think of any issue with this?
If I do it this way I will need to update quite a few tests which rely on the visitorId cookie being set when tracker is created. eg see failing tests here #18789 |
Indeed, seeing https://app.travis-ci.com/github/matomo-org/matomo/jobs/559513056 there are few tests failing. Not sure if they highlight a regression or if they just need to be tweaked. Maybe |
Yes I'd need to add a tracking request to all those calls for those expected outcomes to be right. Would need to understand the rationale for the tests to know if I can just change them or it would be a regression also. |
@justinvelluppillai does #18789 replace this PR? If so be good to close this one then. |
This PR has most of the conversation history so worth keeping. The other is just a sandbox for running test experiments. I don't mind which one is closed eventually once I have ironed out the issues but was expecting the other one to be closed and this one merged. Neither has |
If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'. |
Description:
Fixes #17892
Review