Fixes regression bug introduced in Magento 2.1.6 where the layered navigation options are sometimes being cached using the wrong store id. #9704

hostep · 2017-05-19T18:39:57Z

Description

Fixes problem with layered navigation options being cached using the wrong store id.

This problem only exists on Magento 2.1.6, not on the develop branch.
The problem was introduced in 67b5ff7

Fixed Issues (if relevant)

Translation for layered navigation attribute option not working #9679: Translation for layered navigation attribute option not working

Manual testing scenarios

Set up a new Magento 2.1.6
Make sure you have 2 storeviews, I used the codes: 'en' and 'nl' (English and Dutch)
Go to Stores => Configuration => Web => Url Options and set 'Add Store Code to Urls' to 'Yes'
Create a new category, I used 'Test'
Open the 'Color' attribute and add 2 options:

Admin	English	Dutch
Red	Red	Rood
Black	Black	Zwart

Add the 'Color' attribute to the 'Default' attributeset
Create 2 products, assign them to the 'Test' category, make sure the first one has color 'Red' and the second one has color 'Black'
Reindex
Flush cache
VERY IMPORTANT: Do NOT use the storeswitcher on the frontend in the next steps
In the frontend, go the the test category in English: http(s)://mydomain/en/test.html
Check the color options, it will say: 'Red' and 'Black', good!
Now go to the test category in Dutch, directly using the url, not the storeswitcher: http(s)://mydomain/nl/test.html
Check the color options, it will say: 'Red' and 'Black', not good!
Flush the cache
Refresh the Dutch page
Check the color options, it will say: 'Rood' and 'Zwart', good!
Again, go to the English page using the url
Check the color options, it will say: 'Rood' and 'Zwart', not good!

Discussion

This problem exists because the $cacheKey is using the default store id every time if you don't use the storeswitcher. So the wrong info is cached and being used from the cache in the above cases.

The bug happens, because the original code calls $this->storeResolver->getCurrentStoreId(), this function checks if either the url has a '___store=xxx' param or if a cookie named 'store' exists. If neither of these things exist, it will return the default store view id.
When you use the storeswitcher, it will set a cookie and the functionality will work. But what about visitors going to the url directly just after the cache was flushed? They will put the wrong info into the cache.

This PR targets the 2.1-develop branch, as the develop branch hasn't gotten this MAGETWO-65484 feature yet, which is strange...

Inspiration of this PR came from this other PR, which fixes some similar issues: #9429

Would be really great if this could get included in the next minor release, as this is a pretty important bug which should get fixed ASAP.

It might be a good idea if this functionality is covered by some kind of test.
I have almost 0 experience with writing tests, so I won't do this for now, if somebody else is willing to do this, be my guest :)

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

maghamed · 2017-05-20T10:21:15Z

app/code/Magento/Eav/Model/Entity/Attribute/Frontend/AbstractFrontend.php

     * @param array $cacheTags
     * @codeCoverageIgnore
     */
    public function __construct(
        \Magento\Eav\Model\Entity\Attribute\Source\BooleanFactory $attrBooleanFactory,
        CacheInterface $cache = null,
-        StoreResolverInterface $storeResolver = null,
+        StoreManagerInterface $storeManager = null,


that's a backward incompatible change,
and we can't just remove existing dependency and add another one instead.

So, existing dependency should be @deprecated
while new dependency added at the end of the list

Really? It only existed in Magento 2.1.6, in no other version. It would very much surprise me if any 3rd party already extends this class.

But if you really want me to add it as a new param, then I can do that.
Please let me know.

hostep · 2017-05-21T11:24:33Z

@maghamed: I applied your requested changes, please let me know if everything is ok now. Thanks! :)

UPDATE: I forgot to mark the $storeResolver param in the constructor as @deprecated, but I don't know exactly how to do that? I can't seem to find any examples for marking a param as @deprecated at first sight in Magento's current code?

hostep · 2017-05-22T07:31:18Z

I just noticed the 3 failures in the static tests.

I don't understand the first two, because there is a doc comment about the $storeManager variable?
I understand the third one, but how can I solve it? It is being generated because we want to remain backwards compatible by not removing the $storeResolver from the constructor params. But that param isn't being used anymore, so it makes sense that a warning is being thrown.

Please let me know how to resolve these static test failures. Thanks! :)

[Tango] MAGETWO-66825: Caching of attribute options for Layered Navigation

hostep · 2017-05-22T18:01:37Z

I just see that this feature (with the same bug) has landed on the develop branch: 938313e

hostep · 2017-05-22T19:05:30Z

After figuring out how to run these tests locally, I was able to suppress the third warning.

