8000 [AssetMapper] Fixing bug where JSCompiler used non-absolute importmap… · symfony/symfony@31d04ef · GitHub
[go: up one dir, main page]
Skip to content

Commit 31d04ef

Browse files
weaverryannicolas-grekas
weaverryan
authored and
nicolas-grekas
committed
[AssetMapper] Fixing bug where JSCompiler used non-absolute importmap entry path
1 parent 99d5cbb commit 31d04ef

File tree

8 files changed

+134
-38
lines changed

8 files changed

+134
-38
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ private function findAssetForBareImport(string $importedModule, AssetMapperInter
153153
return $asset;
154154
}
155155

156-
return $assetMapper->getAssetFromSourcePath($importMapEntry->path);
156+
return $assetMapper->getAssetFromSourcePath($this->importMapConfigReader->convertPathToFilesystemPath($importMapEntry->path));
157157
} catch (CircularAssetsException $exception) {
158158
return $exception->getIncompleteMappedAsset();
159159
}

‎src/Symfony/Component/AssetMapper/ImportMap/ImportMapConfigReader.php

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
namespace Symfony\Component\AssetMapper\ImportMap;
1313

1414
use Symfony\Component\AssetMapper\Exception\RuntimeException;
15+
use Symfony\Component\Filesystem\Path;
1516
use Symfony\Component\VarExporter\VarExporter;
1617

1718
/**
@@ -144,7 +145,40 @@ public function createRemoteEntry(string $importName, ImportMapType $type, strin
144145
return ImportMapEntry::createRemote($importName, $type, $path, $version, $packageModuleSpecifier, $isEntrypoint);
145146
}
146147

147-
public function getRootDirectory(): string
148+
/**
149+
* Converts the "path" string from an importmap entry to the filesystem path.
150+
*
151+
* The path may already be a filesystem path. But if it starts with ".",
152+
* then the path is relative and the root directory is prepended.
153+
*/
154+
public function convertPathToFilesystemPath(string $path): string
155+
{
156+
if (!str_starts_with($path, '.')) {
157+
return $path;
158+
}
159+
160+
return Path::join($this->getRootDirectory(), $path);
161+
}
162+
163+
/**
164+
* Converts a filesystem path to a relative path that can be used in the importmap.
165+
*
166+
* If no relative path could be created - e.g. because the path is not in
167+
* the same directory/subdirectory as the root importmap.php file - null is returned.
168+
*/
169+
public function convertFilesystemPathToPath(string $filesystemPath): ?string
170+
{
171+
$rootImportMapDir = realpath($this->getRootDirectory());
172+
$filesystemPath = realpath($filesystemPath);
173+
if (!str_starts_with($filesystemPath, $rootImportMapDir)) {
174+
return null;
175+
}
176+
177+
// remove the root directory, prepend "./" & normalize slashes
178+
return './'.str_replace('\\', '/', substr($filesystemPath, \strlen($rootImportMapDir) + 1));
179+
}
180+
181+
private function getRootDirectory(): string
148182
{
149183
return \dirname($this->importMapConfigPath);
150184
}

‎src/Symfony/Component/AssetMapper/ImportMap/ImportMapGenerator.php

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -209,11 +209,7 @@ private function findAsset(string $path): ?MappedAsset
209209
return $asset;
210210
}
211211

212-
if (str_starts_with($path, '.')) {
213-
$path = $this->importMapConfigReader->getRootDirectory().'/'.$path;
214-
}
215-
216-
return $this->assetMapper->getAssetFromSourcePath($path);
212+
return $this->assetMapper->getAssetFromSourcePath($this->importMapConfigReader->convertPathToFilesystemPath($path));
217213
}
218214

219215
private function findEagerImports(MappedAsset $asset): array

‎src/Symfony/Component/AssetMapper/ImportMap/ImportMapManager.php

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -152,11 +152,10 @@ private function requirePackages(array $packagesToRequire, ImportMapEntries $imp
152152
throw new \LogicException(sprintf('The path "%s" of the package "%s" cannot be found: either pass the logical name of the asset or a relative path starting with "./".', $requireOptions->path, $requireOptions->importName));
153153
}
154154

