-
Notifications
You must be signed in to change notification settings - Fork 119
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
Introduce anonymous product analytics #746
Conversation
Quetzacoalt91
commented
Jun 28, 2024
Questions | Answers |
---|---|
Description? | This PR introduces anonymous analytics to know how many times the module is used to upgrade / restore and how. |
Type? | new feature |
BC breaks? | Yes, some files are being moved, i.e Tasks |
Deprecations? | Nope |
Fixed ticket? | Fixes PrestaShop/PrestaShop#36432 |
Sponsor company | @PrestaShopCorp |
How to test? | Triggering updates and restores will be counted, as well as their ending (Successful or not). |
{ | ||
$matches = []; | ||
preg_match( | ||
'/^V(?<version>[1-9\.]+)_/', |
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.
Suggestion: explain in phpDoc why you exclude some patterns
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.
Not sure I got your message properly. What do you mean by excluding when I try to pickup the version from a string?
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.
If I call getRestoreVersion(), you check $this->getRestoreName()
and verify if it matches V(?<version>[1-9\.]+)
If it matches, you return it. If it does not you return null. So I said you "exclude" patterns which do not match.
An implementation that does not do exclusion would be simply returning `$this->getRestoreName()
I suggest explaining the reason behind the above logic
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.
Here you go
classes/Analytics.php
Outdated
/** | ||
* @var array | ||
*/ |
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 will not work with the new level of PHPstan 😢
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.
Fortunately PHPStan's PR should be merged first, and I'll get all the new feedback from it.
… the analytics included
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.
Thank you for your PR, I tested it and it seems to works as you can see :
Tested from :
1.7.6.4 to 8.1.7
8.0.5 to 8.1.7
8.1.5 to 8.1.7
8.1.7 to 9.0.0
8.1.5 to 9.0.0
8.0.5 to 9.0.0
Because the PR seems to works as expected, It's QA ✔️
Thank you
It appeared during the tests that failing upgrade are not reported in case of a PHP timeout. There is room for improvement. Thanks @AureRita |
@Quetzacoalt91 Maybe it's the moment where |
Indeed, we already have it to send responses gracefully, so it may be completed with this feature. |