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
Set client cookies from PiwikTracker.php #114
Conversation
After submitting the pull, I realized that the logical architecture of the original patch (from #2699) and piwik.js were extremely dissimilar. Consequently, I pulled over a bunch of functions from piwik.js and integrated them into PiwikTracking. The pull is much larger as a result, but also more in-line with the rest of the codebase (particularly piwik.js since it provides similar functionality). The only issue I couldn't immediately resolve is that PHP HTML-encodes the asterisk when setcookie is called. As a result, I cannot exactly reproduce the session cookie used in Java. |
Thanks for the PR, very nice!
you can use setrawcookie() for this purpose. Code Review:
Cheers |
I should have time to make some of the smaller changes (e.g. 2nd, 3rd bullets). What's the easiest way to contribute them without disrupting your process? Should I add commits here? Leave this version as-is and submit another patch once it's accepted? I'd also like to raise some issues around your 5th bullet:
|
this would be the best
it is OK to set the cookies even when it does not necessarily make sense (cron etc.). It is OK because it cannot cause bugs.
Good point! I fixed it separately in: def6da2
See previous commit + this one: Does it fixes the weirdness? |
I have two observations that interact with one another in this part of the code so I have to bring them up together -- please bear with me.
These two observations come together in the code (post your commits), which makes it very difficult to set the ID to a random (valid) ID if a user has a cookie. Specifically,
With respect to my first question, this helps enforce an With respect to my second question, this could be undesirable. If the cookie is ever wrongly-injected, we're forced to send a An alternative is to:
I don't know enough about the |
Yes cid forces the visitor to use set ID and requires token_auth
whereas _id is an indication of the ID as read by cookies.
yes Btw I agree the visitor ID situation could be a bit clearer. I think this ticket will help: http://dev.piwik.org/trac/ticket/3490
cid requires token_auth so is something that few people may use. ID read from cookies are passed with _id |
…nt cookies when the VisitorId is set or updated. Conflicts: libs/PiwikTracker/PiwikTracker.php
…factored existing methods to take advantage of imported functions. Conflicts: libs/PiwikTracker/PiwikTracker.php
…processed correctly before the url is created.
…arallel piwik.js.
…rst Session cookie.
Rebased the pull so I have access to your commits. Added commits to address the session cookie issue (setrawcookie) as well as bullets 2 (cookieVisitorId), 3 (values read from 1st party cookie), and 5 (default values... in the constructor). I also added code to set the custom variable cookie, but don't have a specific use-case for Custom Variables to do any real testing. I did note some differences between the PiwikTracker implementation and the piwik.js implementation. Specifically, the JS |
Thanks for PR! I improved/tidied and merged as part of new ticket http://dev.piwik.org/trac/ticket/4239#ticket |
…Refs matomo-org#4239 Based off the pulll request by @claytondaley and tidied up matomo-org#114
…Refs #4239 Based off the pulll request by @claytondaley and tidied up matomo-org/matomo#114
This pull request adds variables, functions, and logic so PiwikTracker can update client cookies and may suffice to meet the request in ticket #3239 (http://dev.piwik.org/trac/ticket/3239). The code was originally derived from the patch submitted (and ultimately not-accepted) in ticket #2699 (http://dev.piwik.org/trac/ticket/2699). Deeper research showed that the logic inside this patch was incomplete and differed greatly from piwik.js. In response, functions in Piwik.js were copied over and translated into PHP, refactoring existing code to leverage these functions.