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

Update ps_googleanalytics.php #168

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Conversation

panariga
Copy link

wrap hooks code in try...catch to avoid site crashes

Questions Answers
Description? Let's wrap the hooks code in a try...catch block. If any error occurs, the Analytics module may crash and cause a 500 error, which is unacceptable for the front-end. For instance, crashes can occur if paths saved in a cookie are too long, exceeding the maximum allowed size, or if non-ASCII paths have improperly escaped characters. It's better to lose some analytics events than to lose customers due to errors.
Type? bug fix / improvement
BC breaks? no
Deprecations? no
Fixed ticket? no
Sponsor company no
How to test? additional test not needed

wrap hooks code in try...catch to avoid site crashes
@ps-jarvis
Copy link

Hello @panariga!

This is your first pull request on ps_googleanalytics repository of the PrestaShop project.

Thank you, and welcome to this Open Source community!

Copy link
Contributor

@Hlavtox Hlavtox left a comment

Choose a reason for hiding this comment

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

Sorry @panariga, but this is not the proper way to do things. 👍 If there is some issue, it must be fixed.

@panariga
Copy link
Author

I agree that as many issues as can be fixed should be fixed. However, some can't be fixed by design. For example, Bingbot tries all possible variations for faceted search combinations. This results in super-long queries and the module tries to save them to a cookie. Because of that, the operation fails due to the cookie size limitations. This requires hard refactoring to create new storage.

But in general, this module is just for stats. It shouldn’t break the shop's work. Due to the complicated information-gathering routines, you can never guarantee an error-free flow. So, anyway, wrapping in try..catch has no regressions—you still have errors in the console, but users are not frustrated by these errors.

@Hlavtox
Copy link
Contributor

Hlavtox commented Jun 16, 2024

@panariga Well, this is actually quite a legit issue we could definitely fix, either by storing only the url without a query string, or by using a storage for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Ready for review
Development

Successfully merging this pull request may close these issues.

3 participants