Skip to content

Commit 5bd8ef2

Browse files
ENGCOM-4534: Fixes race condition when building merged css/js file during simultaneous requests #21756
- Merge Pull Request #21756 from Ian410/magento2:feature/merged-static-content-race-condition - Merged commits: 1. 1b46c96 2. c6affac 3. 4d37719
2 parents 1ffd8c5 + 4d37719 commit 5bd8ef2

File tree

2 files changed

+54
-20
lines changed

2 files changed

+54
-20
lines changed

lib/internal/Magento/Framework/View/Asset/MergeStrategy/Direct.php

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
namespace Magento\Framework\View\Asset\MergeStrategy;
77

88
use Magento\Framework\App\Filesystem\DirectoryList;
9+
use Magento\Framework\App\ObjectManager;
10+
use Magento\Framework\Math\Random;
911
use Magento\Framework\View\Asset;
1012

1113
/**
@@ -30,38 +32,46 @@ class Direct implements \Magento\Framework\View\Asset\MergeStrategyInterface
3032
*/
3133
private $cssUrlResolver;
3234

35+
/**
36+
* @var Random
37+
*/
38+
private $mathRandom;
39+
3340
/**
3441
* @param \Magento\Framework\Filesystem $filesystem
3542
* @param \Magento\Framework\View\Url\CssResolver $cssUrlResolver
43+
* @param Random|null $mathRandom
3644
*/
3745
public function __construct(
3846
\Magento\Framework\Filesystem $filesystem,
39-
\Magento\Framework\View\Url\CssResolver $cssUrlResolver
47+
\Magento\Framework\View\Url\CssResolver $cssUrlResolver,
48+
Random $mathRandom = null
4049
) {
4150
$this->filesystem = $filesystem;
4251
$this->cssUrlResolver = $cssUrlResolver;
52+
$this->mathRandom = $mathRandom ?: ObjectManager::getInstance()->get(Random::class);
4353
}
4454

4555
/**
46-
* {@inheritdoc}
56+
* @inheritdoc
4757
*/
4858
public function merge(array $assetsToMerge, Asset\LocalInterface $resultAsset)
4959
{
5060
$mergedContent = $this->composeMergedContent($assetsToMerge, $resultAsset);
5161
$filePath = $resultAsset->getPath();
62+
$tmpFilePath = $filePath . $this->mathRandom->getUniqueHash('_');
5263
$staticDir = $this->filesystem->getDirectoryWrite(DirectoryList::STATIC_VIEW);
5364
$tmpDir = $this->filesystem->getDirectoryWrite(DirectoryList::TMP);
54-
$tmpDir->writeFile($filePath, $mergedContent);
55-
$tmpDir->renameFile($filePath, $filePath, $staticDir);
65+
$tmpDir->writeFile($tmpFilePath, $mergedContent);
66+
$tmpDir->renameFile($tmpFilePath, $filePath, $staticDir);
5667
}
5768

5869
/**
5970
* Merge files together and modify content if needed
6071
*
61-
* @param \Magento\Framework\View\Asset\MergeableInterface[] $assetsToMerge
62-
* @param \Magento\Framework\View\Asset\LocalInterface $resultAsset
63-
* @return string
64-
* @throws \Magento\Framework\Exception\LocalizedException
72+
* @param array $assetsToMerge
73+
* @param Asset\LocalInterface $resultAsset
74+
* @return array|string
6575
*/
6676
private function composeMergedContent(array $assetsToMerge, Asset\LocalInterface $resultAsset)
6777
{

lib/internal/Magento/Framework/View/Test/Unit/Asset/MergeStrategy/DirectTest.php

Lines changed: 36 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,19 @@
55
*/
66
namespace Magento\Framework\View\Test\Unit\Asset\MergeStrategy;
77

8-
use Magento\Framework\Filesystem\Directory\WriteInterface;
9-
use \Magento\Framework\View\Asset\MergeStrategy\Direct;
10-
118
use Magento\Framework\App\Filesystem\DirectoryList;
9+
use Magento\Framework\Filesystem\Directory\WriteInterface;
10+
use Magento\Framework\View\Asset\MergeStrategy\Direct;
1211

12+
/**
13+
* Test for Magento\Framework\View\Asset\MergeStrategy\Direct.
14+
*/
1315
class DirectTest extends \PHPUnit\Framework\TestCase
1416
{
17+
/**
18+
* @var \Magento\Framework\Math\Random|\PHPUnit_Framework_MockObject_MockObject
19+
*/
20+
protected $mathRandomMock;
1521
/**
1622
* @var \Magento\Framework\View\Asset\MergeStrategy\Direct
1723
*/
@@ -50,33 +56,47 @@ protected function setUp()
5056
[DirectoryList::TMP, \Magento\Framework\Filesystem\DriverPool::FILE, $this->tmpDir],
5157
]);
5258
$this->resultAsset = $this->createMock(\Magento\Framework\View\Asset\File::class);
53-
$this->object = new Direct($filesystem, $this->cssUrlResolver);
59+
$this->mathRandomMock = $this->getMockBuilder(\Magento\Framework\Math\Random::class)
60+
->disableOriginalConstructor()
61+
->getMock();
62+
$this->object = new Direct($filesystem, $this->cssUrlResolver, $this->mathRandomMock);
5463
}
5564

