From a1a42c0911338bad8296570e5d834ec1b90720f9 Mon Sep 17 00:00:00 2001
From: omkar podey <58183-omkar.podey@users.noreply.drupalcode.org>
Date: Fri, 30 Jun 2023 17:59:15 +0000
Subject: [PATCH] Issue #3352846 by phenaproxima, omkar.podey, tedbow: Make the
LockFileValidator messages more helpful
---
.../src/Validator/LockFileValidator.php | 82 ++++++++++++-------
.../src/Kernel/LockFileValidatorTest.php | 16 ++--
.../Kernel/PackageManagerKernelTestBase.php | 5 +-
.../tests/src/Traits/ValidationTestTrait.php | 3 +-
4 files changed, 71 insertions(+), 35 deletions(-)
diff --git a/package_manager/src/Validator/LockFileValidator.php b/package_manager/src/Validator/LockFileValidator.php
index 6662ab2476..b7b854b35c 100644
--- a/package_manager/src/Validator/LockFileValidator.php
+++ b/package_manager/src/Validator/LockFileValidator.php
@@ -48,23 +48,26 @@ final class LockFileValidator implements EventSubscriberInterface {
) {}
/**
- * Returns the current hash of the given directory's lock file.
+ * Returns the SHA-256 hash of a file.
*
- * @param string $directory
- * Path of a directory containing a composer.lock file.
+ * This method is a thin wrapper around hash_file() to facilitate testing. On
+ * failure, hash_file() emits a warning but doesn't throw an exception. In
+ * tests, however, PHPUnit converts warnings to exceptions, so we need to
+ * catch those and convert them to the value hash_file() will actually return
+ * on error, which is FALSE. We could also just call `hash_file` directly and
+ * use @ to suppress warnings, but those would be unclear and likely to be
+ * accidentally removed later.
+ *
+ * @param string $path
+ * Path of the file to hash.
*
* @return string|false
- * The hash of the given directory's lock file, or FALSE if the lock file
- * does not exist.
+ * The hash of the given file, or FALSE if the file doesn't exist or cannot
+ * be hashed.
*/
- 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
- // have not changed; we don't care about the constraints in composer.json.
+ private function getHash(string $path) {
try {
- return hash_file('sha256', $file);
+ return hash_file('sha256', $path);
}
catch (\Throwable) {
return FALSE;
@@ -72,55 +75,78 @@ final class LockFileValidator implements EventSubscriberInterface {
}
/**
- * Stores the current lock file hash.
+ * Stores the SHA-256 hash of the active lock file.
+ *
+ * We store the hash of the lock file itself, rather than its content-hash
+ * value, which is actually a hash of certain parts of composer.json. Our aim
+ * is to verify that the actual installed packages have not changed
+ * unexpectedly; we don't care about the contents of composer.json.
+ *
+ * @param \Drupal\package_manager\Event\PreCreateEvent $event
+ * The event being handled.
*/
public function storeHash(PreCreateEvent $event): void {
- $hash = $this->getLockFileHash($this->pathLocator->getProjectRoot());
+ $active_lock_file_path = $this->pathLocator->getProjectRoot() . DIRECTORY_SEPARATOR . 'composer.lock';
+ $hash = $this->getHash($active_lock_file_path);
if ($hash) {
$this->state->set(static::STATE_KEY, $hash);
}
else {
$event->addError([
- // @todo Reword in https://www.drupal.org/project/automatic_updates/issues/3352846
- $this->t('The active lock file does not exist.'),
+ $this->t('The active lock file (@file) does not exist.', [
+ '@file' => $active_lock_file_path,
+ ]),
]);
}
}
/**
* Checks that the active lock file is unchanged during stage operations.
+ *
+ * @param \Drupal\package_manager\Event\PreOperationStageEvent $event
+ * The event being handled.
*/
public function validate(PreOperationStageEvent $event): void {
+ $stage = $event->stage;
+
// Early return if the stage is not already created.
- if ($event instanceof StatusCheckEvent && $event->stage->isAvailable()) {
+ if ($event instanceof StatusCheckEvent && $stage->isAvailable()) {
return;
}
$messages = [];
// Ensure we can get a current hash of the lock file.
- $active_hash = $this->getLockFileHash($this->pathLocator->getProjectRoot());
- if (empty($active_hash)) {
- // @todo Reword in https://www.drupal.org/project/automatic_updates/issues/3352846
- $messages[] = $this->t('The active lock file does not exist.');
+ $active_lock_file_path = $this->pathLocator->getProjectRoot() . DIRECTORY_SEPARATOR . 'composer.lock';
+ $active_lock_file_hash = $this->getHash($active_lock_file_path);
+ if (empty($active_lock_file_hash)) {
+ $messages[] = $this->t('The active lock file (@file) does not exist.', [
+ '@file' => $active_lock_file_path,
+ ]);
}
// Ensure we also have a stored hash of the lock file.
- $stored_hash = $this->state->get(static::STATE_KEY);
- if (empty($stored_hash)) {
+ $active_lock_file_stored_hash = $this->state->get(static::STATE_KEY);
+ if (empty($active_lock_file_stored_hash)) {
throw new \LogicException('Stored hash key deleted.');
}
// If we have both hashes, ensure they match.
- if ($active_hash && $stored_hash && !hash_equals($stored_hash, $active_hash)) {
- $messages[] = $this->t('Unexpected changes were detected in composer.lock, which indicates that other Composer operations were performed since this Package Manager operation started. This can put the code base into an unreliable state and therefore is not allowed.');
+ if ($active_lock_file_hash && $active_lock_file_stored_hash && !hash_equals($active_lock_file_stored_hash, $active_lock_file_hash)) {
+ $messages[] = $this->t('Unexpected changes were detected in the active lock file (@file), which indicates that other Composer operations were performed since this Package Manager operation started. This can put the code base into an unreliable state and therefore is not allowed.', [
+ '@file' => $active_lock_file_path,
+ ]);
}
// Don't allow staged changes to be applied if the staged lock file has no
// apparent changes.
if (empty($messages) && $event instanceof PreApplyEvent) {
- $stage_hash = $this->getLockFileHash($event->stage->getStageDirectory());
- if ($stage_hash && hash_equals($active_hash, $stage_hash)) {
- $messages[] = $this->t('There are no pending Composer operations.');
+ $staged_lock_file_path = $stage->getStageDirectory() . DIRECTORY_SEPARATOR . 'composer.lock';
+ $staged_lock_file_hash = $this->getHash($staged_lock_file_path);
+ if ($staged_lock_file_hash && hash_equals($active_lock_file_hash, $staged_lock_file_hash)) {
+ $messages[] = $this->t('There appear to be no pending Composer operations because the active lock file (@active_file) and the staged lock file (@staged_file) are identical.', [
+ '@active_file' => $active_lock_file_path,
+ '@staged_file' => $staged_lock_file_path,
+ ]);
}
}
diff --git a/package_manager/tests/src/Kernel/LockFileValidatorTest.php b/package_manager/tests/src/Kernel/LockFileValidatorTest.php
index 2cc20df485..a2533dbd19 100644
--- a/package_manager/tests/src/Kernel/LockFileValidatorTest.php
+++ b/package_manager/tests/src/Kernel/LockFileValidatorTest.php
@@ -69,8 +69,11 @@ class LockFileValidatorTest extends PackageManagerKernelTestBase {
*/
public function testCreateWithNoLock(): void {
unlink($this->activeDir . '/composer.lock');
-
- $no_lock = ValidationResult::createError([t('The active lock file does not exist.')]);
+ $project_root = $this->container->get(PathLocator::class)->getProjectRoot();
+ $lock_file_path = $project_root . DIRECTORY_SEPARATOR . 'composer.lock';
+ $no_lock = ValidationResult::createError([
+ t('The active lock file (@file) does not exist.', ['@file' => $lock_file_path]),
+ ]);
$stage = $this->assertResults([$no_lock], PreCreateEvent::class);
// The stage was not created successfully, so the status check should be
// clear.
@@ -108,7 +111,8 @@ class LockFileValidatorTest extends PackageManagerKernelTestBase {
file_put_contents($this->activeDir . '/composer.lock', json_encode($lock, JSON_THROW_ON_ERROR));
}, $event_class);
$result = ValidationResult::createError([
- t('Unexpected changes were detected in composer.lock, which indicates that other Composer operations were performed since this Package Manager operation started. This can put the code base into an unreliable state and therefore is not allowed.'),
+ t('Unexpected changes were detected in the active lock file (@file), which indicates that other Composer operations were performed since this Package Manager operation started. This can put the code base into an unreliable state and therefore is not allowed.',
+ ['@file' => $this->activeDir . '/composer.lock']),
], t('Problem detected in lock file during stage operations.'));
$stage = $this->assertResults([$result], $event_class);
// A status check should agree that there is an error here.
@@ -129,7 +133,9 @@ class LockFileValidatorTest extends PackageManagerKernelTestBase {
unlink($this->activeDir . '/composer.lock');
}, $event_class);
$result = ValidationResult::createError([
- t('The active lock file does not exist.'),
+ t('The active lock file (@file) does not exist.', [
+ '@file' => $this->activeDir . '/composer.lock',
+ ]),
], t('Problem detected in lock file during stage operations.'));
$stage = $this->assertResults([$result], $event_class);
// A status check should agree that there is an error here.
@@ -173,7 +179,7 @@ class LockFileValidatorTest extends PackageManagerKernelTestBase {
NoOpStager::setLockFileShouldChange(FALSE);
$result = ValidationResult::createError([
- t('There are no pending Composer operations.'),
+ t('There appear to be no pending Composer operations because the active lock file (<PROJECT_ROOT>/composer.lock) and the staged lock file (<STAGE_DIR>/composer.lock) are identical.'),
], t('Problem detected in lock file during stage operations.'));
$stage = $this->assertResults([$result], PreApplyEvent::class);
// A status check shouldn't produce raise any errors, because it's only
diff --git a/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php b/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php
index e3fec5d570..0dae050e06 100644
--- a/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php
+++ b/package_manager/tests/src/Kernel/PackageManagerKernelTestBase.php
@@ -435,7 +435,10 @@ abstract class PackageManagerKernelTestBase extends KernelTestBase {
protected function assertExpectedResultsFromException(array $expected_results, StageEventException $exception): void {
$event = $exception->event;
$this->assertInstanceOf(PreOperationStageEvent::class, $event);
- $this->assertValidationResultsEqual($expected_results, $event->getResults());
+
+ $stage = $event->stage;
+ $stage_dir = $stage->stageDirectoryExists() ? $stage->getStageDirectory() : NULL;
+ $this->assertValidationResultsEqual($expected_results, $event->getResults(), NULL, $stage_dir);
}
/**
diff --git a/package_manager/tests/src/Traits/ValidationTestTrait.php b/package_manager/tests/src/Traits/ValidationTestTrait.php
index 02228d0023..48fbde56ec 100644
--- a/package_manager/tests/src/Traits/ValidationTestTrait.php
+++ b/package_manager/tests/src/Traits/ValidationTestTrait.php
@@ -55,7 +55,8 @@ trait ValidationTestTrait {
* @param array $subject
* An array with arbitrary keys, and values potentially containing the
* placeholders <PROJECT_ROOT>, <VENDOR_DIR>, <STAGE_ROOT>, or
- * <STAGE_ROOT_PARENT>.
+ * <STAGE_ROOT_PARENT>. <STAGE_DIR> is the placeholder for $stage_dir, if
+ * passed.
* @param \Drupal\package_manager\PathLocator|null $path_locator
* (optional) The path locator (when this trait is used in unit tests).
* @param string|null $stage_dir
--
GitLab