From 78ec570236cd0f696dbc6e3932dc937bb4e47493 Mon Sep 17 00:00:00 2001 From: Ryan Weaver Date: Fri, 27 Oct 2023 11:53:57 -0400 Subject: [PATCH] [AssetMapper] Fixing memory bug where we stored way more file content than needed --- .../Command/AssetMapperCompileCommand.php | 4 +-- .../Factory/MappedAssetFactory.php | 26 ++++++++--------- .../Component/AssetMapper/MappedAsset.php | 4 ++- ...apperDevServerSubscriberFunctionalTest.php | 7 +++-- .../Tests/Factory/MappedAssetFactoryTest.php | 28 +++++++++++++++---- 5 files changed, 42 insertions(+), 27 deletions(-) diff --git a/src/Symfony/Component/AssetMapper/Command/AssetMapperCompileCommand.php b/src/Symfony/Component/AssetMapper/Command/AssetMapperCompileCommand.php index 7bb8accd74abf..9e25a34894818 100644 --- a/src/Symfony/Component/AssetMapper/Command/AssetMapperCompileCommand.php +++ b/src/Symfony/Component/AssetMapper/Command/AssetMapperCompileCommand.php @@ -104,11 +104,9 @@ private function shortenPath(string $path): string private function createManifestAndWriteFiles(SymfonyStyle $io): array { - $allAssets = $this->assetMapper->allAssets(); - $io->comment(sprintf('Compiling and writing asset files to %s', $this->shortenPath($this->assetsFilesystem->getDestinationPath()))); $manifest = []; - foreach ($allAssets as $asset) { + foreach ($this->assetMapper->allAssets() as $asset) { if (null !== $asset->content) { // The original content has been modified by the AssetMapperCompiler $this->assetsFilesystem->write($asset->publicPath, $asset->content); diff --git a/src/Symfony/Component/AssetMapper/Factory/MappedAssetFactory.php b/src/Symfony/Component/AssetMapper/Factory/MappedAssetFactory.php index f38af362ca21f..91c4dc2717939 100644 --- a/src/Symfony/Component/AssetMapper/Factory/MappedAssetFactory.php +++ b/src/Symfony/Component/AssetMapper/Factory/MappedAssetFactory.php @@ -26,7 +26,6 @@ class MappedAssetFactory implements MappedAssetFactoryInterface private array $assetsCache = []; private array $assetsBeingCreated = []; - private array $fileContentsCache = []; public function __construct( private readonly PublicAssetsPathResolverInterface $assetsPathResolver, @@ -46,14 +45,15 @@ public function createMappedAsset(string $logicalPath, string $sourcePath): ?Map $isVendor = $this->isVendor($sourcePath); $asset = new MappedAsset($logicalPath, $sourcePath, $this->assetsPathResolver->resolvePublicPath($logicalPath), isVendor: $isVendor); - [$digest, $isPredigested] = $this->getDigest($asset); + $content = $this->compileContent($asset); + [$digest, $isPredigested] = $this->getDigest($asset, $content); $asset = new MappedAsset( $asset->logicalPath, $asset->sourcePath, $asset->publicPathWithoutDigest, - $this->getPublicPath($asset), - $this->compileContent($asset), + $this->getPublicPath($asset, $content), + $content, $digest, $isPredigested, $isVendor, @@ -75,7 +75,7 @@ public function createMappedAsset(string $logicalPath, string $sourcePath): ?Map * * @return array{0: string, 1: bool} */ - private function getDigest(MappedAsset $asset): array + private function getDigest(MappedAsset $asset, ?string $content): array { // check for a pre-digested file if (preg_match(self::PREDIGESTED_REGEX, $asset->logicalPath, $matches)) { @@ -83,7 +83,7 @@ private function getDigest(MappedAsset $asset): array } // Use the compiled content if any - if (null !== $content = $this->compileContent($asset)) { + if (null !== $content) { return [hash('xxh128', $content), false]; } @@ -95,27 +95,23 @@ private function getDigest(MappedAsset $asset): array private function compileContent(MappedAsset $asset): ?string { - if (\array_key_exists($asset->logicalPath, $this->fileContentsCache)) { - return $this->fileContentsCache[$asset->logicalPath]; - } - if (!is_file($asset->sourcePath)) { throw new RuntimeException(sprintf('Asset source path "%s" could not be found.', $asset->sourcePath)); } if (!$this->compiler->supports($asset)) { - return $this->fileContentsCache[$asset->logicalPath] = null; + return null; } $content = file_get_contents($asset->sourcePath); - $content = $this->compiler->compile($content, $asset); + $compiled = $this->compiler->compile($content, $asset); - return $this->fileContentsCache[$asset->logicalPath] = $content; + return $compiled !== $content ? $compiled : null; } - private function getPublicPath(MappedAsset $asset): ?string + private function getPublicPath(MappedAsset $asset, ?string $content): ?string { - [$digest, $isPredigested] = $this->getDigest($asset); + [$digest, $isPredigested] = $this->getDigest($asset, $content); if ($isPredigested) { return $this->assetsPathResolver->resolvePublicPath($asset->logicalPath); diff --git a/src/Symfony/Component/AssetMapper/MappedAsset.php b/src/Symfony/Component/AssetMapper/MappedAsset.php index 467643fce336e..66959de7636bc 100644 --- a/src/Symfony/Component/AssetMapper/MappedAsset.php +++ b/src/Symfony/Component/AssetMapper/MappedAsset.php @@ -26,7 +26,9 @@ final class MappedAsset public readonly string $publicExtension; /** - * The final content of this asset, if different from the source. + * The final content of this asset if different from the sourcePath. + * + * If null, the content should be read from the sourcePath. */ public readonly ?string $content; diff --git a/src/Symfony/Component/AssetMapper/Tests/AssetMapperDevServerSubscriberFunctionalTest.php b/src/Symfony/Component/AssetMapper/Tests/AssetMapperDevServerSubscriberFunctionalTest.php index 5a64ab6f9b299..f83ff87f9426c 100644 --- a/src/Symfony/Component/AssetMapper/Tests/AssetMapperDevServerSubscriberFunctionalTest.php +++ b/src/Symfony/Component/AssetMapper/Tests/AssetMapperDevServerSubscriberFunctionalTest.php @@ -13,6 +13,7 @@ use Symfony\Bundle\FrameworkBundle\Test\WebTestCase; use Symfony\Component\AssetMapper\Tests\Fixtures\AssetMapperTestAppKernel; +use Symfony\Component\HttpFoundation\BinaryFileResponse; class AssetMapperDevServerSubscriberFunctionalTest extends WebTestCase { @@ -23,11 +24,12 @@ public function testGettingAssetWorks() $client->request('GET', '/assets/file1-b3445cb7a86a0795a7af7f2004498aef.css'); $response = $client->getResponse(); $this->assertSame(200, $response->getStatusCode()); + $this->assertInstanceOf(BinaryFileResponse::class, $response); $this->assertSame(<<getContent()); + EOF, $response->getFile()->getContent()); $this->assertSame('"b3445cb7a86a0795a7af7f2004498aef"', $response->headers->get('ETag')); $this->assertSame('immutable, max-age=604800, public', $response->headers->get('Cache-Control')); $this->assertTrue($response->headers->has('X-Assets-Dev')); @@ -59,11 +61,12 @@ public function testPreDigestedAssetIsReturned() $client->request('GET', '/assets/already-abcdefVWXYZ0123456789.digested.css'); $response = $client->getResponse(); $this->assertSame(200, $response->getStatusCode()); + $this->assertInstanceOf(BinaryFileResponse::class, $response); $this->assertSame(<<getContent()); + EOF, $response->getFile()->getContent()); } protected static function getKernelClass(): string diff --git a/src/Symfony/Component/AssetMapper/Tests/Factory/MappedAssetFactoryTest.php b/src/Symfony/Component/AssetMapper/Tests/Factory/MappedAssetFactoryTest.php index 0d83eca5f410f..5877135c4b9db 100644 --- a/src/Symfony/Component/AssetMapper/Tests/Factory/MappedAssetFactoryTest.php +++ b/src/Symfony/Component/AssetMapper/Tests/Factory/MappedAssetFactoryTest.php @@ -48,14 +48,22 @@ public function testCreateMappedAssetRespectsPreDigestedPaths() $this->assertSame('/final-assets/already-abcdefVWXYZ0123456789.digested.css', $asset->publicPathWithoutDigest); } - public function testCreateMappedAssetWithContentBasic() + public function testCreateMappedAssetWithContentThatChanged() { - $assetMapper = $this->createFactory(); - $expected = <<createFactory($file1Compiler); + $expected = 'totally changed'; $asset = $assetMapper->createMappedAsset('file1.css', __DIR__.'/../Fixtures/dir1/file1.css'); $this->assertSame($expected, $asset->content); @@ -65,6 +73,14 @@ public function testCreateMappedAssetWithContentBasic() $this->assertSame($expected, $asset->content); } + public function testCreateMappedAssetWithContentThatDoesNotChange() + { + $assetMapper = $this->createFactory(); + $asset = $assetMapper->createMappedAsset('file1.css', __DIR__.'/../Fixtures/dir1/file1.css'); + // null content because the final content matches the file source + $this->assertNull($asset->content); + } + public function testCreateMappedAssetWithContentErrorsOnCircularReferences() { $factory = $this->createFactory();