Skip to content

Commit 2ccdfbc

Browse files
fabpotfabpot
committed
bug symfony#52323 [AssetMapper] Allowing circular references in JavaScriptImportPathCompiler (weaverryan)
This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [AssetMapper] Allowing circular references in JavaScriptImportPathCompiler | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | None | License | MIT Hi! In AssetMapper, we throw a `CircularAssetsException` if you ask for the `app.js` asset, but it's still being created. This circular situation only happens via asset compilers: if, while compiling `app.js`, we ask for `other.js`... and then while compiling that, we ask for `app.js`. However, JS modules *do* allow for circular references - e.g. `app.js` imports `other.js` which imports `app.js`. Before this PR, this situation would trigger the `CircularAssetsException`. After, it is handled correctly. `JavaScriptImportPathCompiler` calls `$assetMapper->getAsset()` only to populate the `MappedAsset.javaScriptImports` property. So, allowing for the half-created `MappedAsset` is safe here: we do not use any of the not-yet-populated properties. Cheers! Commits ------- b0953a4 [AssetMapper] Allowing circular references in JavaScriptImportPathCompiler
2 parents 6fad9a4 + b0953a4 commit 2ccdfbc

File tree

5 files changed

+73
-10
lines changed

5 files changed

+73
-10
lines changed

‎src/Symfony/Component/AssetMapper/Compiler/JavaScriptImportPathCompiler.php

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -148,11 +148,15 @@ private function findAssetForBareImport(string $importedModule, AssetMapperInter
148148
return null;
149149
}
150150

151-
if ($asset = $assetMapper->getAsset($importMapEntry->path)) {
152-
return $asset;
153-
}
151+
try {
152+
if ($asset = $assetMapper->getAsset($importMapEntry->path)) {
153+
return $asset;
154+
}
154155

155-
return $assetMapper->getAssetFromSourcePath($importMapEntry->path);
156+
return $assetMapper->getAssetFromSourcePath($importMapEntry->path);
157+
} catch (CircularAssetsException $exception) {
158+
return $exception->getIncompleteMappedAsset();
159+
}
156160
}
157161

158162
private function findAssetForRelativeImport(string $importedModule, MappedAsset $asset, AssetMapperInterface $assetMapper): ?MappedAsset
@@ -168,8 +172,7 @@ private function findAssetForRelativeImport(string $importedModule, MappedAsset
168172
return null;
169173
}
170174

171-
$dependentAsset = $assetMapper->getAssetFromSourcePath($resolvedSourcePath);
172-
175+
$dependentAsset = $assetMapper->getAsset($resolvedSourcePath);
173176
if ($dependentAsset) {
174177
return $dependentAsset;
175178
}

‎src/Symfony/Component/AssetMapper/Exception/CircularAssetsException.php

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,26 @@
1111

1212
namespace Symfony\Component\AssetMapper\Exception;
1313

14+
use Symfony\Component\AssetMapper\MappedAsset;
15+
1416
/**
1517
* Thrown when a circular reference is detected while creating an asset.
1618
*/
1719
class CircularAssetsException extends RuntimeException
1820
{
21+
public function __construct(private MappedAsset $mappedAsset, string $message = '', int $code = 0, \Throwable $previous = null)
22+
{
23+
parent::__construct($message, $code, $previous);
24+
}
25+
26+
/**
27+
* Returns the asset that was being created when the circular reference was detected.
28+
*
29+
* This asset will not be fully initialized: it will be missing some
30+
* properties like digest and content.
31+
*/
32+
public function getIncompleteMappedAsset(): MappedAsset
33+
{
34+
return $this->mappedAsset;
35+
}
1936
}

