From 6f67db84764f5c17fd54a6005ade48c6361c906b Mon Sep 17 00:00:00 2001
From: Wim Leers <44946-wimleers@users.noreply.drupalcode.org>
Date: Mon, 3 Apr 2023 16:31:28 +0000
Subject: [PATCH] Issue #3348441 by kunal.sachdev, phenaproxima, Wim Leers:
Many callers of ValidationResult::createError() violate its interface due to
lack of strict typing
---
package_manager/src/ValidationResult.php | 20 ++++++++++++++-----
.../Validator/EnvironmentSupportValidator.php | 7 ++++---
.../src/Validator/StagedDBUpdateValidator.php | 1 +
.../Validator/SupportedReleaseValidator.php | 3 +--
.../Kernel/ComposerPatchesValidatorTest.php | 2 +-
.../src/Kernel/DiskSpaceValidatorTest.php | 6 +++---
.../Kernel/DuplicateInfoFileValidatorTest.php | 16 +++++++--------
.../EnvironmentSupportValidatorTest.php | 6 +++---
.../Kernel/PendingUpdatesValidatorTest.php | 2 +-
.../Kernel/SupportedReleaseValidatorTest.php | 2 +-
.../WritableFileSystemValidatorTest.php | 4 ++--
.../tests/src/Unit/ValidationResultTest.php | 16 +++++++++++++++
.../ScaffoldFilePermissionsValidator.php | 2 +-
.../StagedDatabaseUpdateValidator.php | 1 +
.../StatusCheck/CronServerValidatorTest.php | 2 +-
.../ScaffoldFilePermissionsValidatorTest.php | 1 +
16 files changed, 60 insertions(+), 31 deletions(-)
diff --git a/package_manager/src/ValidationResult.php b/package_manager/src/ValidationResult.php
index d6e2a16b63..aafd42a239 100644
--- a/package_manager/src/ValidationResult.php
+++ b/package_manager/src/ValidationResult.php
@@ -4,6 +4,7 @@ declare(strict_types = 1);
namespace Drupal\package_manager;
+use Drupal\Component\Assertion\Inspector;
use Drupal\Core\StringTranslation\TranslatableMarkup;
use Drupal\system\SystemManager;
@@ -21,9 +22,18 @@ final class ValidationResult {
* @param \Drupal\Core\StringTranslation\TranslatableMarkup[]|string[] $messages
* The error messages.
* @param \Drupal\Core\StringTranslation\TranslatableMarkup|null $summary
- * The errors summary.
+ * A summary of the result messages.
+ * @param bool $assert_translatable
+ * Whether to assert the messages are translatable. Internal use only.
+ *
+ * @throws \InvalidArgumentException
+ * Thrown if $messages is empty, or if it has 2 or more items but $summary
+ * is NULL.
*/
- private function __construct(protected int $severity, protected array $messages, protected ?TranslatableMarkup $summary = NULL) {
+ private function __construct(protected int $severity, protected array $messages, protected ?TranslatableMarkup $summary, bool $assert_translatable) {
+ if ($assert_translatable) {
+ assert(Inspector::assertAll(fn ($message) => $message instanceof TranslatableMarkup, $messages));
+ }
if (empty($messages)) {
throw new \InvalidArgumentException('At least one message is required.');
}
@@ -43,7 +53,7 @@ final class ValidationResult {
* @return static
*/
public static function createErrorFromThrowable(\Throwable $throwable, ?TranslatableMarkup $summary = NULL): self {
- return new static(SystemManager::REQUIREMENT_ERROR, [$throwable->getMessage()], $summary);
+ return new static(SystemManager::REQUIREMENT_ERROR, [$throwable->getMessage()], $summary, FALSE);
}
/**
@@ -57,7 +67,7 @@ final class ValidationResult {
* @return static
*/
public static function createError(array $messages, ?TranslatableMarkup $summary = NULL): self {
- return new static(SystemManager::REQUIREMENT_ERROR, $messages, $summary);
+ return new static(SystemManager::REQUIREMENT_ERROR, $messages, $summary, TRUE);
}
/**
@@ -71,7 +81,7 @@ final class ValidationResult {
* @return static
*/
public static function createWarning(array $messages, ?TranslatableMarkup $summary = NULL): self {
- return new static(SystemManager::REQUIREMENT_WARNING, $messages, $summary);
+ return new static(SystemManager::REQUIREMENT_WARNING, $messages, $summary, TRUE);
}
/**
diff --git a/package_manager/src/Validator/EnvironmentSupportValidator.php b/package_manager/src/Validator/EnvironmentSupportValidator.php
index bef02f2e44..3cd6cd5bfb 100644
--- a/package_manager/src/Validator/EnvironmentSupportValidator.php
+++ b/package_manager/src/Validator/EnvironmentSupportValidator.php
@@ -4,7 +4,6 @@ declare(strict_types = 1);
namespace Drupal\package_manager\Validator;
-use Drupal\Core\Link;
use Drupal\Core\StringTranslation\StringTranslationTrait;
use Drupal\package_manager\Event\PreOperationStageEvent;
use Drupal\Core\Url;
@@ -49,8 +48,10 @@ final class EnvironmentSupportValidator implements EventSubscriberInterface {
// If the URL is not parseable, catch the exception that Url::fromUri()
// would generate.
try {
- $message = Link::fromTextAndUrl($message, Url::fromUri($help_url))
- ->toString();
+ $message = $this->t('<a href=":url">@message</a>', [
+ ':url' => Url::fromUri($help_url)->toString(),
+ '@message' => $message,
+ ]);
}
catch (\InvalidArgumentException) {
// No need to do anything here. The message just won't be a link.
diff --git a/package_manager/src/Validator/StagedDBUpdateValidator.php b/package_manager/src/Validator/StagedDBUpdateValidator.php
index b6f37fa965..79545027c3 100644
--- a/package_manager/src/Validator/StagedDBUpdateValidator.php
+++ b/package_manager/src/Validator/StagedDBUpdateValidator.php
@@ -57,6 +57,7 @@ class StagedDBUpdateValidator implements EventSubscriberInterface {
$extensions_with_updates = $this->getExtensionsWithDatabaseUpdates($stage_dir);
if ($extensions_with_updates) {
+ $extensions_with_updates = array_map($this->t(...), $extensions_with_updates);
$event->addWarning($extensions_with_updates, $this->t('Possible database updates have been detected in the following extensions.'));
}
}
diff --git a/package_manager/src/Validator/SupportedReleaseValidator.php b/package_manager/src/Validator/SupportedReleaseValidator.php
index c821081c23..bf9dddc6bd 100644
--- a/package_manager/src/Validator/SupportedReleaseValidator.php
+++ b/package_manager/src/Validator/SupportedReleaseValidator.php
@@ -4,7 +4,6 @@ declare(strict_types = 1);
namespace Drupal\package_manager\Validator;
-use Drupal\Component\Render\FormattableMarkup;
use Drupal\package_manager\ComposerInspector;
use Drupal\package_manager\PathLocator;
use Drupal\package_manager\ProjectInfo;
@@ -104,7 +103,7 @@ final class SupportedReleaseValidator implements EventSubscriberInterface {
}
$semantic_version = $staged_package->version;
if (!$this->isSupportedRelease($project_name, $semantic_version)) {
- $unsupported_packages[] = new FormattableMarkup('@project_name (@package_name) @version', [
+ $unsupported_packages[] = $this->t('@project_name (@package_name) @version', [
'@project_name' => $project_name,
'@package_name' => $package_name,
'@version' => $semantic_version,
diff --git a/package_manager/tests/src/Kernel/ComposerPatchesValidatorTest.php b/package_manager/tests/src/Kernel/ComposerPatchesValidatorTest.php
index ab7fbd362e..9a842d67b6 100644
--- a/package_manager/tests/src/Kernel/ComposerPatchesValidatorTest.php
+++ b/package_manager/tests/src/Kernel/ComposerPatchesValidatorTest.php
@@ -281,7 +281,7 @@ class ComposerPatchesValidatorTest extends PackageManagerKernelTestBase {
->toString();
// Reformat the provided results so that they all have the link to the
// online documentation appended to them.
- $messages[$message_index] = $message . ' See <a href="' . $url . '">the help page</a> for information on how to resolve the problem.';
+ $messages[$message_index] = t('@message See <a href=":url">the help page</a> for information on how to resolve the problem.', ['@message' => $message, ':url' => $url]);
}
}
$expected_results[$result_index] = ValidationResult::createError($messages, $result->getSummary());
diff --git a/package_manager/tests/src/Kernel/DiskSpaceValidatorTest.php b/package_manager/tests/src/Kernel/DiskSpaceValidatorTest.php
index bf06f77ddd..a541b615e4 100644
--- a/package_manager/tests/src/Kernel/DiskSpaceValidatorTest.php
+++ b/package_manager/tests/src/Kernel/DiskSpaceValidatorTest.php
@@ -28,9 +28,9 @@ class DiskSpaceValidatorTest extends PackageManagerKernelTestBase {
$root = '<PROJECT_ROOT>';
$vendor = '<VENDOR_DIR>';
- $root_insufficient = "Drupal root filesystem \"$root\" has insufficient space. There must be at least 1024 megabytes free.";
- $vendor_insufficient = "Vendor filesystem \"$vendor\" has insufficient space. There must be at least 1024 megabytes free.";
- $temp_insufficient = 'Directory "temp" has insufficient space. There must be at least 1024 megabytes free.';
+ $root_insufficient = t('Drupal root filesystem "<PROJECT_ROOT>" has insufficient space. There must be at least 1024 megabytes free.');
+ $vendor_insufficient = t('Vendor filesystem "<VENDOR_DIR>" has insufficient space. There must be at least 1024 megabytes free.');
+ $temp_insufficient = t('Directory "temp" has insufficient space. There must be at least 1024 megabytes free.');
$summary = t("There is not enough disk space to create a stage directory.");
return [
diff --git a/package_manager/tests/src/Kernel/DuplicateInfoFileValidatorTest.php b/package_manager/tests/src/Kernel/DuplicateInfoFileValidatorTest.php
index c17377524a..43da1afd87 100644
--- a/package_manager/tests/src/Kernel/DuplicateInfoFileValidatorTest.php
+++ b/package_manager/tests/src/Kernel/DuplicateInfoFileValidatorTest.php
@@ -34,7 +34,7 @@ class DuplicateInfoFileValidatorTest extends PackageManagerKernelTestBase {
],
[
ValidationResult::createError([
- 'The stage directory has 2 instances of module.info.yml as compared to 1 in the active directory. This likely indicates that a duplicate extension was installed.',
+ t('The stage directory has 2 instances of module.info.yml as compared to 1 in the active directory. This likely indicates that a duplicate extension was installed.'),
]),
],
],
@@ -98,7 +98,7 @@ class DuplicateInfoFileValidatorTest extends PackageManagerKernelTestBase {
],
[
ValidationResult::createError([
- 'The stage directory has 2 instances of module.info.yml. This likely indicates that a duplicate extension was installed.',
+ t('The stage directory has 2 instances of module.info.yml. This likely indicates that a duplicate extension was installed.'),
]),
],
],
@@ -137,10 +137,10 @@ class DuplicateInfoFileValidatorTest extends PackageManagerKernelTestBase {
],
[
ValidationResult::createError([
- 'The stage directory has 3 instances of module2.info.yml as compared to 1 in the active directory. This likely indicates that a duplicate extension was installed.',
+ t('The stage directory has 3 instances of module2.info.yml as compared to 1 in the active directory. This likely indicates that a duplicate extension was installed.'),
]),
ValidationResult::createError([
- 'The stage directory has 2 instances of module1.info.yml as compared to 1 in the active directory. This likely indicates that a duplicate extension was installed.',
+ t('The stage directory has 2 instances of module1.info.yml as compared to 1 in the active directory. This likely indicates that a duplicate extension was installed.'),
]),
],
],
@@ -155,10 +155,10 @@ class DuplicateInfoFileValidatorTest extends PackageManagerKernelTestBase {
],
[
ValidationResult::createError([
- 'The stage directory has 3 instances of module2.info.yml. This likely indicates that a duplicate extension was installed.',
+ t('The stage directory has 3 instances of module2.info.yml. This likely indicates that a duplicate extension was installed.'),
]),
ValidationResult::createError([
- 'The stage directory has 2 instances of module1.info.yml. This likely indicates that a duplicate extension was installed.',
+ t('The stage directory has 2 instances of module1.info.yml. This likely indicates that a duplicate extension was installed.'),
]),
],
],
@@ -175,10 +175,10 @@ class DuplicateInfoFileValidatorTest extends PackageManagerKernelTestBase {
],
[
ValidationResult::createError([
- 'The stage directory has 3 instances of module2.info.yml. This likely indicates that a duplicate extension was installed.',
+ t('The stage directory has 3 instances of module2.info.yml. This likely indicates that a duplicate extension was installed.'),
]),
ValidationResult::createError([
- 'The stage directory has 2 instances of module1.info.yml as compared to 1 in the active directory. This likely indicates that a duplicate extension was installed.',
+ t('The stage directory has 2 instances of module1.info.yml as compared to 1 in the active directory. This likely indicates that a duplicate extension was installed.'),
]),
],
],
diff --git a/package_manager/tests/src/Kernel/EnvironmentSupportValidatorTest.php b/package_manager/tests/src/Kernel/EnvironmentSupportValidatorTest.php
index 328839bb95..806f1b0a26 100644
--- a/package_manager/tests/src/Kernel/EnvironmentSupportValidatorTest.php
+++ b/package_manager/tests/src/Kernel/EnvironmentSupportValidatorTest.php
@@ -42,7 +42,7 @@ class EnvironmentSupportValidatorTest extends PackageManagerKernelTestBase {
});
$result = ValidationResult::createError([
- 'Package Manager is not supported by your environment.',
+ t('Package Manager is not supported by your environment.'),
]);
$this->assertEventPropagationStopped(PreApplyEvent::class, [$this->container->get(EnvironmentSupportValidator::class), 'validate']);
@@ -57,7 +57,7 @@ class EnvironmentSupportValidatorTest extends PackageManagerKernelTestBase {
putenv(EnvironmentSupportValidator::VARIABLE_NAME . '=' . $url);
$result = ValidationResult::createError([
- '<a href="' . $url . '">Package Manager is not supported by your environment.</a>',
+ t('<a href=":url">Package Manager is not supported by your environment.</a>', [':url' => $url]),
]);
$this->assertStatusCheckResults([$result]);
$this->assertResults([$result], PreCreateEvent::class);
@@ -73,7 +73,7 @@ class EnvironmentSupportValidatorTest extends PackageManagerKernelTestBase {
});
$result = ValidationResult::createError([
- '<a href="' . $url . '">Package Manager is not supported by your environment.</a>',
+ t('<a href=":url">Package Manager is not supported by your environment.</a>', [':url' => $url]),
]);
$this->assertResults([$result], PreApplyEvent::class);
}
diff --git a/package_manager/tests/src/Kernel/PendingUpdatesValidatorTest.php b/package_manager/tests/src/Kernel/PendingUpdatesValidatorTest.php
index bf4c160057..2c3cb64f4c 100644
--- a/package_manager/tests/src/Kernel/PendingUpdatesValidatorTest.php
+++ b/package_manager/tests/src/Kernel/PendingUpdatesValidatorTest.php
@@ -75,7 +75,7 @@ class PendingUpdatesValidatorTest extends PackageManagerKernelTestBase {
// will think it's pending.
require_once __DIR__ . '/../../fixtures/post_update.php';
$result = ValidationResult::createError([
- 'Some modules have database schema updates to install. You should run the <a href="/update.php">database update script</a> immediately.',
+ t('Some modules have database schema updates to install. You should run the <a href="/update.php">database update script</a> immediately.'),
]);
try {
$stage->apply();
diff --git a/package_manager/tests/src/Kernel/SupportedReleaseValidatorTest.php b/package_manager/tests/src/Kernel/SupportedReleaseValidatorTest.php
index 754234ad3a..87f15c57de 100644
--- a/package_manager/tests/src/Kernel/SupportedReleaseValidatorTest.php
+++ b/package_manager/tests/src/Kernel/SupportedReleaseValidatorTest.php
@@ -163,7 +163,7 @@ class SupportedReleaseValidatorTest extends PackageManagerKernelTestBase {
'type' => 'drupal-theme',
],
[
- ValidationResult::createError(['package_manager_theme (drupal/package_manager_theme) 8.2.0'], $summary),
+ ValidationResult::createError([t('package_manager_theme (drupal/package_manager_theme) 8.2.0')], $summary),
],
],
// For modules that don't start with 'drupal/' will not have update XML
diff --git a/package_manager/tests/src/Kernel/WritableFileSystemValidatorTest.php b/package_manager/tests/src/Kernel/WritableFileSystemValidatorTest.php
index 3b31104e18..ec5f13f4ae 100644
--- a/package_manager/tests/src/Kernel/WritableFileSystemValidatorTest.php
+++ b/package_manager/tests/src/Kernel/WritableFileSystemValidatorTest.php
@@ -141,14 +141,14 @@ class WritableFileSystemValidatorTest extends PackageManagerKernelTestBase {
'write-protected stage root exists' => [
$non_writable_permission,
[
- ValidationResult::createError(['The stage root directory "<STAGE_ROOT>" is not writable.'], $summary),
+ ValidationResult::createError([t('The stage root directory "<STAGE_ROOT>" is not writable.')], $summary),
],
FALSE,
],
'stage root directory does not exist, parent directory not writable' => [
$non_writable_permission,
[
- ValidationResult::createError(['The stage root directory will not able to be created at "<STAGE_ROOT_PARENT>".'], $summary),
+ ValidationResult::createError([t('The stage root directory will not able to be created at "<STAGE_ROOT_PARENT>".')], $summary),
],
TRUE,
],
diff --git a/package_manager/tests/src/Unit/ValidationResultTest.php b/package_manager/tests/src/Unit/ValidationResultTest.php
index b64f3fbc77..3651c4d926 100644
--- a/package_manager/tests/src/Unit/ValidationResultTest.php
+++ b/package_manager/tests/src/Unit/ValidationResultTest.php
@@ -91,6 +91,22 @@ class ValidationResultTest extends UnitTestCase {
ValidationResult::createError($messages, NULL);
}
+ /**
+ * Tests that the messages are asserted to be translatable.
+ *
+ * @testWith ["createError"]
+ * ["createWarning"]
+ */
+ public function testMessagesMustBeTranslatable(string $method): void {
+ // When creating an error from a throwable, the message does not need to be
+ // translatable.
+ ValidationResult::createErrorFromThrowable(new \Exception('Burn it down.'));
+
+ $this->expectException(\AssertionError::class);
+ $this->expectExceptionMessageMatches('/instanceof TranslatableMarkup/');
+ ValidationResult::$method(['Not translatable!']);
+ }
+
/**
* Data provider for test methods that test create exceptions.
*
diff --git a/src/Validator/ScaffoldFilePermissionsValidator.php b/src/Validator/ScaffoldFilePermissionsValidator.php
index 72f7957d13..90ddef01b8 100644
--- a/src/Validator/ScaffoldFilePermissionsValidator.php
+++ b/src/Validator/ScaffoldFilePermissionsValidator.php
@@ -83,7 +83,7 @@ final class ScaffoldFilePermissionsValidator implements EventSubscriberInterface
if ($non_writable_files) {
// Re-key the messages in order to prevent false negative comparisons in
// tests.
- $non_writable_files = array_values($non_writable_files);
+ $non_writable_files = array_map($this->t(...), array_values($non_writable_files));
$event->addError($non_writable_files, $this->t('The following paths must be writable in order to update default site configuration files.'));
}
}
diff --git a/src/Validator/StagedDatabaseUpdateValidator.php b/src/Validator/StagedDatabaseUpdateValidator.php
index 2662c40666..90c3d5018f 100644
--- a/src/Validator/StagedDatabaseUpdateValidator.php
+++ b/src/Validator/StagedDatabaseUpdateValidator.php
@@ -45,6 +45,7 @@ class StagedDatabaseUpdateValidator implements EventSubscriberInterface {
$invalid_extensions = $this->stagedDBUpdateValidator->getExtensionsWithDatabaseUpdates($stage->getStageDirectory());
if ($invalid_extensions) {
+ $invalid_extensions = array_map($this->t(...), $invalid_extensions);
$event->addError($invalid_extensions, $this->t('The update cannot proceed because possible database updates have been detected in the following extensions.'));
}
}
diff --git a/tests/src/Kernel/StatusCheck/CronServerValidatorTest.php b/tests/src/Kernel/StatusCheck/CronServerValidatorTest.php
index 5383df65fb..3926a38c9d 100644
--- a/tests/src/Kernel/StatusCheck/CronServerValidatorTest.php
+++ b/tests/src/Kernel/StatusCheck/CronServerValidatorTest.php
@@ -225,7 +225,7 @@ class CronServerValidatorTest extends AutomaticUpdatesKernelTestBase {
foreach ($expected_results as $i => $result) {
$messages = [];
foreach ($result->getMessages() as $message) {
- $messages[] = "$message See <a href=\"$url\">the Automatic Updates help page</a> for more information on how to resolve this.";
+ $messages[] = t('@message See <a href=":url">the Automatic Updates help page</a> for more information on how to resolve this.', ['@message' => $message, ':url' => $url]);
}
$expected_results[$i] = ValidationResult::createError($messages);
}
diff --git a/tests/src/Kernel/StatusCheck/ScaffoldFilePermissionsValidatorTest.php b/tests/src/Kernel/StatusCheck/ScaffoldFilePermissionsValidatorTest.php
index 4fad511771..08148333c2 100644
--- a/tests/src/Kernel/StatusCheck/ScaffoldFilePermissionsValidatorTest.php
+++ b/tests/src/Kernel/StatusCheck/ScaffoldFilePermissionsValidatorTest.php
@@ -52,6 +52,7 @@ class ScaffoldFilePermissionsValidatorTest extends AutomaticUpdatesKernelTestBas
// Prepend the active directory to every path listed in the error result,
// and add the expected summary.
$messages = array_map($map, $result->getMessages());
+ $messages = array_map(t(...), $messages);
$expected_results[$i] = ValidationResult::createError($messages, t('The following paths must be writable in order to update default site configuration files.'));
}
parent::assertValidationResultsEqual($expected_results, $actual_results, $path_locator);
--
GitLab