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

Do not set a cookie if it finally has not changed #14116

Merged
merged 1 commit into from Jun 24, 2019

Conversation

jocel1
Copy link
Contributor

@jocel1 jocel1 commented Jun 7, 2019

Questions Answers
Branch? develop
Description? Do not set a cookie which has finally not changed
Type? improvement
Category? CO
BC breaks? no
Deprecations? no

This modification could be useful if we are modifying several times the content of the prestashop cookie, to finally put at the end the same value than at the beginning.

Let's say for exemple we have

$this->context->cookie->last_visited_category == null

The front ProductController sets $this->context->cookie->last_visited_category = 2;
Hence _modified inside the cookie will be set to true.
Then we put back $this->context->cookie->last_visited_category = null; through a module

The _modified flag will still be true, and hence the cookie will be written even if there's no change inside it. This PR modify this behavior to avoir writing an unchanged cookie.


This change is Reviewable

@jocel1 jocel1 requested a review from a team as a code owner June 7, 2019 19:40
@prestonBot prestonBot added develop Branch Improvement Type: Improvement labels Jun 7, 2019
@matks
Copy link
Contributor

matks commented Jun 7, 2019

Makes sense ✅

Travis failed for bad reasons, I restart it

@matks matks added this to the 1.7.7.0 milestone Jun 7, 2019
@matks
Copy link
Contributor

matks commented Jun 7, 2019

Just curious, how did you end up thinking about this 😄? Did it impact badly performances on a shop you work on ?

@jocel1
Copy link
Contributor Author

jocel1 commented Jun 7, 2019

@matks Well actually I have a customer with the ps_categorytree module enabled, and it sets
$this->context->cookie->last_visited_category = (int)$product->id_category_default;

In that case, because a cookie is set, reverse proxy cannot cache the page. So I wanted to add an option to always reset the last_visited_category to null in my Varnish module, but because we just check the _modified flag, the cookie was still sent :)

@matks
Copy link
Contributor

matks commented Jun 10, 2019

@PierreRambaud I think no QA is needed, is it ?

@PierreRambaud
Copy link
Contributor

@matks I was thinking about it, maybe just check everything still working as expected is enough?

@Quetzacoalt91 Quetzacoalt91 added the Waiting for QA Status: action required, waiting for test feedback label Jun 11, 2019
@sarahdib sarahdib added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Jun 24, 2019
@matks
Copy link
Contributor

matks commented Jun 24, 2019

Thank you @jocel1

@matks matks merged commit 430dd95 into PrestaShop:develop Jun 24, 2019
@mvorisek
Copy link
Contributor

Instead of crc32 md5 or even hash('sha256', ) should be used to prevent collisions, i.e. to prevent not refreshing the cookie.

@eternoendless eternoendless changed the title do not set a cookie if it finally has not changed Do not set a cookie if it finally has not changed Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
develop Branch Improvement Type: Improvement QA ✔️ Status: check done, code approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants