Skip to content

Commit 0e830c1

Browse files
adrian-martinez-interactiv4adrian-martinez-interactiv4
committed
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
1 parent 1bb25ed commit 0e830c1

File tree

6 files changed

+139
-22
lines changed

6 files changed

+139
-22
lines changed

‎app/code/Magento/PageCache/Controller/Block.php

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ abstract class Block extends \Magento\Framework\App\Action\Action
3030
/**
3131
* @var string
3232
*/
33-
private $additionalPageCacheHandle = 'mage_pagecache_additional_handle';
33+
private $layoutCacheKey = 'mage_pagecache';
3434

3535
/**
3636
* @param \Magento\Framework\App\Action\Context $context
@@ -68,14 +68,12 @@ protected function _getBlocks()
6868
$blocks = $this->jsonSerializer->unserialize($blocks);
6969
$handles = $this->base64jsonSerializer->unserialize($handles);
7070

71-
if (is_array($handles)) {
72-
$handles[] = $this->additionalPageCacheHandle;
73-
}
71+
$layout = $this->_view->getLayout();
72+
$layout->getUpdate()->addCacheKey($this->layoutCacheKey);
7473

7574
$this->_view->loadLayout($handles, true, true, false);
7675
$data = [];
7776

78-
$layout = $this->_view->getLayout();
7977
foreach ($blocks as $blockName) {
8078
$blockInstance = $layout->getBlock($blockName);
8179
if (is_object($blockInstance)) {

‎app/code/Magento/PageCache/Test/Unit/Controller/Block/EsiTest.php

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,11 @@ class EsiTest extends \PHPUnit_Framework_TestCase
3939
*/
4040
protected $layoutMock;
4141

42+
/**
43+
* @var \Magento\Framework\View\Layout\ProcessorInterface|\PHPUnit_Framework_MockObject_MockObject
44+
*/
45+
protected $layoutProcessorMock;
46+
4247
/**
4348
* @var \PHPUnit_Framework_MockObject_MockObject|\Magento\Framework\Translate\InlineInterface
4449
*/
@@ -52,6 +57,8 @@ protected function setUp()
5257
$this->layoutMock = $this->getMockBuilder(\Magento\Framework\View\Layout::class)
5358
->disableOriginalConstructor()->getMock();
5459

60+
$this->layoutProcessorMock = $this->getMockForAbstractClass(\Magento\Framework\View\Layout\ProcessorInterface::class);
61+
5562
$contextMock =
5663
$this->getMockBuilder(\Magento\Framework\App\Action\Context::class)
5764
->disableOriginalConstructor()->getMock();
@@ -63,6 +70,8 @@ protected function setUp()
6370
$this->viewMock = $this->getMockBuilder(\Magento\Framework\App\View::class)
6471
->disableOriginalConstructor()->getMock();
6572

