From ef7959d85aaea70b0c0fb58055011565fa86232a Mon Sep 17 00:00:00 2001 From: Wim Leers <44946-wimleers@users.noreply.drupalcode.org> Date: Thu, 9 Mar 2023 21:17:52 +0000 Subject: [PATCH] Issue #3345633 by Wim Leers, phenaproxima: Remove FixtureManipulator::modifyPackage() last usage --- .../src/FixtureManipulator.php | 299 ++++++++---------- .../Kernel/ComposerPatchesValidatorTest.php | 4 +- .../src/Kernel/FixtureManipulatorTest.php | 49 +-- .../ScaffoldFilePermissionsValidatorTest.php | 35 +- .../StagedProjectsValidatorTest.php | 3 - 5 files changed, 166 insertions(+), 224 deletions(-) diff --git a/package_manager/tests/modules/fixture_manipulator/src/FixtureManipulator.php b/package_manager/tests/modules/fixture_manipulator/src/FixtureManipulator.php index b08421244e..5e4d7f7719 100644 --- a/package_manager/tests/modules/fixture_manipulator/src/FixtureManipulator.php +++ b/package_manager/tests/modules/fixture_manipulator/src/FixtureManipulator.php @@ -2,7 +2,6 @@ namespace Drupal\fixture_manipulator; -use Composer\Semver\VersionParser; use Drupal\Component\FileSystem\FileSystem; use Drupal\Component\Utility\NestedArray; use PhpTuf\ComposerStager\Domain\Service\ProcessOutputCallback\ProcessOutputCallbackInterface; @@ -81,8 +80,7 @@ class FixtureManipulator { * Adds a package. * * @param array $package - * The package info that should be added to installed.json and - * installed.php. Must include the `name` and `type` keys. + * A Composer package definition. Must include the `name` and `type` keys. * @param bool $is_dev_requirement * Whether or not the package is a development requirement. * @param bool $allow_plugins @@ -143,7 +141,28 @@ class FixtureManipulator { file_put_contents("$repo_path/$file_name", $file_contents); } } - $command_options = ['require', "{$package['name']}:{$package['version']}"]; + return $this->requirePackage($package['name'], $package['version'], $is_dev_requirement, $allow_plugins); + } + + /** + * Requires a package. + * + * @param string $package + * A package name. + * @param string $version + * A version constraint. + * @param bool $is_dev_requirement + * Whether or not the package is a development requirement. + * @param bool $allow_plugins + * Whether or not to use the '--no-plugins' option. + */ + public function requirePackage(string $package, string $version, bool $is_dev_requirement = FALSE, bool $allow_plugins = FALSE): self { + if (!$this->committingChanges) { + $this->queueManipulation('requirePackage', func_get_args()); + return $this; + } + + $command_options = ['require', "$package:$version"]; if ($is_dev_requirement) { $command_options[] = '--dev'; } @@ -156,28 +175,31 @@ class FixtureManipulator { } /** - * Modifies a package's installed info. + * Modifies a package's composer.json properties. * - * @todo Since ::setVersion() is not longer calling this method the only test - * the is using this that is not just testing this method itself is - * \Drupal\Tests\automatic_updates\Kernel\StatusCheck\ScaffoldFilePermissionsValidatorTest::testScaffoldFilesChanged - * That test is passing, so we could leave it, then we have to leave - * ::setPackage() which is very complicated. Will leave notes on - * testScaffoldFilesChanged() how we might solve that with composer commands - * instead of this method. - * - * @param string $name + * @param string $package_name * The name of the package to modify. - * @param array $package - * The package info that should be updated in installed.json and - * installed.php. + * @param string $version + * The version to use for the modified package. Can be the same as the + * original version, or a different version. + * @param array $config + * The config to be added to the package's composer.json. + * @param bool $is_dev_requirement + * Whether or not the package is a development requirement. + * + * @see \Composer\Command\ConfigCommand */ - public function modifyPackage(string $name, array $package): self { + public function modifyPackageConfig(string $package_name, string $version, array $config, bool $is_dev_requirement = FALSE): self { if (!$this->committingChanges) { - $this->queueManipulation('modifyPackage', func_get_args()); + $this->queueManipulation('modifyPackageConfig', func_get_args()); return $this; } - $this->setPackage($name, $package, TRUE); + $package = [ + 'name' => $package_name, + 'version' => $version, + ] + $config; + $this->addRepository($package); + $this->runComposerCommand(array_filter(['require', "$package_name:$version", $is_dev_requirement ? '--dev' : NULL])); return $this; } @@ -198,13 +220,7 @@ class FixtureManipulator { $this->queueManipulation('setVersion', func_get_args()); return $this; } - $package = [ - 'name' => $package_name, - 'version' => $version, - ]; - $this->addRepository($package); - $this->runComposerCommand(array_filter(['require', "$package_name:$version", $is_dev_requirement ? '--dev' : NULL])); - return $this; + return $this->modifyPackageConfig($package_name, $version, [], $is_dev_requirement); } /** @@ -236,125 +252,6 @@ class FixtureManipulator { return $this; } - /** - * Changes a package's installation information in a particular directory. - * - * This function is internal and should not be called directly. Use - * ::addPackage(), ::modifyPackage(), and ::removePackage() instead. - * - * @todo Remove this method once ::modifyPackage() doesn't call it. - * - * @param string $pretty_name - * The name of the package to add, update, or remove. - * @param array|null $package - * The package information to be set in installed.json and installed.php, or - * NULL to remove it. Will be merged into the existing information if the - * package is already installed. - * @param bool $should_exist - * Whether or not the package is expected to already be installed. - * @param bool|null $is_dev_requirement - * Whether or not the package is a developer requirement. - */ - private function setPackage(string $pretty_name, ?array $package, bool $should_exist, ?bool $is_dev_requirement = NULL): void { - // @see \Composer\Package\BasePackage::__construct() - $name = strtolower($pretty_name); - - if ($should_exist && isset($is_dev_requirement)) { - throw new \LogicException('Changing an existing project to a dev requirement is not supported'); - } - $composer_folder = $this->dir . '/vendor/composer'; - - $file = $composer_folder . '/installed.json'; - self::ensureFilePathIsWritable($file); - - $data = file_get_contents($file); - $data = json_decode($data, TRUE, 512, JSON_THROW_ON_ERROR); - - // If the package is already installed, find its numerical index. - $position = NULL; - for ($i = 0; $i < count($data['packages']); $i++) { - if ($data['packages'][$i]['name'] === $name) { - $position = $i; - break; - } - } - // Ensure that we actually expect to find the package already installed (or - // not). - $expected_package_message = $should_exist - ? "Expected package '$pretty_name' to be installed, but it wasn't." - : "Expected package '$pretty_name' to not be installed, but it was."; - if ($should_exist !== isset($position)) { - throw new \LogicException($expected_package_message); - } - - if ($package) { - $package = ['name' => $pretty_name] + $package; - $install_json_package = $package; - // Composer will use 'version_normalized', if present, to determine the - // version number. - if (isset($install_json_package['version']) && !isset($install_json_package['version_normalized'])) { - $parser = new VersionParser(); - $install_json_package['version_normalized'] = $parser->normalize($install_json_package['version']); - } - } - - if (isset($position)) { - // If we're going to be updating the package data, merge the incoming data - // into what we already have. - if ($package) { - $install_json_package = $install_json_package + $data['packages'][$position]; - } - - // If `$package === NULL`, the existing package should be removed. - if ($package === NULL) { - array_splice($data['packages'], $position, 1); - $is_existing_dev_package = in_array($name, $data['dev-package-names'], TRUE); - $data['dev-package-names'] = array_diff($data['dev-package-names'], [$name]); - $data['dev-package-names'] = array_values($data['dev-package-names']); - } - } - // Add the package back to the list, if we have data for it. - if (isset($install_json_package)) { - // If it previously existed, put it back in the previous position. - if ($position) { - $data['packages'][$i] = $install_json_package; - } - // Otherwise, it must be new: append it to the list. - else { - $data['packages'][] = $install_json_package; - } - - if ($is_dev_requirement || !empty($is_existing_dev_package)) { - $data['dev-package-names'][] = $name; - } - } - file_put_contents($file, json_encode($data, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES)); - self::ensureFilePathIsWritable($file); - - $file = $composer_folder . '/installed.php'; - self::ensureFilePathIsWritable($file); - - $data = require $file; - - // Ensure that we actually expect to find the package already installed (or - // not). - if ($should_exist !== isset($data['versions'][$name])) { - throw new \LogicException($expected_package_message); - } - if ($package) { - $install_php_package = $should_exist ? - NestedArray::mergeDeep($data['versions'][$name], $package) : - $package; - $data['versions'][$name] = $install_php_package; - } - else { - unset($data['versions'][$name]); - } - - $data = var_export($data, TRUE); - file_put_contents($file, "<?php\nreturn $data;"); - } - /** * Adds a project at a path. * @@ -402,7 +299,9 @@ class FixtureManipulator { } /** - * Modifies a package's installed info. + * Modifies the project root's composer.json properties. + * + * @see \Composer\Command\ConfigCommand * * @param array $additional_config * The configuration to add. @@ -574,7 +473,7 @@ class FixtureManipulator { * Transform the received $package into options for `composer init`. * * @param array $package - * A Composer package definition. + * A Composer package definition. Must include the `name` and `type` keys. * * @return array * The corresponding `composer init` options. @@ -589,6 +488,9 @@ class FixtureManipulator { case 'require': case 'require-dev': + if (empty($v)) { + return NULL; + } $requirements = array_map( fn(string $req_package, string $req_version): string => "$req_package:$req_version", array_keys($v), @@ -610,11 +512,50 @@ class FixtureManipulator { }, array_keys($package), array_values($package))); } + /** + * Creates a path repo. + * + * @param array $package + * A Composer package definition. Must include the `name` and `type` keys. + * @param string $repo_path + * The path at which to create a path repo for this package. + * @param string|null $original_repo_path + * If NULL: this is the first version of this package. Otherwise: a string + * containing the path repo to the first version of this package. This will + * be used to automatically inherit the same files (typically *.info.yml). + */ + private function createPathRepo(array $package, string $repo_path, ?string $original_repo_path): void { + $fs = new SymfonyFileSystem(); + if (is_dir($repo_path)) { + throw new \LogicException("A path repo already exists at $repo_path."); + } + // Create the repo if it does not exist. + $fs->mkdir($repo_path); + // Forks also get the original's additional files (e.g. *.info.yml files). + if ($original_repo_path) { + $fs->mirror($original_repo_path, $repo_path); + // composer.json will be freshly generated by `composer init` below. + $fs->remove($repo_path . '/composer.json'); + } + // Switch the working directory from project root to repo path. + $project_root_dir = $this->dir; + $this->dir = $repo_path; + // Create a composer.json file using `composer init`. + $this->runComposerCommand(['init', ...static::getComposerInitOptionsForPackage($package)]); + // Set the `extra` property in the generated composer.json file using + // `composer config`, because `composer init` does not support it. + foreach ($package['extra'] ?? [] as $extra_property => $extra_value) { + $this->runComposerCommand(['config', "extra.$extra_property", '--json', json_encode($extra_value, JSON_UNESCAPED_SLASHES)]); + } + // Restore the project root as the working directory. + $this->dir = $project_root_dir; + } + /** * Adds a path repository. * * @param array $package - * The package. + * A Composer package definition. Must include the `name` and `type` keys. * * @return string * The repository path. @@ -623,27 +564,45 @@ class FixtureManipulator { $name = $package['name']; $path_repo_base = \Drupal::state()->get(self::PATH_REPO_STATE_KEY); $repo_path = "$path_repo_base/" . str_replace('/', '--', $name); - $fs = new SymfonyFileSystem(); - if (!is_dir($repo_path)) { - // Create the repo if it does not exist. - $fs->mkdir($repo_path); - // Switch the working directory from project root to repo path. - $project_root_dir = $this->dir; - $this->dir = $repo_path; - // Create a composer.json file using `composer init`. - $this->runComposerCommand(['init', ...static::getComposerInitOptionsForPackage($package)]); - // Set the `extra` property in the generated composer.json file using - // `composer config`, because `composer init` does not support it. - foreach ($package['extra'] ?? [] as $extra_property => $extra_value) { - $this->runComposerCommand(['config', "extra.$extra_property", '--json', json_encode($extra_value, JSON_UNESCAPED_SLASHES)]); + + // Determine if the given $package is a new package or a fork of an existing + // one (that means it's either the same version but with other metadata, or + // a new version with other metadata). Existing path repos are never + // modified, not even if the same version of a package is assigned other + // metadata. This allows always comparing with the original metadata. + $is_new_or_fork = !is_dir($repo_path) ? 'new' : 'fork'; + if ($is_new_or_fork === 'fork') { + $original_composer_json_path = $repo_path . DIRECTORY_SEPARATOR . 'composer.json'; + $original_repo_path = $repo_path; + $original_composer_json_data = json_decode(file_get_contents($original_composer_json_path), TRUE); + $forked_composer_json_data = NestedArray::mergeDeep($original_composer_json_data, $package); + if ($original_composer_json_data === $forked_composer_json_data) { + throw new \LogicException(sprintf('Nothing is actually different in this fork of the package %s.', $package['name'])); + } + $package = $forked_composer_json_data; + $repo_path .= "--{$package['version']}"; + // Cannot create multiple forks with the same version. This is likely + // due to a test simulating a failed Stage::apply(). + if (!is_dir($repo_path)) { + $this->createPathRepo($package, $repo_path, $original_repo_path); + } + } + else { + $this->createPathRepo($package, $repo_path, NULL); + } + + // Register the repository, keyed by package name and version. This ensures + // each package is registered only once and its version can be updated (but + // must have unique versions). + $repo_key = "repo.$name"; + if ($is_new_or_fork === 'fork') { + $repositories = json_decode(file_get_contents($this->dir . '/composer.json'), TRUE)['repositories']; + // @todo consistently use 'version' or 'options.versions.PACKAGE_NAME', by fixing ComposerFixtureCreator in https://drupal.org/i/3347055 + $original_version = isset($repositories[$name]['version']) ? $repositories[$name]['version'] : $repositories[$name]['options']['versions'][$name]; + if ($package['version'] !== $original_version) { + $repo_key .= "--" . $package['version']; } - // Restore the project root as the working directory. - $this->dir = $project_root_dir; } - - // Register the repository, keyed by package name. This ensures each package - // is registered only once and its version can be updated. - // @todo Should we create 1 repo per version. $repository = json_encode([ 'type' => 'path', 'url' => $repo_path, @@ -652,9 +611,11 @@ class FixtureManipulator { 'versions' => [ $name => $package['version'], ], + // @see https://getcomposer.org/repoprio + 'canonical' => FALSE, ], ], JSON_UNESCAPED_SLASHES); - $this->runComposerCommand(['config', "repo.$name", $repository]); + $this->runComposerCommand(['config', $repo_key, $repository]); return $repo_path; } diff --git a/package_manager/tests/src/Kernel/ComposerPatchesValidatorTest.php b/package_manager/tests/src/Kernel/ComposerPatchesValidatorTest.php index 25b7af06fa..bcad090fef 100644 --- a/package_manager/tests/src/Kernel/ComposerPatchesValidatorTest.php +++ b/package_manager/tests/src/Kernel/ComposerPatchesValidatorTest.php @@ -214,9 +214,7 @@ class ComposerPatchesValidatorTest extends PackageManagerKernelTestBase { ]); } if ($in_stage & static::REQUIRE_PACKAGE_FROM_ROOT && !($in_active & static::REQUIRE_PACKAGE_FROM_ROOT)) { - $package_data = json_decode(file_get_contents(__DIR__ . '/../../fixtures/path_repos/cweagans--composer-patches/composer.json'), TRUE); - $package_data['version'] = '24.12.1999'; - $stage_manipulator->addPackage($package_data); + $stage_manipulator->requirePackage('cweagans/composer-patches', '24.12.1999'); } if (!($in_stage & static::REQUIRE_PACKAGE_FROM_ROOT) && $in_active & static::REQUIRE_PACKAGE_FROM_ROOT) { $stage_manipulator diff --git a/package_manager/tests/src/Kernel/FixtureManipulatorTest.php b/package_manager/tests/src/Kernel/FixtureManipulatorTest.php index 1728012d78..16e0d66481 100644 --- a/package_manager/tests/src/Kernel/FixtureManipulatorTest.php +++ b/package_manager/tests/src/Kernel/FixtureManipulatorTest.php @@ -6,7 +6,6 @@ namespace Drupal\Tests\package_manager\Kernel; use Drupal\fixture_manipulator\ActiveFixtureManipulator; use Drupal\fixture_manipulator\FixtureManipulator; -use Symfony\Component\Filesystem\Filesystem; /** * @coversDefaultClass \Drupal\fixture_manipulator\FixtureManipulator @@ -64,6 +63,7 @@ class FixtureManipulatorTest extends PackageManagerKernelTestBase { ->addPackage([ 'name' => 'my/package', 'type' => 'library', + 'version' => '1.2.3', ]) ->addPackage( [ @@ -177,8 +177,9 @@ class FixtureManipulatorTest extends PackageManagerKernelTestBase { /** * @covers ::modifyPackage */ - public function testModifyPackage(): void { - $fs = (new Filesystem()); + public function testModifyPackageConfig(): void { + $inspector = $this->container->get('package_manager.composer_inspector'); + // Assert ::modifyPackage() works with a package in an existing fixture not // created by ::addPackage(). $decode_installed_json = function () { @@ -188,41 +189,19 @@ class FixtureManipulatorTest extends PackageManagerKernelTestBase { $this->assertIsArray($original_installed_json); (new ActiveFixtureManipulator()) // @see ::setUp() - ->modifyPackage('my/dev-package', ['version' => '2.1.0']) + ->modifyPackageConfig('my/dev-package', '2.1.0', ['description' => 'something else'], TRUE) ->commitChanges(); - $this->assertSame($original_installed_json, $decode_installed_json()); - - // Assert that ::modifyPackage() throws an error if a package exists in the - // 'installed.json' file but not the 'installed.php' file. We cannot test - // this with the trait functions because they cannot produce this starting - // point. - $existing_incorrect_fixture = __DIR__ . '/../../fixtures/FixtureUtilityTraitTest/missing_installed_php'; - $temp_fixture = $this->siteDirectory . $this->randomMachineName('42'); - $fs->mirror($existing_incorrect_fixture, $temp_fixture); - try { - (new FixtureManipulator()) - ->modifyPackage('the-org/the-package', ['irrelevant' => TRUE]) - ->commitChanges($temp_fixture); - $this->fail('Modifying a non-existent package should raise an error.'); - } - catch (\LogicException $e) { - $this->assertSame("Expected package 'the-org/the-package' to be installed, but it wasn't.", $e->getMessage()); - } - - // We should not be able to modify a non-existent package. - try { - (new ActiveFixtureManipulator()) - ->modifyPackage('junk/drawer', ['type' => 'library']) - ->commitChanges(); - $this->fail('Modifying a non-existent package should raise an error.'); - } - catch (\LogicException $e) { - $this->assertStringContainsString("Expected package 'junk/drawer' to be installed, but it wasn't.", $e->getMessage()); - } + // Verify that the package is indeed properly installed. + $this->assertSame('2.1.0', $inspector->getInstalledPackagesList($this->dir)['my/dev-package']->version); + // Verify that the original exists, but has no description. + $this->assertSame('my/dev-package', $original_installed_json['packages'][3]['name']); + $this->assertArrayNotHasKey('description', $original_installed_json['packages']); + // Verify that the description was updated. + $this->assertSame('something else', $decode_installed_json()['packages'][3]['description']); (new ActiveFixtureManipulator()) // Add a key to an existing package. - ->modifyPackage('my/package', ['type' => 'metapackage']) + ->modifyPackageConfig('my/package', '1.2.3', ['extra' => ['foo' => 'bar']]) // Change a key in an existing package. ->setVersion('my/dev-package', '3.2.1', TRUE) // Move an existing package to dev requirements. @@ -249,7 +228,7 @@ class FixtureManipulatorTest extends PackageManagerKernelTestBase { 'name' => 'my/package', 'version' => '1.2.3', 'version_normalized' => '1.2.3.0', - 'type' => 'metapackage', + 'type' => 'library', ], ]; $installed_php_expected_packages = $install_json_expected_packages; diff --git a/tests/src/Kernel/StatusCheck/ScaffoldFilePermissionsValidatorTest.php b/tests/src/Kernel/StatusCheck/ScaffoldFilePermissionsValidatorTest.php index e2c6bde263..0c3ce3bfa6 100644 --- a/tests/src/Kernel/StatusCheck/ScaffoldFilePermissionsValidatorTest.php +++ b/tests/src/Kernel/StatusCheck/ScaffoldFilePermissionsValidatorTest.php @@ -293,27 +293,34 @@ class ScaffoldFilePermissionsValidatorTest extends AutomaticUpdatesKernelTestBas * @dataProvider providerScaffoldFilesChanged */ public function testScaffoldFilesChanged(array $write_protected_paths, array $active_scaffold_files, array $staged_scaffold_files, array $expected_results): void { - // Rewrite the active and staged installed.json files, inserting the given + // Rewrite the active and staged composer.json files, inserting the given // lists of scaffold files. - // @todo Remove the use of modifyPackage() in https://drupal.org/i/3345633. - (new ActiveFixtureManipulator()) - ->modifyPackage('drupal/core', [ - 'extra' => [ - 'drupal-scaffold' => [ - 'file-mapping' => $active_scaffold_files, + if ($active_scaffold_files) { + (new ActiveFixtureManipulator()) + ->modifyPackageConfig('drupal/core', '9.8.0', [ + 'extra' => [ + 'drupal-scaffold' => [ + 'file-mapping' => $active_scaffold_files, + ], ], - ], - ]) - ->commitChanges(); - $this->getStageFixtureManipulator() - ->setCorePackageVersion('9.8.1') - ->modifyPackage('drupal/core', [ + ]) + ->commitChanges(); + } + $stage_manipulator = $this->getStageFixtureManipulator(); + $stage_manipulator->setVersion('drupal/core-recommended', '9.8.1'); + $stage_manipulator->setVersion('drupal/core-dev', '9.8.1'); + if ($staged_scaffold_files) { + $stage_manipulator->modifyPackageConfig('drupal/core', '9.8.1', [ 'extra' => [ 'drupal-scaffold' => [ 'file-mapping' => $staged_scaffold_files, ], ], ]); + } + else { + $stage_manipulator->setVersion('drupal/core', '9.8.1'); + } // Create fake scaffold files so we can test scenarios in which a scaffold // file that exists in the active directory is deleted in the stage @@ -331,7 +338,7 @@ class ScaffoldFilePermissionsValidatorTest extends AutomaticUpdatesKernelTestBas $updater->apply(); // If no exception was thrown, ensure that we weren't expecting an error. - $this->assertEmpty($expected_results); + $this->assertSame([], $expected_results); } // If we try to overwrite any write-protected paths, even if they're not // scaffold files, we'll get an ApplyFailedException. diff --git a/tests/src/Kernel/StatusCheck/StagedProjectsValidatorTest.php b/tests/src/Kernel/StatusCheck/StagedProjectsValidatorTest.php index f0a7b9d12f..0e81882e91 100644 --- a/tests/src/Kernel/StatusCheck/StagedProjectsValidatorTest.php +++ b/tests/src/Kernel/StatusCheck/StagedProjectsValidatorTest.php @@ -157,7 +157,6 @@ class StagedProjectsValidatorTest extends AutomaticUpdatesKernelTestBase { */ public function testProjectsRemoved(): void { (new ActiveFixtureManipulator()) - ->setCorePackageVersion('9.8.0') ->addPackage([ 'name' => 'drupal/test_theme', 'version' => '1.3.0', @@ -230,7 +229,6 @@ class StagedProjectsValidatorTest extends AutomaticUpdatesKernelTestBase { */ public function testVersionsChanged(): void { (new ActiveFixtureManipulator()) - ->setCorePackageVersion('9.8.0') ->addPackage([ 'name' => 'drupal/test-module', 'version' => '1.3.0', @@ -291,7 +289,6 @@ class StagedProjectsValidatorTest extends AutomaticUpdatesKernelTestBase { */ public function testNoErrors(): void { (new ActiveFixtureManipulator()) - ->setCorePackageVersion('9.8.0') ->addPackage([ 'name' => 'drupal/test-module', 'version' => '1.3.0', -- GitLab