From f689d746417c08c35dec9add9757d511b3ebffd0 Mon Sep 17 00:00:00 2001 From: Ted Bowman <41201-tedbow@users.noreply.drupalcode.org> Date: Sun, 6 Nov 2022 20:05:52 -0500 Subject: [PATCH] Issue #3318927 by tedbow: FixtureUtilityTrait incorrectly assumes that the package keys are the same in install.json and installed.php --- .../vendor/composer/installed.json | 9 + .../vendor/composer/installed.php | 17 ++ .../vendor/composer/installed.json | 9 + .../vendor/composer/installed.php | 11 + .../fake_site/vendor/composer/installed.php | 6 +- .../src/Kernel/FixtureUtilityTraitTest.php | 84 ++++++-- .../tests/src/Traits/FixtureUtilityTrait.php | 66 ++++-- .../StagedProjectsValidatorTest.php | 200 +++++++++++------- 8 files changed, 277 insertions(+), 125 deletions(-) create mode 100644 package_manager/tests/fixtures/FixtureUtilityTraitTest/existing_correct_fixture/vendor/composer/installed.json create mode 100644 package_manager/tests/fixtures/FixtureUtilityTraitTest/existing_correct_fixture/vendor/composer/installed.php create mode 100644 package_manager/tests/fixtures/FixtureUtilityTraitTest/missing_installed_php/vendor/composer/installed.json create mode 100644 package_manager/tests/fixtures/FixtureUtilityTraitTest/missing_installed_php/vendor/composer/installed.php diff --git a/package_manager/tests/fixtures/FixtureUtilityTraitTest/existing_correct_fixture/vendor/composer/installed.json b/package_manager/tests/fixtures/FixtureUtilityTraitTest/existing_correct_fixture/vendor/composer/installed.json new file mode 100644 index 0000000000..4c9f037cd9 --- /dev/null +++ b/package_manager/tests/fixtures/FixtureUtilityTraitTest/existing_correct_fixture/vendor/composer/installed.json @@ -0,0 +1,9 @@ +{ + "packages": [ + { + "name": "the-org/the-package", + "version": "9.8.0" + } + ], + "dev-package-names": [] +} \ No newline at end of file diff --git a/package_manager/tests/fixtures/FixtureUtilityTraitTest/existing_correct_fixture/vendor/composer/installed.php b/package_manager/tests/fixtures/FixtureUtilityTraitTest/existing_correct_fixture/vendor/composer/installed.php new file mode 100644 index 0000000000..4190670a2b --- /dev/null +++ b/package_manager/tests/fixtures/FixtureUtilityTraitTest/existing_correct_fixture/vendor/composer/installed.php @@ -0,0 +1,17 @@ +<?php + +/** + * @file + * Fixture for \Drupal\Tests\package_manager\Kernel\FixtureUtilityTraitTest::testModifyPackage(). + */ + +return [ + 'versions' => + [ + 'the-org/the-package' => + [ + 'name' => 'the-org/the-package', + 'install_path' => __DIR__ . '/../../a_new_path', + ], + ], +]; diff --git a/package_manager/tests/fixtures/FixtureUtilityTraitTest/missing_installed_php/vendor/composer/installed.json b/package_manager/tests/fixtures/FixtureUtilityTraitTest/missing_installed_php/vendor/composer/installed.json new file mode 100644 index 0000000000..4c9f037cd9 --- /dev/null +++ b/package_manager/tests/fixtures/FixtureUtilityTraitTest/missing_installed_php/vendor/composer/installed.json @@ -0,0 +1,9 @@ +{ + "packages": [ + { + "name": "the-org/the-package", + "version": "9.8.0" + } + ], + "dev-package-names": [] +} \ No newline at end of file diff --git a/package_manager/tests/fixtures/FixtureUtilityTraitTest/missing_installed_php/vendor/composer/installed.php b/package_manager/tests/fixtures/FixtureUtilityTraitTest/missing_installed_php/vendor/composer/installed.php new file mode 100644 index 0000000000..e6e509a25d --- /dev/null +++ b/package_manager/tests/fixtures/FixtureUtilityTraitTest/missing_installed_php/vendor/composer/installed.php @@ -0,0 +1,11 @@ +<?php + +/** + * @file + * Fixture for \Drupal\Tests\package_manager\Kernel\FixtureUtilityTraitTest::testModifyPackage(). + */ + +// Composer Utility needs the versions key to be present. +return [ + 'versions' => [], +]; diff --git a/package_manager/tests/fixtures/fake_site/vendor/composer/installed.php b/package_manager/tests/fixtures/fake_site/vendor/composer/installed.php index 4963cf582b..e1e78adc66 100644 --- a/package_manager/tests/fixtures/fake_site/vendor/composer/installed.php +++ b/package_manager/tests/fixtures/fake_site/vendor/composer/installed.php @@ -6,5 +6,9 @@ // Composer Utility needs the versions key to be present. return [ - 'versions' => [], + 'versions' => [ + 'drupal/core' => [ + 'name' => 'drupal/core', + ], + ], ]; diff --git a/package_manager/tests/src/Kernel/FixtureUtilityTraitTest.php b/package_manager/tests/src/Kernel/FixtureUtilityTraitTest.php index 6dd2829c03..f9677be07d 100644 --- a/package_manager/tests/src/Kernel/FixtureUtilityTraitTest.php +++ b/package_manager/tests/src/Kernel/FixtureUtilityTraitTest.php @@ -4,6 +4,7 @@ namespace Drupal\Tests\package_manager\Kernel; use Drupal\Tests\package_manager\Traits\FixtureUtilityTrait; use PHPUnit\Framework\AssertionFailedError; +use Symfony\Component\Filesystem\Filesystem; /** * @coversDefaultClass \Drupal\Tests\package_manager\Traits\FixtureUtilityTrait @@ -36,9 +37,10 @@ class FixtureUtilityTraitTest extends PackageManagerKernelTestBase { $this->addPackage($this->dir, [ 'name' => 'my/dev-package', 'version' => '2.1.0', - 'dev_requirement' => TRUE, 'install_path' => '../relative/path', - ]); + ], + TRUE, + ); } /** @@ -70,38 +72,76 @@ class FixtureUtilityTraitTest extends PackageManagerKernelTestBase { 'name' => 'absolute/path', 'install_path' => '/absolute/path', ]); + $this->fail('Add package should have failed.'); } catch (AssertionFailedError $e) { $this->assertSame('Failed asserting that \'/absolute/path\' starts with "../".', $e->getMessage()); } - $expected_packages = [ + $installed_json_expected_packages = [ 'my/package' => [ 'name' => 'my/package', ], 'my/dev-package' => [ 'name' => 'my/dev-package', 'version' => '2.1.0', - 'dev_requirement' => TRUE, - 'install_path' => '../relative/path', ], ]; + $installed_php_expected_packages = $installed_json_expected_packages; [$installed_json, $installed_php] = $this->getData(); - $installed_json['packages'] = array_intersect_key($installed_json['packages'], $expected_packages); - $this->assertSame($expected_packages, $installed_json['packages']); + $installed_json['packages'] = array_intersect_key($installed_json['packages'], $installed_json_expected_packages); + $this->assertSame($installed_json_expected_packages, $installed_json['packages']); $this->assertContains('my/dev-package', $installed_json['dev-package-names']); $this->assertNotContains('my/package', $installed_json['dev-package-names']); // In installed.php, the relative installation path of my/dev-package should // have been prefixed with the __DIR__ constant, which should be interpreted // when installed.php is loaded by the PHP runtime. - $expected_packages['my/dev-package']['install_path'] = "$this->dir/vendor/composer/../relative/path"; - $this->assertSame($expected_packages, $installed_php); + $installed_php_expected_packages['my/dev-package']['install_path'] = "$this->dir/vendor/composer/../relative/path"; + $installed_php_expected_packages = ['drupal/core' => ['name' => 'drupal/core']] + $installed_php_expected_packages; + $this->assertSame($installed_php_expected_packages, $installed_php); } /** * @covers ::modifyPackage */ public function testModifyPackage(): void { + $fs = (new Filesystem()); + // Assert ::modifyPackage() works with a package in an existing fixture not + // created by ::addPackage(). + $existing_fixture = __DIR__ . '/../../fixtures/FixtureUtilityTraitTest/existing_correct_fixture'; + $temp_fixture = $this->siteDirectory . $this->randomMachineName('42'); + $fs->mirror($existing_fixture, $temp_fixture); + $decode_installed_json = function () use ($temp_fixture) { + return json_decode(file_get_contents($temp_fixture . '/vendor/composer/installed.json'), TRUE, 512, JSON_THROW_ON_ERROR); + }; + $original_installed_json = $decode_installed_json(); + $this->assertIsArray($original_installed_json); + $this->modifyPackage( + $temp_fixture, + 'the-org/the-package', + ['install_path' => '../../a_new_path'], + ); + $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 { + $this->modifyPackage( + $temp_fixture, + 'the-org/the-package', + ['install_path' => '../../a_new_path'], + ); + $this->fail('Modifying a non-existent package should raise an error.'); + } + catch (AssertionFailedError $e) { + $this->assertStringContainsString("Failed asserting that an array has the key 'the-org/the-package'.", $e->getMessage()); + } + // We should not be able to modify a non-existent package. try { $this->modifyPackage($this->dir, 'junk/drawer', ['type' => 'library']); @@ -120,9 +160,8 @@ class FixtureUtilityTraitTest extends PackageManagerKernelTestBase { 'name' => 'my/other-package', 'type' => 'library', ]); - $this->modifyPackage($this->dir, 'my/other-package', ['dev_requirement' => TRUE]); - $expected_packages = [ + $install_json_expected_packages = [ 'my/package' => [ 'name' => 'my/package', 'type' => 'metapackage', @@ -130,25 +169,23 @@ class FixtureUtilityTraitTest extends PackageManagerKernelTestBase { 'my/dev-package' => [ 'name' => 'my/dev-package', 'version' => '3.2.1', - 'dev_requirement' => TRUE, - 'install_path' => '../relative/path', ], 'my/other-package' => [ 'name' => 'my/other-package', 'type' => 'library', - 'dev_requirement' => TRUE, ], ]; - + $installed_php_expected_packages = $install_json_expected_packages; + $installed_php_expected_packages['my/dev-package']['install_path'] = "$this->dir/vendor/composer/../relative/path"; [$installed_json, $installed_php] = $this->getData(); - $installed_json['packages'] = array_intersect_key($installed_json['packages'], $expected_packages); - $this->assertSame($expected_packages, $installed_json['packages']); + $installed_json['packages'] = array_intersect_key($installed_json['packages'], $install_json_expected_packages); + $this->assertSame($install_json_expected_packages, $installed_json['packages']); $this->assertContains('my/dev-package', $installed_json['dev-package-names']); - $this->assertContains('my/other-package', $installed_json['dev-package-names']); + $this->assertNotContains('my/other-package', $installed_json['dev-package-names']); $this->assertNotContains('my/package', $installed_json['dev-package-names']); + $installed_php_expected_packages = ['drupal/core' => ['name' => 'drupal/core']] + $installed_php_expected_packages; // @see ::testAddPackage() - $expected_packages['my/dev-package']['install_path'] = "$this->dir/vendor/composer/../relative/path"; - $this->assertSame($expected_packages, $installed_php); + $this->assertSame($installed_php_expected_packages, $installed_php); } /** @@ -168,9 +205,10 @@ class FixtureUtilityTraitTest extends PackageManagerKernelTestBase { $this->removePackage($this->dir, 'my/dev-package'); foreach (['json', 'php'] as $extension) { - $contents = file_get_contents("$this->dir/vendor/composer/installed.$extension"); - $this->assertStringNotContainsString('my/package', $contents); - $this->assertStringNotContainsString('my/dev-package', $contents); + $file = "$this->dir/vendor/composer/installed.$extension"; + $contents = file_get_contents($file); + $this->assertStringNotContainsString('my/package', $contents, "'my/package' not found in $file"); + $this->assertStringNotContainsString('my/dev-package', $contents, "'my/dev-package' not found in $file"); } } diff --git a/package_manager/tests/src/Traits/FixtureUtilityTrait.php b/package_manager/tests/src/Traits/FixtureUtilityTrait.php index 18a1806185..1740dd1e69 100644 --- a/package_manager/tests/src/Traits/FixtureUtilityTrait.php +++ b/package_manager/tests/src/Traits/FixtureUtilityTrait.php @@ -93,10 +93,12 @@ trait FixtureUtilityTrait { * @param array $package * The package info that should be added to installed.json and * installed.php. Must include a `name` key. + * @param bool $is_dev_requirement + * Whether or not the package is a development requirement. */ - protected function addPackage(string $dir, array $package): void { + protected function addPackage(string $dir, array $package, bool $is_dev_requirement = FALSE): void { $this->assertArrayHasKey('name', $package); - $this->setPackage($dir, $package['name'], $package, FALSE); + $this->setPackage($dir, $package['name'], $package, FALSE, $is_dev_requirement); } /** @@ -148,8 +150,11 @@ trait FixtureUtilityTrait { * 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 $dir, string $name, ?array $package, bool $should_exist): void { + private function setPackage(string $dir, string $name, ?array $package, bool $should_exist, ?bool $is_dev_requirement = NULL): void { + $this->assertNotTrue($should_exist && isset($is_dev_requirement), 'Changing an existing project to a dev requirement is not supported'); $dir .= '/vendor/composer'; $file = $dir . '/installed.json'; @@ -173,29 +178,30 @@ trait FixtureUtilityTrait { : "Expected package '$name' to not be installed, but it was."; $this->assertSame($should_exist, isset($position), $message); + if ($package) { + $package = ['name' => $name] + $package; + $install_json_package = array_diff_key($package, array_flip(['install_path'])); + } + if (isset($position)) { // If we're going to be updating the package data, merge the incoming data // into what we already have. if ($package) { - $package = NestedArray::mergeDeep($data['packages'][$position], $package); + $install_json_package = NestedArray::mergeDeep($data['packages'][$position], $install_json_package); } // Remove the existing package; the array will be re-keyed by // array_splice(). 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 ($package) { - // If an install path was provided, ensure it's relative. - if (array_key_exists('install_path', $package)) { - $this->assertStringStartsWith('../', $package['install_path']); - } - $package['name'] = $name; - $data['packages'][] = $package; + if (isset($package)) { + $data['packages'][] = $install_json_package; - if (!empty($package['dev_requirement'])) { + if ($is_dev_requirement || !empty($is_existing_dev_package)) { $data['dev-package-names'][] = $name; } } @@ -205,17 +211,37 @@ trait FixtureUtilityTrait { $this->assertFileIsWritable($file); $data = require $file; - unset($data['versions'][$name]); - // The installation paths in $data will have been interpreted by the PHP - // runtime, so make them all relative again by stripping $dir out. - array_walk($data['versions'], function (array &$package) use ($dir): void { + + // Ensure that we actually expect to find the package already installed (or + // not). + if ($should_exist) { + $this->assertArrayHasKey($name, $data['versions']); + } + else { + $this->assertArrayNotHasKey($name, $data['versions']); + } + if ($package) { + // If an install path was provided, ensure it's relative. if (array_key_exists('install_path', $package)) { - $package['install_path'] = str_replace("$dir/", '', $package['install_path']); + $this->assertStringStartsWith('../', $package['install_path']); } - }); - if ($package) { - $data['versions'][$name] = $package; + $install_php_package = $should_exist ? + NestedArray::mergeDeep($data['versions'][$name], $package) : + $package; + + // The installation paths in $data will have been interpreted by the PHP + // runtime, so make them all relative again by stripping $dir out. + array_walk($data['versions'], function (array &$install_php_package) use ($dir): void { + if (array_key_exists('install_path', $install_php_package)) { + $install_php_package['install_path'] = str_replace("$dir/", '', $install_php_package['install_path']); + } + }); + $data['versions'][$name] = $install_php_package; } + else { + unset($data['versions'][$name]); + } + $data = var_export($data, TRUE); $data = str_replace("'install_path' => '../", "'install_path' => __DIR__ . '/../", $data); file_put_contents($file, "<?php\nreturn $data;"); diff --git a/tests/src/Kernel/StatusCheck/StagedProjectsValidatorTest.php b/tests/src/Kernel/StatusCheck/StagedProjectsValidatorTest.php index 3c1fb6afb0..5cb6ad6d7b 100644 --- a/tests/src/Kernel/StatusCheck/StagedProjectsValidatorTest.php +++ b/tests/src/Kernel/StatusCheck/StagedProjectsValidatorTest.php @@ -84,18 +84,24 @@ class StagedProjectsValidatorTest extends AutomaticUpdatesKernelTestBase { 'version' => '1.3.1', 'type' => 'library', ]); - $this->addPackage($active_dir, [ - 'name' => 'drupal/dev-test_module', - 'version' => '1.3.0', - 'type' => 'drupal_module', - 'dev_requirement' => TRUE, - ]); - $this->addPackage($active_dir, [ - 'name' => 'other/dev-removed', - 'version' => '1.3.1', - 'type' => 'library', - 'dev_requirement' => TRUE, - ]); + $this->addPackage( + $active_dir, + [ + 'name' => 'drupal/dev-test_module', + 'version' => '1.3.0', + 'type' => 'drupal_module', + ], + TRUE + ); + $this->addPackage( + $active_dir, + [ + 'name' => 'other/dev-removed', + 'version' => '1.3.1', + 'type' => 'library', + ], + TRUE + ); $updater = $this->container->get('automatic_updates.updater'); $updater->begin(['drupal' => '9.8.1']); @@ -108,13 +114,16 @@ class StagedProjectsValidatorTest extends AutomaticUpdatesKernelTestBase { 'type' => 'drupal-module', 'install_path' => '../../modules/test_module2', ]); - $this->addPackage($stage_dir, [ - 'name' => 'drupal/dev-test_module2', - 'version' => '1.3.1', - 'type' => 'drupal-custom-module', - 'dev_requirement' => TRUE, - 'install_path' => '../../modules/dev-test_module2', - ]); + $this->addPackage( + $stage_dir, + [ + 'name' => 'drupal/dev-test_module2', + 'version' => '1.3.1', + 'type' => 'drupal-custom-module', + 'install_path' => '../../modules/dev-test_module2', + ], + TRUE + ); // The validator shouldn't complain about these packages being added or // removed, since it only cares about Drupal modules and themes. $this->addPackage($stage_dir, [ @@ -123,13 +132,16 @@ class StagedProjectsValidatorTest extends AutomaticUpdatesKernelTestBase { 'type' => 'library', 'install_path' => '../other/new_project', ]); - $this->addPackage($stage_dir, [ - 'name' => 'other/dev-new_project', - 'version' => '1.3.1', - 'type' => 'library', - 'dev_requirement' => TRUE, - 'install_path' => '../other/dev-new_project', - ]); + $this->addPackage( + $stage_dir, + [ + 'name' => 'other/dev-new_project', + 'version' => '1.3.1', + 'type' => 'library', + 'install_path' => '../other/dev-new_project', + ], + TRUE + ); $this->removePackage($stage_dir, 'other/removed'); $this->removePackage($stage_dir, 'other/dev-removed'); @@ -168,24 +180,33 @@ class StagedProjectsValidatorTest extends AutomaticUpdatesKernelTestBase { 'version' => '1.3.1', 'type' => 'library', ]); - $this->addPackage($active_dir, [ - 'name' => 'drupal/dev-test_theme', - 'version' => '1.3.0', - 'type' => 'drupal-custom-theme', - 'dev_requirement' => TRUE, - ]); - $this->addPackage($active_dir, [ - 'name' => 'drupal/dev-test_module2', - 'version' => '1.3.1', - 'type' => 'drupal-module', - 'dev_requirement' => TRUE, - ]); - $this->addPackage($active_dir, [ - 'name' => 'other/dev-removed', - 'version' => '1.3.1', - 'type' => 'library', - 'dev_requirement' => TRUE, - ]); + $this->addPackage( + $active_dir, + [ + 'name' => 'drupal/dev-test_theme', + 'version' => '1.3.0', + 'type' => 'drupal-custom-theme', + ], + TRUE + ); + $this->addPackage( + $active_dir, + [ + 'name' => 'drupal/dev-test_module2', + 'version' => '1.3.1', + 'type' => 'drupal-module', + ], + TRUE + ); + $this->addPackage( + $active_dir, + [ + 'name' => 'other/dev-removed', + 'version' => '1.3.1', + 'type' => 'library', + ], + TRUE + ); $updater = $this->container->get('automatic_updates.updater'); $updater->begin(['drupal' => '9.8.1']); @@ -229,18 +250,24 @@ class StagedProjectsValidatorTest extends AutomaticUpdatesKernelTestBase { 'version' => '1.3.1', 'type' => 'library', ]); - $this->addPackage($active_dir, [ - 'name' => 'drupal/dev-test_module', - 'version' => '1.3.0', - 'type' => 'drupal-module', - 'dev_requirement' => TRUE, - ]); - $this->addPackage($active_dir, [ - 'name' => 'other/dev-changed', - 'version' => '1.3.1', - 'type' => 'library', - 'dev_requirement' => TRUE, - ]); + $this->addPackage( + $active_dir, + [ + 'name' => 'drupal/dev-test_module', + 'version' => '1.3.0', + 'type' => 'drupal-module', + ], + TRUE + ); + $this->addPackage( + $active_dir, + [ + 'name' => 'other/dev-changed', + 'version' => '1.3.1', + 'type' => 'library', + ], + TRUE + ); $updater = $this->container->get('automatic_updates.updater'); $updater->begin(['drupal' => '9.8.1']); @@ -297,24 +324,32 @@ class StagedProjectsValidatorTest extends AutomaticUpdatesKernelTestBase { 'version' => '1.3.1', 'type' => 'library', ]); - $this->addPackage($active_dir, [ - 'name' => 'drupal/dev-test_module', - 'version' => '1.3.0', - 'type' => 'drupal-module', - 'dev_requirement' => TRUE, - ]); - $this->addPackage($active_dir, [ - 'name' => 'other/dev-removed', - 'version' => '1.3.1', - 'type' => 'library', - 'dev_requirement' => TRUE, - ]); - $this->addPackage($active_dir, [ - 'name' => 'other/dev-changed', - 'version' => '1.3.1', - 'type' => 'library', - 'dev_requirement' => TRUE, - ]); + $this->addPackage( + $active_dir, [ + 'name' => 'drupal/dev-test_module', + 'version' => '1.3.0', + 'type' => 'drupal-module', + ], + TRUE + ); + $this->addPackage( + $active_dir, + [ + 'name' => 'other/dev-removed', + 'version' => '1.3.1', + 'type' => 'library', + ], + TRUE + ); + $this->addPackage( + $active_dir, + [ + 'name' => 'other/dev-changed', + 'version' => '1.3.1', + 'type' => 'library', + ], + TRUE + ); $updater = $this->container->get('automatic_updates.updater'); $updater->begin(['drupal' => '9.8.1']); @@ -332,13 +367,16 @@ class StagedProjectsValidatorTest extends AutomaticUpdatesKernelTestBase { 'type' => 'library', 'install_path' => '../other/new_project', ]); - $this->addPackage($stage_dir, [ - 'name' => 'other/dev-new_project', - 'version' => '1.3.1', - 'type' => 'library', - 'dev_requirement' => TRUE, - 'install_path' => '../other/dev-new_project', - ]); + $this->addPackage( + $stage_dir, + [ + 'name' => 'other/dev-new_project', + 'version' => '1.3.1', + 'type' => 'library', + 'install_path' => '../other/dev-new_project', + ], + TRUE + ); $this->modifyPackage($stage_dir, 'other/changed', [ 'version' => '1.3.2', ]); -- GitLab