5665
public function testMergeNoAssets()
5766
{
67+
$uniqId = '_b3bf82fa6e140594420fa90982a8e877';
5868
$this->resultAsset->expects($this->once())->method('getPath')->will($this->returnValue('foo/result'));
5969
$this->staticDir->expects($this->never())->method('writeFile');
60-
$this->tmpDir->expects($this->once())->method('writeFile')->with('foo/result', '');
61-
$this->tmpDir->expects($this->once())->method('renameFile')->with('foo/result', 'foo/result', $this->staticDir);
70+
$this->mathRandomMock->expects($this->once())
71+
->method('getUniqueHash')
72+
->willReturn($uniqId);
73+
$this->tmpDir->expects($this->once())->method('writeFile')->with('foo/result' . $uniqId, '');
74+
$this->tmpDir->expects($this->once())->method('renameFile')
75+
->with('foo/result' . $uniqId, 'foo/result', $this->staticDir);
6276
$this->object->merge([], $this->resultAsset);
6377
}
6478

6579
public function testMergeGeneric()
6680
{
81+
$uniqId = '_be50ccf992fd81818c1a2645d1a29e92';
6782
$this->resultAsset->expects($this->once())->method('getPath')->will($this->returnValue('foo/result'));
6883
$assets = $this->prepareAssetsToMerge([' one', 'two']); // note leading space intentionally
6984
$this->staticDir->expects($this->never())->method('writeFile');
70-
$this->tmpDir->expects($this->once())->method('writeFile')->with('foo/result', 'onetwo');
71-
$this->tmpDir->expects($this->once())->method('renameFile')->with('foo/result', 'foo/result', $this->staticDir);
85+
$this->mathRandomMock->expects($this->once())
86+
->method('getUniqueHash')
87+
->willReturn($uniqId);
88+
$this->tmpDir->expects($this->once())->method('writeFile')->with('foo/result' . $uniqId, 'onetwo');
89+
$this->tmpDir->expects($this->once())->method('renameFile')
90+
->with('foo/result' . $uniqId, 'foo/result', $this->staticDir);
7291
$this->object->merge($assets, $this->resultAsset);
7392
}
7493

7594
public function testMergeCss()
7695
{
96+
$uniqId = '_f929c374767e00712449660ea673f2f5';
7797
$this->resultAsset->expects($this->exactly(3))
7898
->method('getPath')
79-
->will($this->returnValue('foo/result'));
99+
->willReturn('foo/result');
80100
$this->resultAsset->expects($this->any())->method('getContentType')->will($this->returnValue('css'));
81101
$assets = $this->prepareAssetsToMerge(['one', 'two']);
82102
$this->cssUrlResolver->expects($this->exactly(2))
@@ -85,10 +105,14 @@ public function testMergeCss()
85105
$this->cssUrlResolver->expects($this->once())
86106
->method('aggregateImportDirectives')
87107
->with('12')
88-
->will($this->returnValue('1020'));
108+
->willReturn('1020');
109+
$this->mathRandomMock->expects($this->once())
110+
->method('getUniqueHash')
111+
->willReturn($uniqId);
89112
$this->staticDir->expects($this->never())->method('writeFile');
90-
$this->tmpDir->expects($this->once())->method('writeFile')->with('foo/result', '1020');
91-
$this->tmpDir->expects($this->once())->method('renameFile')->with('foo/result', 'foo/result', $this->staticDir);
113+
$this->tmpDir->expects($this->once())->method('writeFile')->with('foo/result' . $uniqId, '1020');
114+
$this->tmpDir->expects($this->once())->method('renameFile')
115+
->with('foo/result' . $uniqId, 'foo/result', $this->staticDir);
92116
$this->object->merge($assets, $this->resultAsset);
93117
}
94118

0 commit comments

Comments
 (0)