From 0e830c1829a958774eb2c72891b6de6dba76d9d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Mart=C3=ADnez?= Date: Wed, 17 May 2017 02:21:14 +0200 Subject: [PATCH] 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/PageCache/Controller/Block.php | 8 +-- .../Test/Unit/Controller/Block/EsiTest.php | 24 +++++-- .../Test/Unit/Controller/Block/RenderTest.php | 31 +++++++--- app/etc/env.php.original | 62 +++++++++++++++++++ .../View/Layout/ProcessorInterface.php | 10 ++- .../Framework/View/Model/Layout/Merge.php | 26 +++++++- 6 files changed, 139 insertions(+), 22 deletions(-) create mode 100644 app/etc/env.php.original diff --git a/app/code/Magento/PageCache/Controller/Block.php b/app/code/Magento/PageCache/Controller/Block.php index 1721f9dcae9fd..b4fc8bcd79482 100644 --- 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 index 6e427eb2d9cf5..a7c4de2575ce5 100644 --- 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 index 735b4412d3ff6..e2337f5615121 100644 --- 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 new file mode 100644 index 0000000000000..74a48ba9db8ee --- /dev/null +++ b/app/etc/env.php.original @@ -0,0 +1,62 @@ + + 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 index eb4771b1b89d8..0c2f050f9d83e 100644 --- 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 index 97cbeb83a7a86..a4174aafd1947 100644 --- 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,6 +915,22 @@ 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 * @@ -914,6 +938,6 @@ public function getScope() */ public function getCacheId() { - return $this->generateCacheId(md5(implode('|', $this->getHandles()))); + return $this->generateCacheId(md5(implode('|', array_merge($this->getHandles(), $this->cacheKeys)))); } }