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.

Sign up for GitHub

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

Improve GA/consent manager detection by not restricting to curl and caching result #20000

Closed
justinvelluppillai opened this issue Nov 13, 2022 · 4 comments · Fixed by #20008
Closed

Improve GA/consent manager detection by not restricting to curl and caching result #20000

justinvelluppillai opened this issue Nov 13, 2022 · 4 comments · Fixed by #20008
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Milestone
4.12.4

Comments

@justinvelluppillai
Copy link
Contributor

justinvelluppillai commented Nov 13, 2022 •
edited

Currently we are restricting the request to use curl https://github.com/matomo-org/matomo/pull/19957/files#diff-105cf795eee08d8e19b4daf399977e29fbbf8b3d5d2b6995e06d0909e0b68785R116, whereas we can just use sendHttpRequest(). Let's switch it to use that unless there is reason not to?

Also can we make sure to cache the result for say 4+ weeks or forever or something? Then we don't try to request the data every time which would make these pages and widgets etc quite slow. Ideally, we would even mostly update the cached result only in the background (or store it forever).
@justinvelluppillai justinvelluppillai added the Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. label Nov 13, 2022
@justinvelluppillai justinvelluppillai added this to the 4.12.4 milestone Nov 13, 2022
@justinvelluppillai
Copy link
Contributor Author

@mattab I have added this directly to the current patch milestone as an extension of the previous work which was also scheduled for this release. I recommend we include it but let me know if it's something you would prefer to delay 👍🏽

@peterhashair
Copy link
Contributor

@justinvelluppillai I think this is because sendHttpRequestBy contains a param defines, $acceptInvalidSslCertificate. sendHttpRequest does not have the feature, if I am correct.

@justinvelluppillai
Copy link
Contributor Author

If needed we can still use that function but instead of specifying curl we can use Http::getTransportMethod() - perhaps this would need investigation.

@sgiehl
Copy link
Member

sgiehl commented Nov 14, 2022

Yes. Simply using Http::getTransportMethod() should be they way to go here...

@bx80 bx80 mentioned this issue Nov 14, 2022
Merged
11 tasks
@elabuwa elabuwa added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. 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.4
Development

Successfully merging a pull request may close this issue.

Improve GA/consent manager detection by not restricting to curl and caching result matomo-org/matomo
4 participants
@justinvelluppillai @sgiehl @elabuwa @peterhashair