Skip to content

[AssetMapper] Fixing memory bug where we stored way more file content than needed #52330

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 27, 2023
Merged

[AssetMapper] Fixing memory bug where we stored way more file content than needed #52330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/Symfony/Component/AssetMapper/Command/AssetMapperCompileCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <info>%s</info>', $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);
Expand Down
26 changes: 11 additions & 15 deletions src/Symfony/Component/AssetMapper/Factory/MappedAssetFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ class MappedAssetFactory implements MappedAssetFactoryInterface

private array $assetsCache = [];
private array $assetsBeingCreated = [];
private array $fileContentsCache = [];

public function __construct(
private readonly PublicAssetsPathResolverInterface $assetsPathResolver,
Expand All @@ -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,
Expand All @@ -75,15 +75,15 @@ 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)) {
return [$matches[1], true];
}

// Use the compiled content if any
if (null !== $content = $this->compileContent($asset)) {
if (null !== $content) {
return [hash('xxh128', $content), false];
}

Expand All @@ -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);
Expand Down
4 changes: 3 additions & 1 deletion src/Symfony/Component/AssetMapper/MappedAsset.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
7 changes: 5 additions & 2 deletions src/Symfony/Component/AssetMapper/Tests/AssetMapperDevServerSubscriberFunctionalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -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(<<<EOF
/* file1.css */
body {}

EOF, $response->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'));
Expand Down Expand Up @@ -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(<<<EOF
/* already-abcdefVWXYZ0123456789.digested.css */
body {}

EOF, $response->getContent());
EOF, $response->getFile()->getContent());
}

protected static function getKernelClass(): string
Expand Down
28 changes: 22 additions & 6 deletions src/Symfony/Component/AssetMapper/Tests/Factory/MappedAssetFactoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = <<<EOF
/* file1.css */
body {}
$file1Compiler = new class() implements AssetCompilerInterface {
public function supports(MappedAsset $asset): bool
{
return true;
}

EOF;
public function compile(string $content, MappedAsset $asset, AssetMapperInterface $assetMapper): string
{
return 'totally changed';
}
};

$assetMapper = $this->createFactory($file1Compiler);
$expected = 'totally changed';

$asset = $assetMapper->createMappedAsset('file1.css', __DIR__.'/../Fixtures/dir1/file1.css');
$this->assertSame($expected, $asset->content);
Expand All @@ -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();
Expand Down