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

Jump to bottom

Use browser client hints for detection #18843

Merged
merged 22 commits into from Jul 4, 2022
Merged

Use browser client hints for detection #18843

merged 22 commits into from Jul 4, 2022

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Feb 24, 2022

Description:

this requires matomo-org/device-detector#6989 to be finished and released

fixes #16125

Review

@sgiehl sgiehl added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. RFC Indicates the issue is a request for comments where the author is looking for feedback. labels Feb 24, 2022
@sgiehl
Copy link
Member Author

sgiehl commented Feb 24, 2022

build js

@sgiehl sgiehl force-pushed the clienthints branch 2 times, most recently from 481271a to f2e256b Compare March 8, 2022 14:25
@sgiehl sgiehl force-pushed the clienthints branch 4 times, most recently from d5be14c to e42d823 Compare March 18, 2022 09:15
@justinvelluppillai
Copy link
Contributor

How are we looking here @sgiehl? Anything we need to do to push this forward?

@sgiehl
Copy link
Member Author

sgiehl commented Mar 21, 2022

@justinvelluppillai Had to rewrite the JS a bit. As the former version didn't work in all cases. Once tests are fixed and added, this should be ready for a first review.

@sgiehl sgiehl force-pushed the clienthints branch 3 times, most recently from 5e4e078 to 5308797 Compare March 24, 2022 16:09
@sgiehl sgiehl added this to the 4.10.0 milestone Apr 8, 2022
@sgiehl sgiehl changed the title [POC] Use browser client hints for detection Use browser client hints for detection Apr 8, 2022
@sgiehl sgiehl removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. RFC Indicates the issue is a request for comments where the author is looking for feedback. labels Apr 12, 2022
@sgiehl
Copy link
Member Author

sgiehl commented Apr 14, 2022

Had some more thoughts on that one today. The implementation is kind of finished, but some more tests would be needed.
We also need to consider to make it possible to set the client hints in Matomo PHP Tracker, so we are also able to add some tests where those data is provided.
The general implementation should be ready for a first review.

@github-actions
Copy link
Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Apr 29, 2022
Findus23
Findus23 approved these changes Apr 30, 2022
View reviewed changes
Copy link
Member

@Findus23 Findus23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't count as a proper code review, but I looked through everything and it seems reasonable to me.

core/Tracker/Request.php Show resolved Hide resolved
js/piwik.js Show resolved Hide resolved
@Findus23 Findus23 added the Do not close PRs with this label won't be marked as stale by the Close Stale Issues action label Apr 30, 2022
sgiehl added 21 commits July 4, 2022 17:20
@sgiehl
inject client hints in js
c6066d2
@sgiehl
use client hints for detection
ac010a2
@sgiehl
don't use catch, as yui compressor can't parse it
ca4df26
@sgiehl
rebuilt js files
34f0395
@sgiehl
use new version of device detector
d9ee3bf
@sgiehl
more code adjustments
ca0db32
@sgiehl
updates expected test files
52127cf
@sgiehl
improve js
99c3921
@sgiehl
fix header detection
8122e34
@sgiehl
improve cache key handling
8d2b121
@sgiehl
fix tests
178e0bf
@sgiehl
use a separate queue to wait for client hints if needed
31b5a4d
@sgiehl
try to fix js tests
2503161
@sgiehl
also consider X_HTTP_REQUESTED_WITH header as client hints
550e389
@sgiehl
updates expected test files
9ec9fc0
@sgiehl
Extend demo detection with client hints
6ddd910
@sgiehl
code improvements
91d8de3
@sgiehl
use new version of matomo-php-tracker
4439b1b
@sgiehl
Adds test case for client hints set through matomo php tracker
0a80078
@sgiehl
apply review feedback
c5f4c03
@sgiehl
submodule update
d39499b
@sgiehl
Copy link
Member Author

sgiehl commented Jul 4, 2022

All dependencies have been merged and updated in the PR. Once the tests have run through, I think this one can be merged.

@sgiehl sgiehl removed the Needs Review PRs that need a code review label Jul 4, 2022
@sgiehl sgiehl merged commit ecae3ea into 4.x-dev Jul 4, 2022
@sgiehl sgiehl deleted the clienthints branch July 4, 2022 17:43
@MatomoForumNotifications
Copy link

This pull request has been mentioned on Matomo forums. There might be relevant details there:

https://forum.matomo.org/t/when-will-support-sec-ch-ua-be-implemented/46570/2

@justinvelluppillai justinvelluppillai added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinvelluppillai justinvelluppillai justinvelluppillai left review comments

@sanchezzzhak sanchezzzhak sanchezzzhak left review comments

@Findus23 Findus23 Findus23 approved these changes

@bx80 bx80 bx80 approved these changes

Assignees
No one assigned
Labels
Do not close PRs with this label won't be marked as stale by the Close Stale Issues action not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
4.12.0
Development

Successfully merging this pull request may close these issues.

support Sec-CH-UA Client Hints in addition to UserAgent
6 participants
@sgiehl @justinvelluppillai @MatomoForumNotifications @bx80 @sanchezzzhak @Findus23