[AssetMapper] Fixing bug where JSCompiler used non-absolute importmap entry path

weaverryan · nicolas-grekas · commit ef3397fa388c · 2023-10-31T08:55:25.000+01:00
diff --git a/Compiler/JavaScriptImportPathCompiler.php b/Compiler/JavaScriptImportPathCompiler.php
@@ -153,7 +153,7 @@ private function findAssetForBareImport(string $importedModule, AssetMapperInter
                 return $asset;
             }
 
-            return $assetMapper->getAssetFromSourcePath($importMapEntry->path);
+            return $assetMapper->getAssetFromSourcePath($this->importMapConfigReader->convertPathToFilesystemPath($importMapEntry->path));
         } catch (CircularAssetsException $exception) {
             return $exception->getIncompleteMappedAsset();
         }
diff --git a/ImportMap/ImportMapConfigReader.php b/ImportMap/ImportMapConfigReader.php
@@ -12,6 +12,7 @@
 namespace Symfony\Component\AssetMapper\ImportMap;
 
 use Symfony\Component\AssetMapper\Exception\RuntimeException;
+use Symfony\Component\Filesystem\Path;
 use Symfony\Component\VarExporter\VarExporter;
 
 /**
@@ -144,7 +145,40 @@ public function createRemoteEntry(string $importName, ImportMapType $type, strin
         return ImportMapEntry::createRemote($importName, $type, $path, $version, $packageModuleSpecifier, $isEntrypoint);
     }
 
-    public function getRootDirectory(): string
+    /**
+     * Converts the "path" string from an importmap entry to the filesystem path.
+     *
+     * The path may already be a filesystem path. But if it starts with ".",
+     * then the path is relative and the root directory is prepended.
+     */
+    public function convertPathToFilesystemPath(string $path): string
+    {
+        if (!str_starts_with($path, '.')) {
+            return $path;
+        }
+
+        return Path::join($this->getRootDirectory(), $path);
+    }
+
+    /**
+     * Converts a filesystem path to a relative path that can be used in the importmap.
+     *
+     * If no relative path could be created - e.g. because the path is not in
+     * the same directory/subdirectory as the root importmap.php file - null is returned.
+     */
+    public function convertFilesystemPathToPath(string $filesystemPath): ?string
+    {
+        $rootImportMapDir = realpath($this->getRootDirectory());
+        $filesystemPath = realpath($filesystemPath);
+        if (!str_starts_with($filesystemPath, $rootImportMapDir)) {
+            return null;
+        }
+
+        // remove the root directory, prepend "./" & normalize slashes
+        return './'.str_replace('\\', '/', substr($filesystemPath, \strlen($rootImportMapDir) + 1));
+    }
+
+    private function getRootDirectory(): string
     {
         return \dirname($this->importMapConfigPath);
     }
diff --git a/ImportMap/ImportMapGenerator.php b/ImportMap/ImportMapGenerator.php
@@ -209,11 +209,7 @@ private function findAsset(string $path): ?MappedAsset
             return $asset;
         }
 
-        if (str_starts_with($path, '.')) {
-            $path = $this->importMapConfigReader->getRootDirectory().'/'.$path;
-        }
-
-        return $this->assetMapper->getAssetFromSourcePath($path);
+        return $this->assetMapper->getAssetFromSourcePath($this->importMapConfigReader->convertPathToFilesystemPath($path));
     }
 
     private function findEagerImports(MappedAsset $asset): array
diff --git a/ImportMap/ImportMapManager.php b/ImportMap/ImportMapManager.php
@@ -152,11 +152,10 @@ private function requirePackages(array $packagesToRequire, ImportMapEntries $imp
                 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));
             }
 
-            $rootImportMapDir = $this->importMapConfigReader->getRootDirectory();
             // convert to a relative path (or fallback to the logical path)
             $path = $asset->logicalPath;
