Issue #3318927 by tedbow: FixtureUtilityTrait incorrectly assumes that the...

Issue #3318927 by tedbow: FixtureUtilityTrait incorrectly assumes that the package keys are the same in install.json and installed.php

package_manager/tests/fixtures/FixtureUtilityTraitTest/existing_correct_fixture/vendor/composer/installed.json

+{
+    "packages": [
+        {
+            "name": "the-org/the-package",
+            "version": "9.8.0"
+        }
+    ],
+    "dev-package-names": []
+}
\ No newline at end of file

package_manager/tests/fixtures/FixtureUtilityTraitTest/existing_correct_fixture/vendor/composer/installed.php

+<?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',
+    ],
+  ],
+];

package_manager/tests/fixtures/FixtureUtilityTraitTest/missing_installed_php/vendor/composer/installed.json

+{
+    "packages": [
+        {
+            "name": "the-org/the-package",
+            "version": "9.8.0"
+        }
+    ],
+    "dev-package-names": []
+}
\ No newline at end of file

package_manager/tests/fixtures/FixtureUtilityTraitTest/missing_installed_php/vendor/composer/installed.php

+<?php
+
+/**
+ * @file
+ * Fixture for  \Drupal\Tests\package_manager\Kernel\FixtureUtilityTraitTest::testModifyPackage().
+ */
+
+// Composer Utility needs the versions key to be present.
+return [
+  'versions' => [],
+];

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',
+    ],
+  ],
 ];

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");
     }
   }
 

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;");

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',
     ]);