-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Customer-data is not updates after login when full page cache disabled #6473
Conversation
I just tested @ihor-sviziev's patch and I can verify that it fixed the described issue. Although I did not test to ensure there are no unintended consequences. |
Have you been able to get this to work with you start with nothing in local storage? It seems like just removing the check for private_content_version would work all the time |
@jstrez77 yes, as I remember I had this issue even iin incognito mode |
Any updates? |
Hi, @ihor-sviziev, thank you for the contribution. |
@vrann I found that this issue was fixed in develop branch inside #4170, however it wasn't ported to 2.1. take a look on these commits: 861f596 9efb5ad I don't think that it's a good idea to set up the private version cookie even if built in full page cache is disabled. My fix fixing the same issue even when private version cookie is not set (in case when built in full page cache disabled). I see there two options:
I can update pull request according to your chose . What do you think? |
@ihor-sviziev reviewed commits and I see that the VarnishPlugin was not changed so presumably has the same issue as the BuiltinPlugin. Plan 1 makes sense: |
Revert commit 861f596
Revert commit 9efb5ad
@vrann I updated my pull request. Could you review it and if it ok - i'll create backport PR to 2.1-develop branch based on this one. |
@ihor-sviziev thank you. Testing this PR, doesn't seem like working at the moment: Does it work for you? Do you know where the prolem could be? |
It worked for me. I will check it again and provide you an answer
|
Hi @vrann I just checked, it works good for me. |
@ihor-sviziev sorry, forgot to mention, I deleted |
@vrann looks like my fix isn't cover all cases. Will test it more carefully and provide you an update. For now I'm closing this PR. |
…te-version-to-be-merged-perf mView patch update
If you disable full page cache (tested with built-in) - after login
Preconditions
Steps to reproduce
Expected result
Actual result