73+
$this->layoutMock->expects($this->any())->method('getUpdate')->will($this->returnValue($this->layoutProcessorMock));
74+
6675
$contextMock->expects($this->any())->method('getRequest')->will($this->returnValue($this->requestMock));
6776
$contextMock->expects($this->any())->method('getResponse')->will($this->returnValue($this->responseMock));
6877
$contextMock->expects($this->any())->method('getView')->will($this->returnValue($this->viewMock));
@@ -89,11 +98,9 @@ protected function setUp()
8998
public function testExecute($blockClass, $shouldSetHeaders)
9099
{
91100
$block = 'block';
92-
$requestHandles = ['handle1', 'handle2'];
93-
$additionalPageCacheHandle = 'mage_pagecache_additional_handle';
94-
$pageCacheHandles = array_merge($requestHandles, [$additionalPageCacheHandle]);
101+
$handles = ['handle1', 'handle2'];
95102
$html = 'some-html';
96-
$mapData = [['blocks', '', json_encode([$block])], ['handles', '', base64_encode(json_encode($requestHandles))]];
103+
$mapData = [['blocks', '', json_encode([$block])], ['handles', '', base64_encode(json_encode($handles))]];
97104

98105
$blockInstance1 = $this->getMock(
99106
$blockClass,
@@ -108,10 +115,17 @@ public function testExecute($blockClass, $shouldSetHeaders)
108115

109116
$this->requestMock->expects($this->any())->method('getParam')->will($this->returnValueMap($mapData));
110117

111-
$this->viewMock->expects($this->once())->method('loadLayout')->with($this->equalTo($pageCacheHandles));
118+
$this->viewMock->expects($this->once())->method('loadLayout')->with($this->equalTo($handles));
112119

113120
$this->viewMock->expects($this->once())->method('getLayout')->will($this->returnValue($this->layoutMock));
114121

122+
$this->layoutMock->expects($this->at(0))
123+
->method('getUpdate')
124+
->will($this->returnValue($this->layoutProcessorMock));
125+
$this->layoutProcessorMock->expects($this->at(0))
126+
->method('addCacheKey')
127+
->willReturnSelf();
128+
115129
$this->layoutMock->expects($this->once())
116130
->method('getBlock')
117131
->with($this->equalTo($block))

‎app/code/Magento/PageCache/Test/Unit/Controller/Block/RenderTest.php

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -44,14 +44,20 @@ class RenderTest extends \PHPUnit_Framework_TestCase
4444
*/
4545
protected $layoutMock;
4646

47+
/**
48+
* @var \Magento\Framework\View\Layout\ProcessorInterface|\PHPUnit_Framework_MockObject_MockObject
49+
*/
50+
protected $layoutProcessorMock;
51+
4752
/**
4853
* Set up before test
4954
*/
5055
protected function setUp()
5156
{
52-
$this->layoutMock = $this->getMockBuilder(
53-
\Magento\Framework\View\Layout::class
54-
)->disableOriginalConstructor()->getMock();
57+
$this->layoutMock = $this->getMockBuilder(\Magento\Framework\View\Layout::class)
58+
->disableOriginalConstructor()->getMock();
59+
60+
$this->layoutProcessorMock = $this->getMockForAbstractClass(\Magento\Framework\View\Layout\ProcessorInterface::class);
5561

5662
$contextMock = $this->getMockBuilder(\Magento\Framework\App\Action\Context::class)
5763
->disableOriginalConstructor()->getMock();
@@ -65,6 +71,8 @@ protected function setUp()
6571
$this->viewMock = $this->getMockBuilder(\Magento\Framework\App\View::class)
6672
->disableOriginalConstructor()->getMock();
6773

74+
$this->layoutMock->expects($this->any())->method('getUpdate')->will($this->returnValue($this->layoutProcessorMock));
75+
6876
$contextMock->expects($this->any())->method('getRequest')->will($this->returnValue($this->requestMock));
6977
$contextMock->expects($this->any())->method('getResponse')->will($this->returnValue($this->responseMock));
7078
$contextMock->expects($this->any())->method('getView')->will($this->returnValue($this->viewMock));
@@ -111,10 +119,7 @@ public function testExecuteNoParams()
111119
public function testExecute()
112120
{
113121
$blocks = ['block1', 'block2'];
114-
$requestHandles = ['handle1', 'handle2'];
115-
$additionalPageCacheHandle = 'mage_pagecache_additional_handle';
116-
$pageCacheHandles = array_merge($requestHandles, [$additionalPageCacheHandle]);
117-
122+
$handles = ['handle1', 'handle2'];
118123
$originalRequest = '{"route":"route","controller":"controller","action":"action","uri":"uri"}';
119124
$expectedData = ['block1' => 'data1', 'block2' => 'data2'];
120125

@@ -162,14 +167,20 @@ public function testExecute()
162167
$this->requestMock->expects($this->at(11))
163168
->method('getParam')
164169
->with($this->equalTo('handles'), $this->equalTo(''))
165-
->will($this->returnValue(base64_encode(json_encode($requestHandles))));
166-
$this->viewMock->expects($this->once())->method('loadLayout')->with($this->equalTo($pageCacheHandles));
170+
->will($this->returnValue(base64_encode(json_encode($handles))));
171+
$this->viewMock->expects($this->once())->method('loadLayout')->with($this->equalTo($handles));
167172
$this->viewMock->expects($this->any())->method('getLayout')->will($this->returnValue($this->layoutMock));
168173
$this->layoutMock->expects($this->at(0))
174+
->method('getUpdate')
175+
->will($this->returnValue($this->layoutProcessorMock));
176+
$this->layoutProcessorMock->expects($this->at(0))
177+
->method('addCacheKey')
178+
->willReturnSelf();
179+
$this->layoutMock->expects($this->at(1))
169180
->method('getBlock')
170181
->with($this->equalTo($blocks[0]))
171182
->will($this->returnValue($blockInstance1));
172-
$this->layoutMock->expects($this->at(1))
183+
$this->layoutMock->expects($this->at(2))
173184
->method('getBlock')
174185
->with($this->equalTo($blocks[1]))
175186
->will($this->returnValue($blockInstance2));

‎app/etc/env.php.original

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
<?php
2+
return array (
3+
'backend' =>
4+
array (
5+
'frontName' => 'suadmin',
6+
),
7+
'crypt' =>
8+
array (
9+
'key' => '5b2bd4f91709ea56ce8a0b1bc00b67a0',
10+
),
11+
'session' =>
12+
array (
13+
'save' => 'files',
14+
),
15+
'db' =>
16+
array (
17+
'table_prefix' => '',
18+
'connection' =>
19+
array (
20+
'default' =>
21+
array (
22+
'host' => 'localhost',
23+
'dbname' => 'mg2develop',
24+
'username' => 'zend',
25+
'password' => '',
26+
'model' => 'mysql4',
27+
'engine' => 'innodb',
28+
'initStatements' => 'SET NAMES utf8;',
29+
'active' => '1',
30+
),
31+
),
32+
),
33+
'resource' =>
34+
array (
35+
'default_setup' =>
36+
array (
37+
'connection' => 'default',
38+
),
39+
),
40+
'x-frame-options' => 'SAMEORIGIN',
41+
'MAGE_MODE' => 'default',
42+
'cache_types' =>
43+
array (
44+
'config' => 1,
45+
'layout' => 1,
46+
'block_html' => 1,
47+
'collections' => 1,
48+
'reflection' => 1,
49+
'db_ddl' => 1,
50+
'eav' => 1,
51+
'customer_notification' => 1,
52+
'config_integration' => 1,
53+
'config_integration_api' => 1,
54+
'full_page' => 1,
55+
'translate' => 1,
56+
'config_webservice' => 1,
57+
),
58+
'install' =>
59+
array (
60+
'date' => 'Mon, 08 May 2017 23:53:11 +0000',
61+
),
62+
);

‎lib/internal/Magento/Framework/View/Layout/ProcessorInterface.php

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,15 @@ public function getFileLayoutUpdatesXml();
126126
public function getContainers();
127127

128128
/**
129-
* Return cache ID based current area/package/theme/store and handles
129+
* Add cache key for generating different cache id for same handles
130+
*
131+
* @param array|string $cacheKey
132+
* @return ProcessorInterface
133+
*/
134+
public function addCacheKey($cacheKey);
135+
136+
/**
137+
* Return cache ID based current area/package/theme/store, handles and cache key(s)
130138
*
131139
* @return string
132140
*/

‎lib/internal/Magento/Framework/View/Model/Layout/Merge.php

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
use Magento\Framework\Config\Dom\ValidationException;
1010
use Magento\Framework\Filesystem\DriverPool;
1111
use Magento\Framework\Filesystem\File\ReadFactory;
12+
use Magento\Framework\View\Layout\ProcessorInterface;
1213
use Magento\Framework\View\Model\Layout\Update\Validator;
1314

1415
/**
@@ -128,6 +129,13 @@ class Merge implements \Magento\Framework\View\Layout\ProcessorInterface
128129
*/
129130
protected $cacheSuffix;
130131

132+
/**
133+
* Cache keys to be able to generate different cache id for same handles
134+
*
135+
* @var array
136+
*/
137+
protected $cacheKeys = [];
138+
131139
/**
132140
* All processed handles used in this update
133141
*
@@ -907,13 +915,29 @@ public function getScope()
907915
return $this->scope;
908916
}
909917

918+
/**
919+
* Add cache key(s) for generating different cache id for same handles
920+
*
921+
* @param array|string $cacheKeys
922+
* @return $this
923+
*/
924+
public function addCacheKey($cacheKeys)
925+
{
926+
if (!is_array($cacheKeys)) {
927+
$cacheKeys = [$cacheKeys];
928+
}
929+
$this->cacheKeys = array_merge($this->cacheKeys, $cacheKeys);
930+
931+
return $this;
932+
}
933+
910934
/**
911935
* Return cache ID based current area/package/theme/store and handles
912936
*
913937
* @return string
914938
*/
915939
public function getCacheId()
916940
{
917-
return $this->generateCacheId(md5(implode('|', $this->getHandles())));
941+
return $this->generateCacheId(md5(implode('|', array_merge($this->getHandles(), $this->cacheKeys))));
918942
}
919943
}

0 commit comments

Comments
 (0)