155-
$rootImportMapDir = $this->importMapConfigReader->getRootDirectory();
156155
// convert to a relative path (or fallback to the logical path)
157156
$path = $asset->logicalPath;
158-
if ($rootImportMapDir && str_starts_with(realpath($asset->sourcePath), realpath($rootImportMapDir))) {
159-
$path = './'.substr(realpath($asset->sourcePath), \strlen(realpath($rootImportMapDir)) + 1);
157+
if (null !== $relativePath = $this->importMapConfigReader->convertFilesystemPathToPath($asset->sourcePath)) {
158+
$path = $relativePath;
160159
}
161160

162161
$newEntry = ImportMapEntry::createLocal(
@@ -213,10 +212,6 @@ private function findAsset(string $path): ?MappedAsset
213212
return $asset;
214213
}
215214

216-
if (str_starts_with($path, '.')) {
217-
$path = $this->importMapConfigReader->getRootDirectory().'/'.$path;
218-
}
219-
220-
return $this->assetMapper->getAssetFromSourcePath($path);
215+
return $this->assetMapper->getAssetFromSourcePath($this->importMapConfigReader->convertPathToFilesystemPath($path));
221216
}
222217
}

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

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -38,11 +38,20 @@ public function testCompileFindsCorrectImports(string $input, array $expectedJav
3838
->willReturnCallback(function ($importName) {
3939
return match ($importName) {
4040
'module_in_importmap_local_asset' => ImportMapEntry::createLocal('module_in_importmap_local_asset', ImportMapType::JS, 'module_in_importmap_local_asset.js', false),
41-
'module_in_importmap_remote' => ImportMapEntry::createRemote('module_in_importmap_remote', ImportMapType::JS, '/path/to/vendor/module_in_importmap_remote.js', '1.2.3', 'could_be_anything', false),
42-
'@popperjs/core' => ImportMapEntry::createRemote('@popperjs/core', ImportMapType::JS, '/path/to/vendor/@popperjs/core.js', '1.2.3', 'could_be_anything', false),
41+
'module_in_importmap_remote' => ImportMapEntry::createRemote('module_in_importmap_remote', ImportMapType::JS, './vendor/module_in_importmap_remote.js', '1.2.3', 'could_be_anything', false),
42+
'@popperjs/core' => ImportMapEntry::createRemote('@popperjs/core', ImportMapType::JS, '/project/assets/vendor/@popperjs/core.js', '1.2.3', 'could_be_anything', false),
4343
default => null,
4444
};
4545
});
46+
$importMapConfigReader->expects($this->any())
47+
->method('convertPathToFilesystemPath')
48+
->willReturnCallback(function ($path) {
49+
return match ($path) {
50+
'./vendor/module_in_importmap_remote.js' => '/project/assets/vendor/module_in_importmap_remote.js',
51+
'/project/assets/vendor/@popperjs/core.js' => '/project/assets/vendor/@popperjs/core.js',
52+
default => throw new \RuntimeException(sprintf('Unexpected path "%s"', $path)),
53+
};
54+
});
4655

4756
$assetMapper = $this->createMock(AssetMapperInterface::class);
4857
$assetMapper->expects($this->any())
@@ -61,8 +70,8 @@ public function testCompileFindsCorrectImports(string $input, array $expectedJav
6170
'/project/assets/other.js' => new MappedAsset('other.js', publicPathWithoutDigest: '/assets/other.js'),
6271
'/project/assets/subdir/foo.js' => new MappedAsset('subdir/foo.js', publicPathWithoutDigest: '/assets/subdir/foo.js'),
6372
'/project/assets/styles.css' => new MappedAsset('styles.css', publicPathWithoutDigest: '/assets/styles.css'),
64-
'/path/to/vendor/module_in_importmap_remote.js' => new MappedAsset('module_in_importmap_remote.js', publicPathWithoutDigest: '/assets/module_in_importmap_remote.js'),
65-
'/path/to/vendor/@popperjs/core.js' => new MappedAsset('assets/vendor/@popperjs/core.js', publicPathWithoutDigest: '/assets/@popperjs/core.js'),
73+
'/project/assets/vendor/module_in_importmap_remote.js' => new MappedAsset('module_in_importmap_remote.js', publicPathWithoutDigest: '/assets/module_in_importmap_remote.js'),
74+
'/project/assets/vendor/@popperjs/core.js' => new MappedAsset('assets/vendor/@popperjs/core.js', publicPathWithoutDigest: '/assets/@popperjs/core.js'),
6675
default => null,
6776
};
6877
});
@@ -439,7 +448,12 @@ public function testCompileHandlesCircularBareImportAssets()
439448
$importMapConfigReader->expects($this->once())
440449
->method('findRootImportMapEntry')
441450
->with('@popperjs/core')
442-
->willReturn(ImportMapEntry::createRemote('@popperjs/core', ImportMapType::JS, '/path/to/vendor/@popperjs/core.js', '1.2.3', 'could_be_anything', false));
451+
->willReturn(ImportMapEntry::createRemote('@popperjs/core', ImportMapType::JS, './vendor/@popperjs/core.js', '1.2.3', 'could_be_anything', false));
452+
$importMapConfigReader->expects($this->any())
453+
->method('convertPathToFilesystemPath')
454+
->with('./vendor/@popperjs/core.js')
455+
->willReturn('/path/to/vendor/@popperjs/core.js');
456+
443457
$assetMapper = $this->createMock(AssetMapperInterface::class);
444458
$assetMapper->expects($this->once())
445459
->method('getAssetFromSourcePath')