The first two warnings feel like a bug in PHPCS itself, if I change the default assignment of the $cacheTags param to an empty array in the constructor, the static tests go through fine, so PHPCS isn't properly detecting the different params, I think.
Just a super wild guess here, but it might be fixed by upgrading PHPCS to at least version 2.0.0RC1, where they switched to using a comment tokenizer instead of a parser: squizlabs/PHP_CodeSniffer@9113cd6 (but this is only guessing, I haven't actually tested this)

Anyway, I just pushed an update to suppress the third warning.

…wrong store id.

hostep · 2017-05-25T13:11:23Z

I just pushed another update, which should fix all static tests.

I took the approach like was done in 938313e with the $defaultCacheTags member.
Not entirely sure if this doesn't break your backwards compatibility rules, but I don't know another way of fixing the static tests (which I still believe occur because of a bug in PHP CodeSniffer).

I'm also still waiting on feedback about how to mark the $storeResolver param as @deprecated.

Also: since this bug now also exists in the develop branch, who will take care of this? Should I create a new PR against the develop branch, or are you guys looking into this?

Thanks!

hostep · 2017-05-29T12:21:56Z

@maghamed or @okorshenko: it would be great if I could get some feedback on this PR, because it feels like this PR doesn't get any attention anymore from Magento. And since it fixes a regression bug it should get included in the next minor release preferably.

Thanks!

okorshenko · 2017-05-29T21:00:43Z

Hi @hostep , @ishakhsuvarov is working on this PR. Initial testing successfully passed. We are working right now on more detailed testing of this pull request.

Thank you

… the layered navigation options are sometimes being cached using the wrong store id. #9704

[EngCom] Public Pull Requests - 2.1 - MAGETWO-69665: Backport of MAGETWO-59089 for Magento 2.1: Magento 2.1.1 Problem with change currency #9841 - MAGETWO-69543: Fixes regression bug introduced in Magento 2.1.6 where the layered navigation options are sometimes being cached using the wrong store id. #9704 - MAGETWO-69151: Do not hardcode product link types [2.1 backport] #9601 - MAGETWO-69691: Travis build failures because the job exceeded the maximum time limit for jobs - 2.1-develop

magento-team · 2017-06-07T10:20:11Z

@hostep thank you for your contribution

hostep · 2017-06-07T11:28:14Z

@magento-team or @okorshenko: can you let me know who will take care of fixing this on the develop branch? Are you guys looking into this, or should I create a new PR for this?

Thanks!

ishakhsuvarov · 2017-06-07T12:13:58Z

@hostep I don't think it is in progress internally at the moment, so feel free to submit a PR for that.
Thank you

hostep · 2017-06-07T17:53:16Z

@ishakhsuvarov: new PR created for the develop branch: #9873

maghamed reviewed May 20, 2017

View reviewed changes

hostep force-pushed the issue-9679 branch from bd36eb1 to 1329691 Compare May 21, 2017 11:23

hostep referenced this pull request May 22, 2017

Merge pull request #1114 from magento-tango/MAGETWO-66825-1

938313e

[Tango] MAGETWO-66825: Caching of attribute options for Layered Navigation

hostep force-pushed the issue-9679 branch from 1329691 to d0ca637 Compare May 22, 2017 19:00

Fixes problem with layered navigation options being cached using the …

488be10

…wrong store id.

hostep force-pushed the issue-9679 branch from d0ca637 to 488be10 Compare May 25, 2017 13:07

ishakhsuvarov self-assigned this May 29, 2017

ishakhsuvarov added this to the May 2017 milestone May 29, 2017

ishakhsuvarov added the Release Line: 2.1 label May 29, 2017

okorshenko modified the milestones: May 2017, June 2017 Jun 1, 2017

magento-team merged commit 488be10 into magento:2.1-develop Jun 7, 2017

magento-team pushed a commit that referenced this pull request Jun 7, 2017

MAGETWO-69543: Fixes regression bug introduced in Magento 2.1.6 where…

a167e20

… the layered navigation options are sometimes being cached using the wrong store id. #9704

magento-team pushed a commit that referenced this pull request Jun 7, 2017

MAGETWO-69543: Fixes regression bug introduced in Magento 2.1.6 where…

2d8bb99

… the layered navigation options are sometimes being cached using the wrong store id. #9704

hostep mentioned this pull request Jun 7, 2017

Fixes layered navigation options being cached using the wrong store id. #9873

Merged

4 tasks

hostep mentioned this pull request Jun 7, 2017

Image swatch broken when EAV cache enabled after upgrade to 2.1.7 #9849

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes regression bug introduced in Magento 2.1.6 where the layered navigation options are sometimes being cached using the wrong store id. #9704

Fixes regression bug introduced in Magento 2.1.6 where the layered navigation options are sometimes being cached using the wrong store id. #9704

hostep commented May 19, 2017 •

edited by ishakhsuvarov

Loading

maghamed May 20, 2017

hostep May 20, 2017 •

edited

Loading

hostep commented May 21, 2017 •

edited

Loading

hostep commented May 22, 2017

hostep commented May 22, 2017

hostep commented May 22, 2017 •

edited

Loading

hostep commented May 25, 2017 •

edited

Loading

hostep commented May 29, 2017

okorshenko commented May 29, 2017 •

edited

Loading

magento-team commented Jun 7, 2017

hostep commented Jun 7, 2017

ishakhsuvarov commented Jun 7, 2017

hostep commented Jun 7, 2017

Fixes regression bug introduced in Magento 2.1.6 where the layered navigation options are sometimes being cached using the wrong store id. #9704

Fixes regression bug introduced in Magento 2.1.6 where the layered navigation options are sometimes being cached using the wrong store id. #9704

Conversation

hostep commented May 19, 2017 • edited by ishakhsuvarov Loading

Description

Fixed Issues (if relevant)

Manual testing scenarios

Discussion

Contribution checklist

maghamed May 20, 2017

Choose a reason for hiding this comment

hostep May 20, 2017 • edited Loading

Choose a reason for hiding this comment

hostep commented May 21, 2017 • edited Loading

hostep commented May 22, 2017

hostep commented May 22, 2017

hostep commented May 22, 2017 • edited Loading

hostep commented May 25, 2017 • edited Loading

hostep commented May 29, 2017

okorshenko commented May 29, 2017 • edited Loading

magento-team commented Jun 7, 2017

hostep commented Jun 7, 2017

ishakhsuvarov commented Jun 7, 2017

hostep commented Jun 7, 2017

hostep commented May 19, 2017 •

edited by ishakhsuvarov

Loading

hostep May 20, 2017 •

edited

Loading

hostep commented May 21, 2017 •

edited

Loading

hostep commented May 22, 2017 •

edited

Loading

hostep commented May 25, 2017 •

edited

Loading

okorshenko commented May 29, 2017 •

edited

Loading