‎src/Symfony/Component/AssetMapper/Factory/MappedAssetFactory.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,13 +37,14 @@ public function __construct(
3737
public function createMappedAsset(string $logicalPath, string $sourcePath): ?MappedAsset
3838
{
3939
if (isset($this->assetsBeingCreated[$logicalPath])) {
40-
throw new CircularAssetsException(sprintf('Circular reference detected while creating asset for "%s": "%s".', $logicalPath, implode(' -> ', $this->assetsBeingCreated).' -> '.$logicalPath));
40+
throw new CircularAssetsException($this->assetsCache[$logicalPath], sprintf('Circular reference detected while creating asset for "%s": "%s".', $logicalPath, implode(' -> ', $this->assetsBeingCreated).' -> '.$logicalPath));
4141
}
4242
$this->assetsBeingCreated[$logicalPath] = $logicalPath;
4343

4444
if (!isset($this->assetsCache[$logicalPath])) {
4545
$isVendor = $this->isVendor($sourcePath);
4646
$asset = new MappedAsset($logicalPath, $sourcePath, $this->assetsPathResolver->resolvePublicPath($logicalPath), isVendor: $isVendor);
47+
$this->assetsCache[$logicalPath] = $asset;
4748

4849
$content = $this->compileContent($asset);
4950
[$digest, $isPredigested] = $this->getDigest($asset, $content);

‎src/Symfony/Component/AssetMapper/Tests/Compiler/JavaScriptImportPathCompilerTest.php

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -417,6 +417,48 @@ public static function providePathsCanUpdateTests(): iterable
417417
];
418418
}
419419

420+
public function testCompileHandlesCircularRelativeAssets()
421+
{
422+
$appAsset = new MappedAsset('app.js', 'anythingapp', '/assets/app.js');
423+
$otherAsset = new MappedAsset('other.js', 'anythingother', '/assets/other.js');
424+
425+
$importMapConfigReader = $this->createMock(ImportMapConfigReader::class);
426+
$assetMapper = $this->createMock(AssetMapperInterface::class);
427+
$assetMapper->expects($this->once())
428+
->method('getAsset')
429+
->with('other.js')
430+
->willThrowException(new CircularAssetsException($otherAsset));
431+
432+
$compiler = new JavaScriptImportPathCompiler($importMapConfigReader);
433+
$input = 'import "./other.js";';
434+
$compiler->compile($input, $appAsset, $assetMapper);
435+
$this->assertCount(1, $appAsset->getJavaScriptImports());
436+
$this->assertSame($otherAsset, $appAsset->getJavaScriptImports()[0]->asset);
437+
}
438+
439+
public function testCompileHandlesCircularBareImportAssets()
440+
{
441+
$bootstrapAsset = new MappedAsset('bootstrap', 'anythingbootstrap', '/assets/bootstrap.js');
442+
$popperAsset = new MappedAsset('@popperjs/core', 'anythingpopper', '/assets/popper.js');
443+
444+
$importMapConfigReader = $this->createMock(ImportMapConfigReader::class);
445+
$importMapConfigReader->expects($this->once())
446+
->method('findRootImportMapEntry')
447+
->with('@popperjs/core')
448+
->willReturn(ImportMapEntry::createRemote('@popperjs/core', ImportMapType::JS, '/path/to/vendor/@popperjs/core.js', '1.2.3', 'could_be_anything', false));
449+
$assetMapper = $this->createMock(AssetMapperInterface::class);
450+
$assetMapper->expects($this->once())
451+
->method('getAssetFromSourcePath')
452+
->with('/path/to/vendor/@popperjs/core.js')
453+
->willThrowException(new CircularAssetsException($popperAsset));
454+
455+
$compiler = new JavaScriptImportPathCompiler($importMapConfigReader);
456+
$input = 'import "@popperjs/core";';
457+
$compiler->compile($input, $bootstrapAsset, $assetMapper);
458+
$this->assertCount(1, $bootstrapAsset->getJavaScriptImports());
459+
$this->assertSame($popperAsset, $bootstrapAsset->getJavaScriptImports()[0]->asset);
460+
}
461+
420462
/**
421463
* @dataProvider provideMissingImportModeTests
422464
*/
@@ -487,7 +529,7 @@ public function testErrorMessageAvoidsCircularException()
487529
}
488530

489531
if ('htmx.js' === $logicalPath) {
490-
throw new CircularAssetsException();
532+
throw new CircularAssetsException(new MappedAsset('htmx.js'));
491533
}
492534
});
493535

‎src/Symfony/Component/AssetMapper/Tests/Factory/MappedAssetFactoryTest.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
use Symfony\Component\AssetMapper\Compiler\AssetCompilerInterface;
1919
use Symfony\Component\AssetMapper\Compiler\CssAssetUrlCompiler;
2020
use Symfony\Component\AssetMapper\Compiler\JavaScriptImportPathCompiler;
21-
use Symfony\Component\AssetMapper\Exception\RuntimeException;
21+
use Symfony\Component\AssetMapper\Exception\CircularAssetsException;
2222
use Symfony\Component\AssetMapper\Factory\MappedAssetFactory;
2323
use Symfony\Component\AssetMapper\ImportMap\ImportMapConfigReader;
2424
use Symfony\Component\AssetMapper\MappedAsset;
@@ -85,7 +85,7 @@ public function testCreateMappedAssetWithContentErrorsOnCircularReferences()
8585
{
8686
$factory = $this->createFactory();
8787

88-
$this->expectException(RuntimeException::class);
88+
$this->expectException(CircularAssetsException::class);
8989
$this->expectExceptionMessage('Circular reference detected while creating asset for "circular1.css": "circular1.css -> circular2.css -> circular1.css".');
9090
$factory->createMappedAsset('circular1.css', __DIR__.'/../Fixtures/circular_dir/circular1.css');
9191
}

0 commit comments

Comments
 (0)