-            if ($rootImportMapDir && str_starts_with(realpath($asset->sourcePath), realpath($rootImportMapDir))) {
-                $path = './'.substr(realpath($asset->sourcePath), \strlen(realpath($rootImportMapDir)) + 1);
+            if (null !== $relativePath = $this->importMapConfigReader->convertFilesystemPathToPath($asset->sourcePath)) {
+                $path = $relativePath;
             }
 
             $newEntry = ImportMapEntry::createLocal(
@@ -213,10 +212,6 @@ private function findAsset(string $path): ?MappedAsset
             return $asset;
         }
 
-        if (str_starts_with($path, '.')) {
-            $path = $this->importMapConfigReader->getRootDirectory().'/'.$path;
-        }
-
-        return $this->assetMapper->getAssetFromSourcePath($path);
+        return $this->assetMapper->getAssetFromSourcePath($this->importMapConfigReader->convertPathToFilesystemPath($path));
     }
 }
diff --git a/Tests/Compiler/JavaScriptImportPathCompilerTest.php b/Tests/Compiler/JavaScriptImportPathCompilerTest.php
@@ -38,11 +38,20 @@ public function testCompileFindsCorrectImports(string $input, array $expectedJav
             ->willReturnCallback(function ($importName) {
                 return match ($importName) {
                     'module_in_importmap_local_asset' => ImportMapEntry::createLocal('module_in_importmap_local_asset', ImportMapType::JS, 'module_in_importmap_local_asset.js', false),
-                    '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),
-                    '@popperjs/core' => ImportMapEntry::createRemote('@popperjs/core', ImportMapType::JS, '/path/to/vendor/@popperjs/core.js', '1.2.3', 'could_be_anything', false),
+                    '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),
+                    '@popperjs/core' => ImportMapEntry::createRemote('@popperjs/core', ImportMapType::JS, '/project/assets/vendor/@popperjs/core.js', '1.2.3', 'could_be_anything', false),
                     default => null,
                 };
             });
+        $importMapConfigReader->expects($this->any())
+            ->method('convertPathToFilesystemPath')
+            ->willReturnCallback(function ($path) {
+                return match ($path) {
+                    './vendor/module_in_importmap_remote.js' => '/project/assets/vendor/module_in_importmap_remote.js',
+                    '/project/assets/vendor/@popperjs/core.js' => '/project/assets/vendor/@popperjs/core.js',
+                    default => throw new \RuntimeException(sprintf('Unexpected path "%s"', $path)),
+                };
+            });
 
         $assetMapper = $this->createMock(AssetMapperInterface::class);
         $assetMapper->expects($this->any())
@@ -61,8 +70,8 @@ public function testCompileFindsCorrectImports(string $input, array $expectedJav
                     '/project/assets/other.js' => new MappedAsset('other.js', publicPathWithoutDigest: '/assets/other.js'),
                     '/project/assets/subdir/foo.js' => new MappedAsset('subdir/foo.js', publicPathWithoutDigest: '/assets/subdir/foo.js'),
                     '/project/assets/styles.css' => new MappedAsset('styles.css', publicPathWithoutDigest: '/assets/styles.css'),
-                    '/path/to/vendor/module_in_importmap_remote.js' => new MappedAsset('module_in_importmap_remote.js', publicPathWithoutDigest: '/assets/module_in_importmap_remote.js'),
-                    '/path/to/vendor/@popperjs/core.js' => new MappedAsset('assets/vendor/@popperjs/core.js', publicPathWithoutDigest: '/assets/@popperjs/core.js'),
+                    '/project/assets/vendor/module_in_importmap_remote.js' => new MappedAsset('module_in_importmap_remote.js', publicPathWithoutDigest: '/assets/module_in_importmap_remote.js'),
+                    '/project/assets/vendor/@popperjs/core.js' => new MappedAsset('assets/vendor/@popperjs/core.js', publicPathWithoutDigest: '/assets/@popperjs/core.js'),
                     default => null,
                 };
             });
@@ -439,7 +448,12 @@ public function testCompileHandlesCircularBareImportAssets()
         $importMapConfigReader->expects($this->once())
             ->method('findRootImportMapEntry')
             ->with('@popperjs/core')
