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

[Forwardport] Prevent layout cache corruption in edge case #16487

Conversation

gelanivishal
Copy link
Contributor

Original Pull Request

#12314

Description

When layout is loaded from the cache, it ought to repopulate the list of applied layout handles to prevent any chance of a layout handle being reapplied later, layering on top of the XML loaded from cache. This pull request adds that missing functionality.

The bug caused by the status quo is triggered when layout is loaded early before the page actually begins to render and before all layout handles may be applied. (Layout is loaded early, for example, when $resultPage->getConfig()->getTitle() is called, as happens in multiple core controllers, including checkout.) If this initial layout load matches a cache key but the latter layout load (with all layout handles assembled) does not, then when the latter layout load occurs, the guard condition which attempts to block duplicate layout handle application (in _merge of Magento\Framework\View\Model\Layout\Merge) does not get triggered, since the layout handles loaded from cache were not populated.

This duplicate XML generally does not cause issues, because earlier nodes are replaced by later nodes via the _overrideElementWorkaround method in \Magento\Framework\View\Layout\ScheduledStructure\Helper, and since the duplicate nodes are the same as the originals, there's no problem. However, because this method removes the entire tree (including descendants) of the first instance of duplicated nodes, it can lead to nodes missing from the final structure, depending on the layout order sequence. (For example, if a parent node such as <container name="content" label="Main Content Area"/> ends up appearing after some element that was supposed to be inserted into that container, the child block never gets re-added.) This method's algorithm is a little clunky itself and could probably be refined, but by merging this fix, we avoid at least one scenario where this algorithm is even needed.

Manual testing scenarios

As far as I've seen this does not cause any issues in core alone, but it can cause issues when custom modules extend core in standard ways. This describes one concrete case where this can occur, but it is likely to occur in other cases as well.

  1. Create a custom module:
    1. Add an afterExecute plugin on Magento\Checkout\Controller\Index\Index.
    2. In this plugin method, conditionally add a layout handle if the cart contains a particular product.
  2. Enable and clear layout cache.
  3. Add a product to the cart other than the one checked in the plugin condition.
  4. Load checkout. (Checkout should appear correctly.)
  5. Add the product to the cart checked in the plugin condition.
  6. Load checkout again. (Without this fix, checkout will break, as the customer.customer.data block will not be loaded. With this fix, checkout will load correctly.)

@magento-engcom-team
Copy link
Contributor

Hi @gelanivishal. 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 {$VERSION} instance - deploy vanilla Magento instance

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

@magento-engcom-team
Copy link
Contributor

Hi @orlangur, thank you for the review.
ENGCOM-2164 has been created to process this Pull Request

@magento-engcom-team
Copy link
Contributor

Hi @gelanivishal. Thank you for your contribution.
We will aim to release these changes as part of 2.3.0.
Please check the release notes for final confirmation.

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