-
Notifications
You must be signed in to change notification settings - Fork 94
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
Fix hookFilterProductContent to allow hook chain #137
Conversation
idnovate
commented
Mar 14, 2022
•
edited by hibatallahAouadni
Loading
edited by hibatallahAouadni
Questions | Answers |
---|---|
Description? | All hookFilter* functions should always return the following: return $params; Returning $params ensures that other modules in the same hook chain receive the correct $params argument. |
Type? | bug fix / critical |
BC breaks? | no |
Deprecations? | no |
Fixed ticket? | Fixes PrestaShop/PrestaShop#28342 |
How to test? | To reproduce the bug install an additional module with hookFilterProductContent AFTER the productcomments and run in debug mode. |
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.
You break phpstan and php cs fixer with your fix
@idnovate, you have to change docblock in the hook implementation to not return void but array :) |
Done. Is it not easier for you to apply and commit this patch than wait for the author? Or even when there's just a change in a single line but placed tabs instead of spaces. Which is the aim of it? |
@idnovate it depends, sometimes we can use GH UI to request changes in a specific part of the file, but this part with the comment wasn't available :D |
Thanks @idnovate I'm sending this PR to our QA :) |
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.
Hi @idnovate,
I checked with the module https://github.com/PrestaShop/ps_qualityassurance/
Without PR, I have this exception on the FO > Product page
With PR
Thanks to check and feedback!
@khouloudbelguith hi, how do you test second scenario? do you have return $params? edit: I see now, without return you're producing the same error that was fixed in the PR 😁 Try with return $params; 👍🏻 |
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.
Hi @kpodemski,
I tried the first case with dump ($params);
=> NOK
I attached a screen record
first.case.with.dump.params.mp4
I tried the second solution return $params;
=> NOK
I attached a screen record
return.params.mp4
Thanks!
Hello @khouloudbelguith The fix is valid. Please test it with the example module. Maybe there are some specific things related to the Information:
This issue is critical as it affects some very popular hosting environments that use LiteSpeed cache. (the litespeedmodule is affected by this bug) |
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.
Thanks @idnovate
It's a good fix for the module itself, but maybe we should make chained hook execution more resilient to invalid implementation in the core (like checking that the hook actually returned something before replacing the chained arguments)
Any chance to put it on your schedule? it's quite a critical issue :( |
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.
Hi @idnovate, @kpodemski,
Thanks for the details and sorry for the delay.
It is ok ✔️
I check this PR with 1.7.8.x, 8.0.x and develop.
I installed those modules:
- filterproductcontent.zip
- productcomments_filter_test.zip
- https://github.com/PrestaShop/ps_qualityassurance/
productcomment_filter.mp4
quality.mp4
filter_product.mp4
I launched the automatic tests on the FO/product_page => OK
I launched the automatic tests on the BO/Module manager => OK
Thanks!
Thanks @idnovate and @khouloudbelguith |