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

Fix PageCache: async rendering of blocks can corrupt layout cache #8554 #9050 #11174

Conversation

adrian-martinez-interactiv4
Copy link
Contributor

@adrian-martinez-interactiv4 adrian-martinez-interactiv4 commented Oct 1, 2017

Related with closed PR#9560

Description
This pull request expects to fix layout cache corruption due to async pagecache requests, when the async request generates a layout cacheId based on same handles as a cms page, but without loading the cms page associated, so cms page related layout updates get lost. Further explanation on issues #8554 and #9050. The testing scenario may sound weird, but it actually happens due to layout cache expiration, so layout cache expires but full page cache is still valid.

Fixed Issues

  1. PageCache: async rendering of blocks can corrupt layout cache #8554: PageCache: async rendering of blocks can corrupt layout cache
  2. Randomly getting an empty <body> that gets cached #9050: Randomly getting an empty body that gets cached

Manual testing scenarios

  1. Use the cms home page to add a block via Layout Update XML. (I've also experienced issued with page layout setting not being used).
  2. Ensure layout and full page cache are enabled, at least this two.
  3. Visit cms page on frontend, so layout cache and full page cache are generated.
  4. Clean layout cache: bin/magento cache:clean layout
  5. curl -gI "http://local.mg2develop.com/page_cache/block/render/?ajax=1&blocks=[%22header.login%22]&handles=[%22default%22%2C%22cms_index_index%22%2C%22cms_page_view%22%2C%22cms_index_index_id_home%22]&originalRequest=a&version="
  6. Clean only full page cache: bin/magento cache:clean full_page

Adapted Tests
\Magento\PageCache\Test\Unit\Controller\Block\RenderTest::testExecute
\Magento\PageCache\Test\Unit\Controller\Block\EsiTest::testExecute

Expected Result
Page rendered correctly, as the first time.

Actual Result
Depending on included content, full blank page may be returned, but at least, it fails to apply page layout update handles, so block added via layout update is not shown.

Refactored original solution, implemented cache keys for layout to be able to generate different unique cache ids also based on possibly added cache keys
Refactored original solution, implemented cache keys for layout to be able to generate different unique cache ids also based on possibly added cache keys
* upstream/develop: (2518 commits)
  MQE-279: Create repositories in magento organization
  MQE-279: Create repositories in magento organization
  MQE-279: Create repositories in magento organization
  Deliver magento-partners/magento2ce#25: fixed static tests failures
  magento#10930 getcachetags for price issue
  Fixed Typo Mistake in comment.
  Add escaping of js translation version
  MAGETWO-72469: Merge 2.2-develop into develop
  magento#10789: enable universal use of modal and tab_group  - Coding style updated
  magento#10848: added query param to url build  - Test Updated
  Deprecated Magento\Store\Model\Store::$_isAdminSecure
  MAGETWO-72401: fix jenkins unittest
  magento#10826: response condition order changes.  - Updated coding style
  MAGETWO-70859: Inconsistent behaviour of Scheduled Changes
  cviking26_issue-9489 // syntax hotfix
  MAGETWO-72469: Merge 2.2-develop into develop
  MAGETWO-72469: Merge 2.2-develop into develop
  cviking26_issue-9489 // changed array declaration
  cviking26_issue-9489 // added query param to url build - description in magento#9489
  MAGETWO-72046: Error processing your request when placing reorder for simple product of configurable if attribute is changed - for 2.2
  ...
…ento#8554 magento#9050 magento#9560

Create cache key object to be injected separately with its own interface
…ento#8554 magento#9050 magento#9560

Renamed interface, LayoutCacheKeyInterface made optional in constructor, injected via di.xml, some other little fixes
@vrann
Copy link
Contributor

vrann commented Oct 3, 2017

@adrian-martinez-interactiv4 why did you close the #9560 ?

@adrian-martinez-interactiv4
Copy link
Contributor Author

@vrann because the PR was my first one, and was shot from the develop branch of my fork. I wanted to clean my fork repository, also prepare my local working copy to work with develop, 2.1-develop and 2.2-develop branches, keeping them always aligned with the main repository. I know I could have switched the base branch of the PR, but since I've automatized the rebasing of this branches against the main repository to keep them updated, original PR related with my develop branch got closed.

@magento-team magento-team merged commit 3f60168 into magento:develop Oct 5, 2017
@adrian-martinez-interactiv4 adrian-martinez-interactiv4 deleted the FR#FIX-PAGECACHE-LAYOUT-CACHE-KEY branch October 10, 2017 11:59
@adrian-martinez-interactiv4 adrian-martinez-interactiv4 restored the FR#FIX-PAGECACHE-LAYOUT-CACHE-KEY branch October 10, 2017 11:59
@adrian-martinez-interactiv4 adrian-martinez-interactiv4 deleted the FR#FIX-PAGECACHE-LAYOUT-CACHE-KEY branch October 17, 2017 18:07
@pemann
Copy link

pemann commented Sep 13, 2018

Is there 2.2.x version of this?

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.

5 participants