Issue #3265874 by kunal.sachdev, phenaproxima: Do not allow apply if active...

Issue #3265874 by kunal.sachdev, phenaproxima: Do not allow apply if active and staged composer locks are identical

package_manager/src/Validator/LockFileValidator.php

@@ -57,14 +57,17 @@ class LockFileValidator implements PreOperationStageValidatorInterface {
   }
 
   /**
-   * Returns the current hash of the active directory's lock file.
+   * Returns the current hash of the given directory's lock file.
+   *
+   * @param string $directory
+   *   Path of a directory containing a composer.lock file.
    *
    * @return string|false
-   *   The hash of the active directory's lock file, or FALSE if the lock file
+   *   The hash of the given directory's lock file, or FALSE if the lock file
    *   does not exist.
    */
-  protected function getHash() {
-    $file = $this->pathLocator->getProjectRoot() . DIRECTORY_SEPARATOR . 'composer.lock';
+  protected function getLockFileHash(string $directory) {
+    $file = $directory . DIRECTORY_SEPARATOR . 'composer.lock';
     // We want to directly hash the lock file itself, rather than look at its
     // content-hash value, which is actually a hash of the relevant parts of
     // composer.json. We're trying to verify that the actual installed packages
@@ -81,7 +84,7 @@ class LockFileValidator implements PreOperationStageValidatorInterface {
    * Stores the current lock file hash.
    */
   public function storeHash(PreCreateEvent $event): void {
-    $hash = $this->getHash();
+    $hash = $this->getLockFileHash($this->pathLocator->getProjectRoot());
     if ($hash) {
       $this->state->set(static::STATE_KEY, $hash);
     }
@@ -97,8 +100,8 @@ class LockFileValidator implements PreOperationStageValidatorInterface {
    */
   public function validateStagePreOperation(PreOperationStageEvent $event): void {
     // Ensure we can get a current hash of the lock file.
-    $hash = $this->getHash();
-    if (empty($hash)) {
+    $active_hash = $this->getLockFileHash($this->pathLocator->getProjectRoot());
+    if (empty($active_hash)) {
       $error = $this->t('Could not hash the active lock file.');
     }
 
@@ -109,10 +112,19 @@ class LockFileValidator implements PreOperationStageValidatorInterface {
     }
 
     // If we have both hashes, ensure they match.
-    if ($hash && $stored_hash && !hash_equals($stored_hash, $hash)) {
+    if ($active_hash && $stored_hash && !hash_equals($stored_hash, $active_hash)) {
       $error = $this->t('Stored lock file hash does not match the active lock file.');
     }
 
+    // Don't allow staged changes to be applied if the staged lock file has no
+    // apparent changes.
+    if (empty($error) && $event instanceof PreApplyEvent) {
+      $stage_hash = $this->getLockFileHash($event->getStage()->getStageDirectory());
+      if ($stage_hash && hash_equals($active_hash, $stage_hash)) {
+        $error = $this->t('There are no pending Composer operations.');
+      }
+    }
+
     // @todo Let the validation result carry all the relevant messages in
     //   https://www.drupal.org/i/3247479.
     if (isset($error)) {

package_manager/tests/src/Kernel/ExcludedPathsTest.php

@@ -14,6 +14,17 @@ use Drupal\package_manager\EventSubscriber\ExcludedPathsSubscriber;
  */
 class ExcludedPathsTest extends PackageManagerKernelTestBase {
 
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp(): void {
+    // In this test, we want to disable the lock file validator because, even
+    // though both the active and stage directories will have a valid lock file,
+    // this validator will complain because they don't differ at all.
+    $this->disableValidators[] = 'package_manager.validator.lock_file';
+    parent::setUp();
+  }
+
   /**
    * {@inheritdoc}
    */

package_manager/tests/src/Kernel/LockFileValidatorTest.php

@@ -2,6 +2,7 @@
 
 namespace Drupal\Tests\package_manager\Kernel;
 
+use Drupal\package_manager\Event\PostRequireEvent;
 use Drupal\package_manager\Event\PreApplyEvent;
 use Drupal\package_manager\Event\PreCreateEvent;
 use Drupal\package_manager\Event\PreRequireEvent;
@@ -119,6 +120,21 @@ class LockFileValidatorTest extends PackageManagerKernelTestBase {
     $this->assertResults([$result], $event_class);
   }
 
+  /**
+   * Tests validation when the staged and active lock files are identical.
+   */
+  public function testApplyWithNoChange(): void {
+    $this->addListener(PostRequireEvent::class, function (PostRequireEvent $event) {
+      $stage_dir = $event->getStage()->getStageDirectory();
+      mkdir($stage_dir);
+      copy("$this->activeDir/composer.lock", "$stage_dir/composer.lock");
+    });
+    $result = ValidationResult::createError([
+      'There are no pending Composer operations.',
+    ]);
+    $this->assertResults([$result], PreApplyEvent::class);
+  }
+
   /**
    * Data provider for test methods that validate the staging area.
    *

tests/src/Functional/AutomaticUpdatesFunctionalTestBase.php

@@ -40,6 +40,10 @@ abstract class AutomaticUpdatesFunctionalTestBase extends BrowserTestBase {
     // package_manager_bypass is disabling those operations.
     'automatic_updates.composer_executable_validator',
     'package_manager.validator.composer_executable',
+    // Disable the lock file validator, because it may cause the tests to fail
+    // if either the active and stage directories don't have a composer.lock
+    // file, which is the case with some of our fixtures.
+    'package_manager.validator.lock_file',
   ];
 
   /**

tests/src/Kernel/ReadinessValidation/StagedDatabaseUpdateValidatorTest.php

@@ -30,6 +30,10 @@ class StagedDatabaseUpdateValidatorTest extends AutomaticUpdatesKernelTestBase {
    * {@inheritdoc}
    */
   protected function setUp(): void {
+    // In this test, we want to disable the lock file validator because, even
+    // though both the active and stage directories will have a valid lock file,
+    // this validator will complain because they don't differ at all.
+    $this->disableValidators[] = 'package_manager.validator.lock_file';
     parent::setUp();
 
     TestStage::$stagingRoot = $this->vfsRoot->url();