‎src/Symfony/Component/AssetMapper/Tests/ImportMap/ImportMapConfigReaderTest.php

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -109,10 +109,50 @@ public function testGetEntriesAndWriteEntries()
109109
$this->assertSame($originalImportMapData, $newImportMapData);
110110
}
111111

112-
public function testGetRootDirectory()
112+
/**
113+
* @dataProvider getPathToFilesystemPathTests
114+
*/
115+
public function testConvertPathToFilesystemPath(string $path, string $expectedPath)
116+
{
117+
$configReader = new ImportMapConfigReader(realpath(__DIR__.'/../Fixtures/importmap.php'), $this->createMock(RemotePackageStorage::class));
118+
// normalize path separators for comparison
119+
$expectedPath = str_replace('\\', '/', $expectedPath);
120+
$this->assertSame($expectedPath, $configReader->convertPathToFilesystemPath($path));
121+
}
122+
123+
public static function getPathToFilesystemPathTests()
124+
{
125+
yield 'no change' => [
126+
'path' => 'dir1/file2.js',
127+
'expectedPath' => 'dir1/file2.js',
128+
];
129+
130+
yield 'prefixed with relative period' => [
131+
'path' => './dir1/file2.js',
132+
'expectedPath' => realpath(__DIR__.'/../Fixtures').'/dir1/file2.js',
133+
];
134+
}
135+
136+
/**
137+
* @dataProvider getFilesystemPathToPathTests
138+
*/
139+
public function testConvertFilesystemPathToPath(string $path, ?string $expectedPath)
113140
{
114141
$configReader = new ImportMapConfigReader(__DIR__.'/../Fixtures/importmap.php', $this->createMock(RemotePackageStorage::class));
115-
$this->assertSame(__DIR__.'/../Fixtures', $configReader->getRootDirectory());
142+
$this->assertSame($expectedPath, $configReader->convertFilesystemPathToPath($path));
143+
}
144+
145+
public static function getFilesystemPathToPathTests()
146+
{
147+
yield 'not in root directory' => [
148+
'path' => __FILE__,
149+
'expectedPath' => null,
150+
];
151+
152+
yield 'converted to relative path' => [
153+
'path' => __DIR__.'/../Fixtures/dir1/file2.js',
154+
'expectedPath' => './dir1/file2.js',
155+
];
116156
}
117157

