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

Prevent cookie overflow #25

Merged
merged 2 commits into from Sep 14, 2022
Merged

Prevent cookie overflow #25

merged 2 commits into from Sep 14, 2022

Conversation

daresh
Copy link
Contributor

@daresh daresh commented Mar 15, 2022

The module should not store the whole history of viewed products, but only as much as configured.

Questions Answers
Description? "Viewed Products" module
Type? bug fix
BC breaks? no
Deprecations? no
Fixed ticket? Fixes PrestaShop/PrestaShop#26996
How to test? Create a script displaying cookie contents and verify that only a limited amount of last viewed products gets stored there. For example: var_export(Context::getContext()->cookie);

The module should not store the whole history of viewed products, but only as much as configured.
@NeOMakinG
Copy link

@daresh PHPStan is failing

@daresh
Copy link
Contributor Author

daresh commented Mar 16, 2022

@NeOMakinG why? What is the error?

ps_viewedproduct.php Outdated Show resolved Hide resolved
@AureRita
Copy link

AureRita commented Mar 21, 2022

QA by dev please

@Progi1984 Progi1984 added this to the 1.2.2 milestone Mar 24, 2022
@tswfi
Copy link

tswfi commented Jul 4, 2022

so when might this get merged and new release made?

@AureRita
Copy link

AureRita commented Jul 4, 2022

friendly reminder @matks

@matks
Copy link
Contributor

matks commented Jul 7, 2022

Hello ! sorry for late test, with the 8.0.0 feature freeze we've accumulated a lot of PRs to be checked. We're trying to catch up but it's a big pile 😅

@FabienPapet
Copy link
Member

This fix looks OK for me and tested on my local environment, however there is a technical limitation due to cookie encryption.

Cookies are limited to a 4096 length, and if you set the PRODUCTS_VIEWED_NBR to a high value (100 for example), the encrypted cookie length will be higher than the maximum cookie size and the problem will still exist.

I think we need to validate the configuration to a maximum allowed value like 16 24 or 32 to really fix the problem.

What do you think ?

@matks
Copy link
Contributor

matks commented Aug 1, 2022

Thank you @FabienPapet for testing this PR

I add "waiting for author" until we have @daresh answer

@daresh
Copy link
Contributor Author

daresh commented Aug 1, 2022

I think that's a very good idea to limit the maximum setting for the viewed products.

@matks
Copy link
Contributor

matks commented Aug 31, 2022

@daresh can you add changes to this PR to handle @FabienPapet suggestion?

@kpodemski
Copy link

There are more and more reports about this issue. I think we should merge it as is and improve it later.

What do you think @FabienPapet @matks ?

@FabienPapet
Copy link
Member

Good for me !

@kpodemski
Copy link

Great @FabienPapet ! Mathieu @matks ?

@matks
Copy link
Contributor

matks commented Sep 14, 2022

OK 😄

@matks
Copy link
Contributor

matks commented Sep 14, 2022

Thank you @daresh

@matks matks merged commit 598a046 into PrestaShop:dev Sep 14, 2022
@matks matks modified the milestones: 1.2.2, 1.2.3 Sep 14, 2022
@daresh daresh deleted the patch-1 branch September 14, 2022 19:40
@LuigiMdg
Copy link

Has been solved?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet