Merge branch '1.x' into 2.x

nicolas-grekas · nicolas-grekas · commit e0d37f94873d · 2022-01-27T10:45:14.000+01:00
* 1.x:
  Fix tests
  Refactor PackageJsonSynchronizer to prevent unintentional duplicate dependencies
  Skipping missing source directories during copy-from-package unconfiguration
  Fixing missing blank line at end of modified patch files
diff --git a/phpunit.xml.dist b/phpunit.xml.dist
@@ -19,6 +19,7 @@
   <php>
     <ini name="error_reporting" value="-1" />
     <env name="SYMFONY_DEPRECATIONS_HELPER" value="weak" />
+    <env name="LC_ALL" value="C" />
   </php>
 
   <filter>
diff --git a/src/Configurator/CopyFromPackageConfigurator.php b/src/Configurator/CopyFromPackageConfigurator.php
@@ -145,6 +145,9 @@ public function copyFile(string $source, string $target, array $options)
 
     private function removeFilesFromDir(string $source, string $target)
     {
+        if (!is_dir($source)) {
+            return;
+        }
         $iterator = $this->createSourceIterator($source, \RecursiveIteratorIterator::CHILD_FIRST);
         foreach ($iterator as $item) {
             $targetPath = $this->path->concatenate([$target, $iterator->getSubPathName()]);
diff --git a/src/PackageJsonSynchronizer.php b/src/PackageJsonSynchronizer.php
@@ -13,9 +13,6 @@
 
 use Composer\Json\JsonFile;
 use Composer\Json\JsonManipulator;
-use Composer\Semver\Constraint\ConstraintInterface;
-use Composer\Semver\Intervals;
-use Composer\Semver\VersionParser;
 use Seld\JsonLint\ParsingException;
 
 /**
@@ -40,14 +37,17 @@ public function shouldSynchronize(): bool
 
     public function synchronize(array $phpPackages): bool
     {
-        // Remove all links and add again only the existing packages
         try {
-            $didAddLink = $this->removePackageJsonLinks((new JsonFile($this->rootDir.'/package.json'))->read());
+            JsonFile::parseJson(file_get_contents($this->rootDir.'/package.json'));
         } catch (ParsingException $e) {
             // if package.json is invalid (possible during a recipe upgrade), we can't update the file
             return false;
         }
 
+        $didChangePackageJson = $this->removeObsoletePackageJsonLinks();
+
+        $dependencies = [];
+
         foreach ($phpPackages as $k => $phpPackage) {
             if (\is_string($phpPackage)) {
                 // support for smooth upgrades from older flex versions
@@ -56,22 +56,29 @@ public function synchronize(array $phpPackages): bool
                     'keywords' => ['symfony-ux'],
                 ];
             }
-            $didAddLink = $this->addPackageJsonLink($phpPackage) || $didAddLink;
+
+            foreach ($this->resolvePackageDependencies($phpPackage) as $dependency => $constraint) {
+                $dependencies[$dependency][$phpPackage['name']] = $constraint;
+            }
         }
 
-        $this->registerPeerDependencies($phpPackages);
+        $didChangePackageJson = $this->registerDependencies($dependencies) || $didChangePackageJson;
 
         // Register controllers and entrypoints in controllers.json
         $this->registerWebpackResources($phpPackages);
 
-        return $didAddLink;
+        return $didChangePackageJson;
     }
 
-    private function removePackageJsonLinks(array $packageJson): bool
+    private function removeObsoletePackageJsonLinks(): bool
     {
-        $didRemoveLink = false;
-        $jsDependencies = $packageJson['dependencies'] ?? [];
-        $jsDevDependencies = $packageJson['devDependencies'] ?? [];
+        $didChangePackageJson = false;
+
+        $manipulator = new JsonManipulator(file_get_contents($this->rootDir.'/package.json'));
+        $content = json_decode($manipulator->getContents(), true);
+
+        $jsDependencies = $content['dependencies'] ?? [];
+        $jsDevDependencies = $content['devDependencies'] ?? [];
 
         foreach (['dependencies' => $jsDependencies, 'devDependencies' => $jsDevDependencies] as $key => $packages) {
             foreach ($packages as $name => $version) {
@@ -82,39 +89,78 @@ private function removePackageJsonLinks(array $packageJson): bool
                     continue;
                 }
 
-                $manipulator = new JsonManipulator(file_get_contents($this->rootDir.'/package.json'));
-                $manipulator->removeSubNode('devDependencies', $name);
-                file_put_contents($this->rootDir.'/package.json', $manipulator->getContents());
-                $didRemoveLink = true;
+                $manipulator->removeSubNode($key, $name);
+                $didChangePackageJson = true;
             }
         }
 
-        return $didRemoveLink;
+        file_put_contents($this->rootDir.'/package.json', $manipulator->getContents());
+
+        return $didChangePackageJson;
     }
 
-    private function addPackageJsonLink(array $phpPackage): bool
+    private function resolvePackageDependencies($phpPackage): array
     {
+        $dependencies = [];
+
         if (!$packageJson = $this->resolvePackageJson($phpPackage)) {
-            return false;
+            return $dependencies;
         }
 
-        $manipulator = new JsonManipulator(file_get_contents($this->rootDir.'/package.json'));
-        $manipulator->addSubNode('devDependencies', '@'.$phpPackage['name'], 'file:'.substr($packageJson->getPath(), 1 + \strlen($this->rootDir), -13));
+        $dependencies['@'.$phpPackage['name']] = 'file:'.substr($packageJson->getPath(), 1 + \strlen($this->rootDir), -13);
+
+        foreach ($packageJson->read()['peerDependencies'] ?? [] as $peerDependency => $constraint) {
+            $dependencies[$peerDependency] = $constraint;
+        }
+
+        return $dependencies;
+    }
+
+    private function registerDependencies(array $flexDependencies): bool
+    {
+        $didChangePackageJson = false;
 
+        $manipulator = new JsonManipulator(file_get_contents($this->rootDir.'/package.json'));
         $content = json_decode($manipulator->getContents(), true);
 
-        $devDependencies = $content['devDependencies'];
-        uksort($devDependencies, 'strnatcmp');
-        $manipulator->addMainKey('devDependencies', $devDependencies);
+        foreach ($flexDependencies as $dependency => $constraints) {
+            if (1 !== \count($constraints) && 1 !== \count(array_count_values($constraints))) {
+                // If the flex packages have a colliding peer dependency, leave the resolution to the user
+                continue;
+            }
 
-        $newContents = $manipulator->getContents();
-        if ($newContents === file_get_contents($this->rootDir.'/package.json')) {
-            return false;
+            $constraint = array_shift($constraints);
+
+            $parentNode = isset($content['dependencies'][$dependency]) ? 'dependencies' : 'devDependencies';
+            if (!isset($content[$parentNode][$dependency])) {
+                $content['devDependencies'][$dependency] = $constraint;
+                $didChangePackageJson = true;
+            } elseif ($constraint !== $content[$parentNode][$dependency]) {
+                $content[$parentNode][$dependency] = $constraint;
+                $didChangePackageJson = true;
+            }
         }
 
-        file_put_contents($this->rootDir.'/package.json', $newContents);
+        if ($didChangePackageJson) {
+            if (isset($content['dependencies'])) {
+                $manipulator->addMainKey('dependencies', $content['dependencies']);
+            }
+
+            if (isset($content['devDependencies'])) {
+                $devDependencies = $content['devDependencies'];
+                uksort($devDependencies, 'strnatcmp');
+                $manipulator->addMainKey('devDependencies', $devDependencies);
+            }
+
+            $newContents = $manipulator->getContents();
+            if ($newContents === file_get_contents($this->rootDir.'/package.json')) {
+                return false;
+            }
+
+            file_put_contents($this->rootDir.'/package.json', $manipulator->getContents());
+        }
 
-        return true;
+        return $didChangePackageJson;
     }
 
     private function registerWebpackResources(array $phpPackages)
@@ -151,7 +197,7 @@ private function registerWebpackResources(array $phpPackages)
                     continue;
                 }
 
-                // Otherwise, the package exists: merge new config with uer config
+                // Otherwise, the package exists: merge new config with user config
                 $previousConfig = $previousControllersJson['controllers'][$name][$controllerName];
 
                 $config = [];
@@ -180,39 +226,6 @@ private function registerWebpackResources(array $phpPackages)
         file_put_contents($controllersJsonPath, json_encode($newControllersJson, \JSON_PRETTY_PRINT | \JSON_UNESCAPED_SLASHES)."\n");
     }
 
-    public function registerPeerDependencies(array $phpPackages)
-    {
-        $peerDependencies = [];
-
-        foreach ($phpPackages as $phpPackage) {
-            if (!$packageJson = $this->resolvePackageJson($phpPackage)) {
-                continue;
-            }
-
-            $versionParser = new VersionParser();
-
-            foreach ($packageJson->read()['peerDependencies'] ?? [] as $peerDependency => $constraint) {
-                $peerDependencies[$peerDependency][$constraint] = $versionParser->parseConstraints($constraint);
-            }
-        }
-
-        if (!$peerDependencies) {
-            return;
-        }
-
-        $manipulator = new JsonManipulator(file_get_contents($this->rootDir.'/package.json'));
-        $content = json_decode($manipulator->getContents(), true);
-        $devDependencies = $content['devDependencies'] ?? [];
-
-        foreach ($peerDependencies as $peerDependency => $constraints) {
-            $devDependencies[$peerDependency] = $this->compactConstraints($constraints);
-        }
-        uksort($devDependencies, 'strnatcmp');
-        $manipulator->addMainKey('devDependencies', $devDependencies);
-
-        file_put_contents($this->rootDir.'/package.json', $manipulator->getContents());
-    }
-
     private function resolvePackageJson(array $phpPackage): ?JsonFile
     {
         $packageDir = $this->rootDir.'/'.$this->vendorDir.'/'.$phpPackage['name'];
@@ -233,26 +246,4 @@ private function resolvePackageJson(array $phpPackage): ?JsonFile
 
         return null;
     }
-
-    /**
-     * @param ConstraintInterface[] $constraints
-     */
-    private function compactConstraints(array $constraints): string
-    {
-        foreach ($constraints as $k1 => $constraint1) {
-            foreach ($constraints as $k2 => $constraint2) {
-                if ($k1 !== $k2 && Intervals::isSubsetOf($constraint1, $constraint2)) {
-                    unset($constraints[$k2]);
-                }
-            }
-        }
-
-        uksort($constraints, 'strnatcmp');
-
-        foreach ($constraints as $k => $constraint) {
-            $constraints[$k] = \count($constraints) > 1 && false !== strpos($k, '|') ? '('.$k.')' : $k;
-        }
-
-        return implode(',', $constraints);
-    }
 }
diff --git a/src/Update/DiffHelper.php b/src/Update/DiffHelper.php
@@ -35,6 +35,11 @@ public static function removeFilesFromPatch(string $patch, array $files, array &
             $patch = $contentBefore.substr($patch, $end);
         }
 
+        // valid patches end with a blank line
+        if ($patch && "\n" !== substr($patch, \strlen($patch) - 1, 1)) {
+            $patch = $patch."\n";
+        }
+
         return $patch;
     }
 }
diff --git a/tests/Configurator/CopyFromPackageConfiguratorTest.php b/tests/Configurator/CopyFromPackageConfiguratorTest.php
@@ -110,7 +110,11 @@ public function testUnconfigure()
         file_put_contents($this->targetFile, '');
         $this->assertFileExists($this->targetFile);
         $lock = $this->getMockBuilder(Lock::class)->disableOriginalConstructor()->getMock();
-        $this->createConfigurator()->unconfigure($this->recipe, [$this->sourceFileRelativePath => $this->targetFileRelativePath], $lock);
+        $this->createConfigurator()->unconfigure(
+            $this->recipe,
+            [$this->sourceFileRelativePath => $this->targetFileRelativePath, 'missingdir/' => ''],
+            $lock
+        );
         $this->assertFileDoesNotExist($this->targetFile);
     }
 
diff --git a/tests/Fixtures/packageJson/elevated_dependencies_package.json b/tests/Fixtures/packageJson/elevated_dependencies_package.json
@@ -0,0 +1,15 @@
+{
+   "name": "symfony/fixture",
+   "dependencies": {
+      "@hotcookies": "^1.1|^2",
+      "@hotdogs": "^2",
+      "@symfony/existing-package": "file:vendor/symfony/existing-package/Resources/assets"
+   },
+   "devDependencies": {
+      "@symfony/stimulus-bridge": "^1.0.0",
+      "stimulus": "^1.1.1"
+   },
+   "browserslist": [
+      "defaults"
+   ]
+}
diff --git a/tests/PackageJsonSynchronizerTest.php b/tests/PackageJsonSynchronizerTest.php
@@ -11,7 +11,6 @@
 
 namespace Symfony\Flex\Tests;
 
-use Composer\Semver\Intervals;
 use PHPUnit\Framework\TestCase;
 use Symfony\Component\Filesystem\Filesystem;
 use Symfony\Flex\PackageJsonSynchronizer;
@@ -145,7 +144,6 @@ public function testSynchronizeNewPackage()
             '{
    "name": "symfony/fixture",
    "devDependencies": {
-      "@hotcookies": "'.(method_exists(Intervals::class, 'isSubsetOf') ? '^1.1' : '^1.1,(^1.1|^2)').'",
       "@hotdogs": "^2",
       "@symfony/existing-package": "file:vendor/symfony/existing-package/Resources/assets",
       "@symfony/new-package": "file:vendor/symfony/new-package/assets",
@@ -215,4 +213,36 @@ public function testArrayFormattingHasNotChanged()
             trim(file_get_contents($this->tempDir.'/package.json'))
         );
     }
+
+    public function testExistingElevatedPackage()
+    {
+        (new Filesystem())->copy($this->tempDir.'/elevated_dependencies_package.json', $this->tempDir.'/package.json', true);
+
+        $this->synchronizer->synchronize([
+            [
+                'name' => 'symfony/existing-package',
+                'keywords' => ['symfony-ux'],
+            ],
+        ]);
+
+        // Should keep existing package references and config
+        $this->assertSame(
+            [
+                'name' => 'symfony/fixture',
+                'dependencies' => [
+                    '@hotcookies' => '^1.1|^2',
+                    '@hotdogs' => '^2',
+                    '@symfony/existing-package' => 'file:vendor/symfony/existing-package/Resources/assets',
+                ],
+                'devDependencies' => [
+                    '@symfony/stimulus-bridge' => '^1.0.0',
+                    'stimulus' => '^1.1.1',
+                ],
+                'browserslist' => [
+                    'defaults',
+                ],
+            ],
+            json_decode(file_get_contents($this->tempDir.'/package.json'), true)
+        );
+    }
 }
diff --git a/tests/Update/DiffHelperTest.php b/tests/Update/DiffHelperTest.php
@@ -105,6 +105,7 @@ public function getRemoveFilesFromPatchTests(): iterable
 +    dbal:
 +        # "TEST_TOKEN" is typically set by ParaTest
 +        dbname_suffix: '_test%env(default::TEST_TOKEN)%'
+
 EOF
             , [
                 '.env' => <<<EOF
@@ -154,6 +155,7 @@ public function getRemoveFilesFromPatchTests(): iterable
 +    dbal:
 +        # "TEST_TOKEN" is typically set by ParaTest
 +        dbname_suffix: '_test%env(default::TEST_TOKEN)%'
+
 EOF
             , [
                 'config/packages/doctrine.yaml' => <<<EOF
@@ -204,6 +206,7 @@ public function getRemoveFilesFromPatchTests(): iterable
 -            type: pool
 -            pool: doctrine.system_cache_pool
          query_cache_driver:
+
 EOF
             , [
                 'config/packages/test/doctrine.yaml' => <<<EOF
@@ -244,6 +247,7 @@ public function getRemoveFilesFromPatchTests(): iterable
 -            type: pool
 -            pool: doctrine.system_cache_pool
          query_cache_driver:
+
 EOF
             , [
                 'config/packages/test/doctrine.yaml' => <<<EOF

Original file line number	Diff line number	Diff line change
`@@ -145,6 +145,9 @@ public function copyFile(string $source, string $target, array $options)`
`145`	`145`
`146`	`146`	`private function removeFilesFromDir(string $source, string $target)`
`147`	`147`	`{`
	`148`	`+ if (!is_dir($source)) {`
	`149`	`+ return;`
	`150`	`+ }`
`148`	`151`	`$iterator = $this->createSourceIterator($source, \RecursiveIteratorIterator::CHILD_FIRST);`
`149`	`152`	`foreach ($iterator as $item) {`
`150`	`153`	`$targetPath = $this->path->concatenate([$target, $iterator->getSubPathName()]);`
Original file line number	Diff line number	Diff line change
`@@ -35,6 +35,11 @@ public static function removeFilesFromPatch(string $patch, array $files, array &`
`35`	`35`	`$patch = $contentBefore.substr($patch, $end);`
`36`	`36`	`}`
`37`	`37`
	`38`	`+ // valid patches end with a blank line`
	`39`	`+ if ($patch && "\n" !== substr($patch, \strlen($patch) - 1, 1)) {`
	`40`	`+ $patch = $patch."\n";`
	`41`	`+ }`
	`42`	`+`
`38`	`43`	`return $patch;`
`39`	`44`	`}`
`40`	`45`	`}`