Fix PageCache: async rendering of blocks can corrupt layout cache

Refactored original solution, implemented cache keys for layout to be able to generate different unique cache ids also based on possibly added cache keys
magento · May 17, 2017 · 0e830c1 · 0e830c1
1 parent 1bb25ed
commit 0e830c1
Show file tree

Hide file tree

Showing 6 changed files with 139 additions and 22 deletions.
diff --git a/app/code/Magento/PageCache/Controller/Block.php b/app/code/Magento/PageCache/Controller/Block.php
@@ -30,7 +30,7 @@ abstract class Block extends \Magento\Framework\App\Action\Action
     /**
      * @var string
      */
-    private $additionalPageCacheHandle = 'mage_pagecache_additional_handle';
+    private $layoutCacheKey = 'mage_pagecache';
 
     /**
      * @param \Magento\Framework\App\Action\Context $context
@@ -68,14 +68,12 @@ protected function _getBlocks()
         $blocks = $this->jsonSerializer->unserialize($blocks);
         $handles = $this->base64jsonSerializer->unserialize($handles);
 
-        if (is_array($handles)) {
-            $handles[] = $this->additionalPageCacheHandle;
-        }
+        $layout = $this->_view->getLayout();
+        $layout->getUpdate()->addCacheKey($this->layoutCacheKey);
 
         $this->_view->loadLayout($handles, true, true, false);
         $data = [];
 
-        $layout = $this->_view->getLayout();
         foreach ($blocks as $blockName) {
             $blockInstance = $layout->getBlock($blockName);
             if (is_object($blockInstance)) {

diff --git a/app/code/Magento/PageCache/Test/Unit/Controller/Block/EsiTest.php b/app/code/Magento/PageCache/Test/Unit/Controller/Block/EsiTest.php
@@ -39,6 +39,11 @@ class EsiTest extends \PHPUnit_Framework_TestCase
      */
     protected $layoutMock;
 
+    /**
+     * @var \Magento\Framework\View\Layout\ProcessorInterface|\PHPUnit_Framework_MockObject_MockObject
+     */
+    protected $layoutProcessorMock;
+
     /**
      * @var \PHPUnit_Framework_MockObject_MockObject|\Magento\Framework\Translate\InlineInterface
      */
@@ -52,6 +57,8 @@ protected function setUp()
         $this->layoutMock = $this->getMockBuilder(\Magento\Framework\View\Layout::class)
             ->disableOriginalConstructor()->getMock();
 
+        $this->layoutProcessorMock = $this->getMockForAbstractClass(\Magento\Framework\View\Layout\ProcessorInterface::class);
+
         $contextMock =
             $this->getMockBuilder(\Magento\Framework\App\Action\Context::class)
                 ->disableOriginalConstructor()->getMock();
@@ -63,6 +70,8 @@ protected function setUp()
         $this->viewMock = $this->getMockBuilder(\Magento\Framework\App\View::class)
             ->disableOriginalConstructor()->getMock();
 
+        $this->layoutMock->expects($this->any())->method('getUpdate')->will($this->returnValue($this->layoutProcessorMock));
+
         $contextMock->expects($this->any())->method('getRequest')->will($this->returnValue($this->requestMock));
         $contextMock->expects($this->any())->method('getResponse')->will($this->returnValue($this->responseMock));
         $contextMock->expects($this->any())->method('getView')->will($this->returnValue($this->viewMock));