-            ->willReturn(ImportMapEntry::createRemote('@popperjs/core', ImportMapType::JS, '/path/to/vendor/@popperjs/core.js', '1.2.3', 'could_be_anything', false));
+            ->willReturn(ImportMapEntry::createRemote('@popperjs/core', ImportMapType::JS, './vendor/@popperjs/core.js', '1.2.3', 'could_be_anything', false));
+        $importMapConfigReader->expects($this->any())
+            ->method('convertPathToFilesystemPath')
+            ->with('./vendor/@popperjs/core.js')
+            ->willReturn('/path/to/vendor/@popperjs/core.js');
+
         $assetMapper = $this->createMock(AssetMapperInterface::class);
         $assetMapper->expects($this->once())
             ->method('getAssetFromSourcePath')
diff --git a/Tests/ImportMap/ImportMapConfigReaderTest.php b/Tests/ImportMap/ImportMapConfigReaderTest.php
@@ -109,10 +109,50 @@ public function testGetEntriesAndWriteEntries()
         $this->assertSame($originalImportMapData, $newImportMapData);
     }
 
-    public function testGetRootDirectory()
+    /**
+     * @dataProvider getPathToFilesystemPathTests
+     */
+    public function testConvertPathToFilesystemPath(string $path, string $expectedPath)
+    {
+        $configReader = new ImportMapConfigReader(realpath(__DIR__.'/../Fixtures/importmap.php'), $this->createMock(RemotePackageStorage::class));
+        // normalize path separators for comparison
+        $expectedPath = str_replace('\\', '/', $expectedPath);
+        $this->assertSame($expectedPath, $configReader->convertPathToFilesystemPath($path));
+    }
+
+    public static function getPathToFilesystemPathTests()
+    {
+        yield 'no change' => [
+            'path' => 'dir1/file2.js',
+            'expectedPath' => 'dir1/file2.js',
+        ];
+
+        yield 'prefixed with relative period' => [
+            'path' => './dir1/file2.js',
+            'expectedPath' => realpath(__DIR__.'/../Fixtures').'/dir1/file2.js',
+        ];
+    }
+
+    /**
+     * @dataProvider getFilesystemPathToPathTests
+     */
+    public function testConvertFilesystemPathToPath(string $path, ?string $expectedPath)
     {
         $configReader = new ImportMapConfigReader(__DIR__.'/../Fixtures/importmap.php', $this->createMock(RemotePackageStorage::class));
-        $this->assertSame(__DIR__.'/../Fixtures', $configReader->getRootDirectory());
+        $this->assertSame($expectedPath, $configReader->convertFilesystemPathToPath($path));
+    }
+
+    public static function getFilesystemPathToPathTests()
+    {
+        yield 'not in root directory' => [
+            'path' => __FILE__,
+            'expectedPath' => null,
+        ];
+
+        yield 'converted to relative path' => [
+            'path' => __DIR__.'/../Fixtures/dir1/file2.js',
+            'expectedPath' => './dir1/file2.js',
+        ];
     }
 
     public function testFindRootImportMapEntry()
diff --git a/Tests/ImportMap/ImportMapGeneratorTest.php b/Tests/ImportMap/ImportMapGeneratorTest.php
@@ -23,6 +23,7 @@
 use Symfony\Component\AssetMapper\ImportMap\JavaScriptImport;
 use Symfony\Component\AssetMapper\MappedAsset;
 use Symfony\Component\Filesystem\Filesystem;