118158
public function testFindRootImportMapEntry()

‎src/Symfony/Component/AssetMapper/Tests/ImportMap/ImportMapGeneratorTest.php

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
use Symfony\Component\AssetMapper\ImportMap\JavaScriptImport;
2424
use Symfony\Component\AssetMapper\MappedAsset;
2525
use Symfony\Component\Filesystem\Filesystem;
26+
use Symfony\Component\Filesystem\Path;
2627

2728
class ImportMapGeneratorTest extends TestCase
2829
{
@@ -252,8 +253,14 @@ public function testGetRawImportMapData(array $importMapEntries, array $mappedAs
252253
$this->mockImportMap($importMapEntries);
253254
$this->mockAssetMapper($mappedAssets);
254255
$this->configReader->expects($this->any())
255-
->method('getRootDirectory')
256-
->willReturn('/fake/root');
256+
->method('convertPathToFilesystemPath')
257+
->willReturnCallback(function (string $path) {
258+
if (!str_starts_with($path, '.')) {
259+
return $path;
260+
}
261+
262+
return Path::join('/fake/root', $path);
263+
});
257264

258265
$this->assertEquals($expectedData, $manager->getRawImportMapData());
259266
}

‎src/Symfony/Component/AssetMapper/Tests/ImportMap/ImportMapManagerTest.php

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -75,8 +75,22 @@ public function testRequire(array $packages, int $expectedProviderPackageArgumen
7575
;
7676

7777
$this->configReader->expects($this->any())
78-
->method('getRootDirectory')
79-
->willReturn(self::$writableRoot);
78+
->method('convertPathToFilesystemPath')
79+
->willReturnCallback(function ($path) {
80+
if (str_ends_with($path, 'some_file.js')) {
81+
return '/path/to/assets/some_file.js';
82+
}
83+
84+
throw new \Exception(sprintf('Unexpected path "%s"', $path));
85+
});
86+
$this->configReader->expects($this->any())
87+
->method('convertFilesystemPathToPath')
88+
->willReturnCallback(function ($path) {
89+
return match ($path) {
90+
'/path/to/assets/some_file.js' => './assets/some_file.js',
91+
default => throw new \Exception(sprintf('Unexpected path "%s"', $path)),
92+
};
93+
});
8094
$this->configReader->expects($this->once())
8195
->method('getEntries')
8296
->willReturn(new ImportMapEntries())
@@ -187,15 +201,15 @@ public static function getRequirePackageTests(): iterable
187201
];
188202

189203
yield 'single_package_with_a_path' => [
190-
'packages' => [new PackageRequireOptions('some/module', path: self::$writableRoot.'/assets/some_file.js')],
191-
'expectedProviderPackageArgumentCount' => 0,
192-
'resolvedPackages' => [],
193-
'expectedImportMap' => [
194-
'some/module' => [
195-
// converted to relative path
196-
'path' => './assets/some_file.js',
197-
],
204+
'packages' => [new PackageRequireOptions('some/module', path: self::$writableRoot.'/assets/some_file.js')],
205+
'expectedProviderPackageArgumentCount' => 0,
206+
'resolvedPackages' => [],
207+
'expectedImportMap' => [
208+
'some/module' => [
209+
// converted to relative path
210+
'path' => './assets/some_file.js',
198211
],
212+
],
199213
];
200214
}
201215

@@ -289,10 +303,6 @@ public function testUpdateWithSpecificPackages()
289303

290304
$this->remotePackageDownloader->expects($this->once())
291305
->method('downloadPackages');
292-
293-
$this->configReader->expects($this->any())
294-
->method('getRootDirectory')
295-
->willReturn(self::$writableRoot);
296306
$this->configReader->expects($this->once())
297307
->method('writeEntries')
298308
->with($this->callback(function (ImportMapEntries $entries) {

0 commit comments

Comments
 (0)
0