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

Backport 2.2 Set cache id prefix on installation #22351

Closed
wants to merge 13 commits into from
Closed

Backport 2.2 Set cache id prefix on installation #22351

wants to merge 13 commits into from

Conversation

Ctucker9233
Copy link

@Ctucker9233 Ctucker9233 commented Apr 15, 2019

Original pull request #18641 by @schmengler

Description (*)

The undocumented id_prefix option for the cache frontend is used to prefix cache keys. If it is not set, Magento uses the first 12 bits of the md5 hash of the absolute path to Magentos app/etc directory. But if this is not exactly the same on all web servers, cache invalidation does not work.

Source: https://github.com/magento/magento2/blob/2.3-develop/lib/internal/Magento/Framework/App/Cache/Frontend/Factory.php#L121-L130

To prevent this issue, the value shall be set on installation, so that the fall back on the fly does not happen anymore. Optionally, the value can be specified explicitly.

Fixed Issues (if relevant)

  1. Multisite installation, default website slow (X-Magento-Vary) #15828: Multisite installation, default website slow

Manual testing scenarios (*)

  1. Install Magento via CLI or Web Wizard
  2. There should be new options "cache-id-prefix" and "page-cache-id-prefix"
  3. The value is saved in env.php at cache/frontend/{default,page_cache}/id_prefix

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)

@m2-assistant
Copy link

m2-assistant bot commented Apr 15, 2019

Hi @Ctucker9233. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.2-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

Copy link
Contributor

@orlangur orlangur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ctucker9233 original commit author must be preserved

@ghost ghost assigned orlangur Apr 15, 2019
@Ctucker9233
Copy link
Author

@orlangur I've added the author's name to the description. Is that sufficient?

@Ctucker9233 Ctucker9233 requested a review from orlangur April 16, 2019 00:10
@orlangur
Copy link
Contributor

@Ctucker9233 no. You need to rewrite branch history so that each commit corresponds to original one preserving authorship: https://github.com/magento/magento2/pull/18641/commits

@Ctucker9233
Copy link
Author

@orlangur I'm not able to do that. If you could assist me I'd appreciate it. If not, just close.

@Ctucker9233
Copy link
Author

@orlangur are you talking about transfering the ownership of the repository? I can see where to do that. Going forward, to preserve author info, should I have submitted the pull request from the original author's branch? I'm sorry I'm having trouble understanding how this works.

@schmengler
Copy link
Contributor

@Ctucker9233 No, thank you, I don't need ownership of your fork :) If you used git cherry-pick or git rebase, the original author should have been preserved. It looks like you applied the commits manually on your own instead.

@orlangur
Copy link
Contributor

@Ctucker9233 just check any other backport - you'll see "author" and "committer" in each commit.

The easiest way to fix is

  • create a new local branch based on 2.2-develop named like this PR branch Ctucker9233-issue-18641
  • cherry pick four commits one by one as Fabian suggested
  • apply additional fixes in separate commit if needed
  • force push this newly created local branch

I can add more details if you get stuck on any step.

@Ctucker9233
Copy link
Author

@orlangur @schmengler I appreciate your help guys. I think this should be closed for now. Unfortunately, I need more time to play around with it and figure out how to do it. Or mark it up for grabs.

@Ctucker9233
Copy link
Author

Ok, trying this againn using cherry-pick #22439

@m2-assistant
Copy link

m2-assistant bot commented Apr 20, 2019

Hi @Ctucker9233, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

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

Successfully merging this pull request may close these issues.

4 participants