+use Symfony\Component\Filesystem\Path;
 
 class ImportMapGeneratorTest extends TestCase
 {
@@ -252,8 +253,14 @@ public function testGetRawImportMapData(array $importMapEntries, array $mappedAs
         $this->mockImportMap($importMapEntries);
         $this->mockAssetMapper($mappedAssets);
         $this->configReader->expects($this->any())
-            ->method('getRootDirectory')
-            ->willReturn('/fake/root');
+            ->method('convertPathToFilesystemPath')
+            ->willReturnCallback(function (string $path) {
+                if (!str_starts_with($path, '.')) {
+                    return $path;
+                }
+
+                return Path::join('/fake/root', $path);
+            });
 
         $this->assertEquals($expectedData, $manager->getRawImportMapData());
     }
diff --git a/Tests/ImportMap/ImportMapManagerTest.php b/Tests/ImportMap/ImportMapManagerTest.php
@@ -75,8 +75,22 @@ public function testRequire(array $packages, int $expectedProviderPackageArgumen
         ;
 
         $this->configReader->expects($this->any())
-            ->method('getRootDirectory')
-            ->willReturn(self::$writableRoot);
+            ->method('convertPathToFilesystemPath')
+            ->willReturnCallback(function ($path) {
+                if (str_ends_with($path, 'some_file.js')) {
+                    return '/path/to/assets/some_file.js';
+                }
+
+                throw new \Exception(sprintf('Unexpected path "%s"', $path));
+            });
+        $this->configReader->expects($this->any())
+            ->method('convertFilesystemPathToPath')
+            ->willReturnCallback(function ($path) {
+                return match ($path) {
+                    '/path/to/assets/some_file.js' => './assets/some_file.js',
+                    default => throw new \Exception(sprintf('Unexpected path "%s"', $path)),
+                };
+            });
         $this->configReader->expects($this->once())
             ->method('getEntries')
             ->willReturn(new ImportMapEntries())
@@ -187,15 +201,15 @@ public static function getRequirePackageTests(): iterable
         ];
 
         yield 'single_package_with_a_path' => [
-            'packages' => [new PackageRequireOptions('some/module', path: self::$writableRoot.'/assets/some_file.js')],
-            'expectedProviderPackageArgumentCount' => 0,
-            'resolvedPackages' => [],
-            'expectedImportMap' => [
-                'some/module' => [
-                    // converted to relative path
-                    'path' => './assets/some_file.js',
-                ],
+        'packages' => [new PackageRequireOptions('some/module', path: self::$writableRoot.'/assets/some_file.js')],
+        'expectedProviderPackageArgumentCount' => 0,
+        'resolvedPackages' => [],
+        'expectedImportMap' => [
+            'some/module' => [
+                // converted to relative path
+                'path' => './assets/some_file.js',
             ],
+        ],
         ];
     }
 
@@ -289,10 +303,6 @@ public function testUpdateWithSpecificPackages()
 
         $this->remotePackageDownloader->expects($this->once())
             ->method('downloadPackages');
-
-        $this->configReader->expects($this->any())
-            ->method('getRootDirectory')
-            ->willReturn(self::$writableRoot);
         $this->configReader->expects($this->once())
             ->method('writeEntries')
             ->with($this->callback(function (ImportMapEntries $entries) {

Original file line number	Diff line number	Diff line change
`@@ -153,7 +153,7 @@ private function findAssetForBareImport(string $importedModule, AssetMapperInter`
`153`	`153`	`return $asset;`
`154`	`154`	`}`
`155`	`155`
`156`		`- return $assetMapper->getAssetFromSourcePath($importMapEntry->path);`
	`156`	`+ return $assetMapper->getAssetFromSourcePath($this->importMapConfigReader->convertPathToFilesystemPath($importMapEntry->path));`
`157`	`157`	`} catch (CircularAssetsException $exception) {`
`158`	`158`	`return $exception->getIncompleteMappedAsset();`
`159`	`159`	`}`
Original file line number	Diff line number	Diff line change
`@@ -209,11 +209,7 @@ private function findAsset(string $path): ?MappedAsset`
`209`	`209`	`return $asset;`
`210`	`210`	`}`
`211`	`211`
`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));`
`217`	`213`	`}`
`218`	`214`
`219`	`215`	`private function findEagerImports(MappedAsset $asset): array`
Original file line number	Diff line number	Diff line change
`@@ -152,11 +152,10 @@ private function requirePackages(array $packagesToRequire, ImportMapEntries $imp`
`152`	`152`	`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));`
`153`	`153`	`}`
`154`	`154`
`155`		`- $rootImportMapDir = $this->importMapConfigReader->getRootDirectory();`
`156`	`155`	`// convert to a relative path (or fallback to the logical path)`
`157`	`156`	`$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;`
`160`	`159`	`}`
`161`	`160`
`162`	`161`	`$newEntry = ImportMapEntry::createLocal(`
`@@ -213,10 +212,6 @@ private function findAsset(string $path): ?MappedAsset`
`213`	`212`	`return $asset;`
`214`	`213`	`}`
`215`	`214`
`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));`
`221`	`216`	`}`
`222`	`217`	`}`