@@ -89,11 +98,9 @@ protected function setUp()
     public function testExecute($blockClass, $shouldSetHeaders)
     {
         $block = 'block';
-        $requestHandles = ['handle1', 'handle2'];
-        $additionalPageCacheHandle = 'mage_pagecache_additional_handle';
-        $pageCacheHandles = array_merge($requestHandles, [$additionalPageCacheHandle]);
+        $handles = ['handle1', 'handle2'];
         $html = 'some-html';
-        $mapData = [['blocks', '', json_encode([$block])], ['handles', '', base64_encode(json_encode($requestHandles))]];
+        $mapData = [['blocks', '', json_encode([$block])], ['handles', '', base64_encode(json_encode($handles))]];
 
         $blockInstance1 = $this->getMock(
             $blockClass,
@@ -108,10 +115,17 @@ public function testExecute($blockClass, $shouldSetHeaders)
 
         $this->requestMock->expects($this->any())->method('getParam')->will($this->returnValueMap($mapData));
 
-        $this->viewMock->expects($this->once())->method('loadLayout')->with($this->equalTo($pageCacheHandles));
+        $this->viewMock->expects($this->once())->method('loadLayout')->with($this->equalTo($handles));
 
         $this->viewMock->expects($this->once())->method('getLayout')->will($this->returnValue($this->layoutMock));
 
+        $this->layoutMock->expects($this->at(0))
+            ->method('getUpdate')
+            ->will($this->returnValue($this->layoutProcessorMock));
+        $this->layoutProcessorMock->expects($this->at(0))
+            ->method('addCacheKey')
+            ->willReturnSelf();
+
         $this->layoutMock->expects($this->once())
             ->method('getBlock')
             ->with($this->equalTo($block))

diff --git a/app/code/Magento/PageCache/Test/Unit/Controller/Block/RenderTest.php b/app/code/Magento/PageCache/Test/Unit/Controller/Block/RenderTest.php
@@ -44,14 +44,20 @@ class RenderTest extends \PHPUnit_Framework_TestCase
      */
     protected $layoutMock;
 
+    /**
+     * @var \Magento\Framework\View\Layout\ProcessorInterface|\PHPUnit_Framework_MockObject_MockObject
+     */
+    protected $layoutProcessorMock;
+
     /**
      * Set up before test
      */
     protected function setUp()
     {
-        $this->layoutMock = $this->getMockBuilder(
-            \Magento\Framework\View\Layout::class
-        )->disableOriginalConstructor()->getMock();
+        $this->layoutMock = $this->getMockBuilder(\Magento\Framework\View\Layout::class)
+            ->disableOriginalConstructor()->getMock();
+
+        $this->layoutProcessorMock = $this->getMockForAbstractClass(\Magento\Framework\View\Layout\ProcessorInterface::class);
 
         $contextMock = $this->getMockBuilder(\Magento\Framework\App\Action\Context::class)
             ->disableOriginalConstructor()->getMock();
@@ -65,6 +71,8 @@ protected function setUp()
         $this->viewMock = $this->getMockBuilder(\Magento\Framework\App\View::class)
             ->disableOriginalConstructor()->getMock();
 
+        $this->layoutMock->expects($this->any())->method('getUpdate')->will($this->returnValue($this->layoutProcessorMock));
+
         $contextMock->expects($this->any())->method('getRequest')->will($this->returnValue($this->requestMock));
         $contextMock->expects($this->any())->method('getResponse')->will($this->returnValue($this->responseMock));
         $contextMock->expects($this->any())->method('getView')->will($this->returnValue($this->viewMock));
@@ -111,10 +119,7 @@ public function testExecuteNoParams()
     public function testExecute()
     {
         $blocks = ['block1', 'block2'];
-        $requestHandles = ['handle1', 'handle2'];
-        $additionalPageCacheHandle = 'mage_pagecache_additional_handle';
-        $pageCacheHandles = array_merge($requestHandles, [$additionalPageCacheHandle]);
-
+        $handles = ['handle1', 'handle2'];
         $originalRequest = '{"route":"route","controller":"controller","action":"action","uri":"uri"}';
         $expectedData = ['block1' => 'data1', 'block2' => 'data2'];
 
@@ -162,14 +167,20 @@ public function testExecute()
         $this->requestMock->expects($this->at(11))
             ->method('getParam')
             ->with($this->equalTo('handles'), $this->equalTo(''))
-            ->will($this->returnValue(base64_encode(json_encode($requestHandles))));
-        $this->viewMock->expects($this->once())->method('loadLayout')->with($this->equalTo($pageCacheHandles));
+            ->will($this->returnValue(base64_encode(json_encode($handles))));
+        $this->viewMock->expects($this->once())->method('loadLayout')->with($this->equalTo($handles));
         $this->viewMock->expects($this->any())->method('getLayout')->will($this->returnValue($this->layoutMock));
         $this->layoutMock->expects($this->at(0))
+            ->method('getUpdate')
+            ->will($this->returnValue($this->layoutProcessorMock));
+        $this->layoutProcessorMock->expects($this->at(0))
+            ->method('addCacheKey')
+            ->willReturnSelf();
+        $this->layoutMock->expects($this->at(1))
             ->method('getBlock')
             ->with($this->equalTo($blocks[0]))
             ->will($this->returnValue($blockInstance1));
-        $this->layoutMock->expects($this->at(1))
+        $this->layoutMock->expects($this->at(2))
             ->method('getBlock')
             ->with($this->equalTo($blocks[1]))
             ->will($this->returnValue($blockInstance2));

diff --git a/app/etc/env.php.original b/app/etc/env.php.original
@@ -0,0 +1,62 @@
+<?php
+return array (
+  'backend' => 
+  array (
+    'frontName' => 'suadmin',
+  ),
+  'crypt' => 
+  array (
+    'key' => '5b2bd4f91709ea56ce8a0b1bc00b67a0',
+  ),
+  'session' => 
+  array (
+    'save' => 'files',
+  ),
+  'db' => 
+  array (
+    'table_prefix' => '',
+    'connection' => 
+    array (
+      'default' => 
+      array (
+        'host' => 'localhost',
+        'dbname' => 'mg2develop',
+        'username' => 'zend',
+        'password' => '',
+        'model' => 'mysql4',
+        'engine' => 'innodb',
+        'initStatements' => 'SET NAMES utf8;',
+        'active' => '1',
+      ),
+    ),
+  ),
+  'resource' => 
+  array (
+    'default_setup' => 
+    array (
+      'connection' => 'default',
+    ),
+  ),
+  'x-frame-options' => 'SAMEORIGIN',
+  'MAGE_MODE' => 'default',
+  'cache_types' => 
+  array (
+    'config' => 1,
+    'layout' => 1,
+    'block_html' => 1,
+    'collections' => 1,
+    'reflection' => 1,
+    'db_ddl' => 1,
+    'eav' => 1,
+    'customer_notification' => 1,
+    'config_integration' => 1,
+    'config_integration_api' => 1,
+    'full_page' => 1,
+    'translate' => 1,
+    'config_webservice' => 1,
+  ),
+  'install' => 
+  array (
+    'date' => 'Mon, 08 May 2017 23:53:11 +0000',
+  ),
+);
diff --git a/lib/internal/Magento/Framework/View/Layout/ProcessorInterface.php b/lib/internal/Magento/Framework/View/Layout/ProcessorInterface.php
@@ -126,7 +126,15 @@ public function getFileLayoutUpdatesXml();
     public function getContainers();
 
     /**
-     * Return cache ID based current area/package/theme/store and handles
+     * Add cache key for generating different cache id for same handles
+     *
+     * @param array|string $cacheKey
+     * @return ProcessorInterface
+     */
+    public function addCacheKey($cacheKey);
+
+    /**
+     * Return cache ID based current area/package/theme/store, handles and cache key(s)
      *
      * @return string
      */

diff --git a/lib/internal/Magento/Framework/View/Model/Layout/Merge.php b/lib/internal/Magento/Framework/View/Model/Layout/Merge.php
@@ -9,6 +9,7 @@
 use Magento\Framework\Config\Dom\ValidationException;
 use Magento\Framework\Filesystem\DriverPool;
 use Magento\Framework\Filesystem\File\ReadFactory;
+use Magento\Framework\View\Layout\ProcessorInterface;
 use Magento\Framework\View\Model\Layout\Update\Validator;
 
 /**
@@ -128,6 +129,13 @@ class Merge implements \Magento\Framework\View\Layout\ProcessorInterface
      */
     protected $cacheSuffix;
 
+    /**
+     * Cache keys to be able to generate different cache id for same handles
+     *
+     * @var array
+     */
+    protected $cacheKeys = [];
+
     /**
      * All processed handles used in this update
      *
@@ -907,13 +915,29 @@ public function getScope()
         return $this->scope;
     }
 
+    /**
+     * Add cache key(s) for generating different cache id for same handles
+     *
+     * @param array|string $cacheKeys
+     * @return $this
+     */
+    public function addCacheKey($cacheKeys)
+    {
+        if (!is_array($cacheKeys)) {
+            $cacheKeys = [$cacheKeys];
+        }
+        $this->cacheKeys = array_merge($this->cacheKeys, $cacheKeys);
+
+        return $this;
+    }
+
     /**
      * Return cache ID based current area/package/theme/store and handles
      *
      * @return string
      */
     public function getCacheId()
     {
-        return $this->generateCacheId(md5(implode('|', $this->getHandles())));
+        return $this->generateCacheId(md5(implode('|', array_merge($this->getHandles(), $this->cacheKeys))));
     }
 }