From c980ece64ec31e8ae96a3c73d450472dff69b531 Mon Sep 17 00:00:00 2001 From: Alex Pott <alex.a.pott@googlemail.com> Date: Mon, 25 Dec 2023 09:46:12 +0100 Subject: [PATCH] Issue #3406478 by Wim Leers, phenaproxima, borisson_, alexpott, larowlan: Refine ValidKeysConstraintValidator's messages: use #3401883's new Mapping infrastructure --- .../Constraint/ValidKeysConstraint.php | 48 ++--- .../ValidKeysConstraintValidator.php | 162 ++++++++++++++++- .../Kernel/Migrate/d6/MigrateBlockTest.php | 6 +- .../ValidKeysConstraintValidatorTest.php | 168 ++++++++++++++++-- 4 files changed, 344 insertions(+), 40 deletions(-) diff --git a/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ValidKeysConstraint.php b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ValidKeysConstraint.php index 842615c9ea24..3fd7bb271bbc 100644 --- a/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ValidKeysConstraint.php +++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ValidKeysConstraint.php @@ -5,28 +5,35 @@ namespace Drupal\Core\Validation\Plugin\Validation\Constraint; use Drupal\Core\Config\Schema\Mapping; -use Drupal\Core\TypedData\MapDataDefinition; use Symfony\Component\Validator\Constraint; use Symfony\Component\Validator\Context\ExecutionContextInterface; use Symfony\Component\Validator\Exception\InvalidArgumentException; /** - * Checks that all the keys of a mapping are known. + * Checks that all the keys of a mapping are valid and required keys present. * * @Constraint( * id = "ValidKeys", * label = @Translation("Valid mapping keys", context = "Validation"), + * type = { "mapping" }, * ) */ class ValidKeysConstraint extends Constraint { /** - * The error message if an invalid key appears. + * The error message if a key is invalid. * * @var string */ public string $invalidKeyMessage = "'@key' is not a supported key."; + /** + * The error message if a key is unknown for the resolved type. + * + * @var string + */ + public string $dynamicInvalidKeyMessage = "'@key' is an unknown key because @dynamic_type_property_path is @dynamic_type_property_value (see config schema type @resolved_dynamic_type)."; + /** * The error message if the array being validated is a list. * @@ -65,33 +72,28 @@ public function getRequiredOptions() { * The keys that will be considered valid. */ public function getAllowedKeys(ExecutionContextInterface $context): array { + $mapping = $context->getObject(); + assert($mapping instanceof Mapping); + $resolved_type = $mapping->getDataDefinition()->getDataType(); + $valid_keys = $mapping->getValidKeys(); + // If we were given an explicit array of allowed keys, return that. if (is_array($this->allowedKeys)) { - return $this->allowedKeys; + if (!empty(array_diff($this->allowedKeys, $valid_keys))) { + throw new InvalidArgumentException(sprintf( + 'The type \'%s\' explicitly specifies the allowed keys (%s), but they are not a subset of the statically defined mapping keys in the schema (%s).', + $resolved_type, + implode(', ', $this->allowedKeys), + implode(', ', $valid_keys) + )); + } + return array_intersect($valid_keys, $this->allowedKeys); } // The only other value we'll accept is the string `<infer>`. elseif ($this->allowedKeys === '<infer>') { - return static::inferKeys($context->getObject()); + return $mapping->getValidKeys(); } throw new InvalidArgumentException("'$this->allowedKeys' is not a valid set of allowed keys."); } - /** - * Tries to auto-detect the schema-defined keys in a mapping. - * - * @param \Drupal\Core\Config\Schema\Mapping $mapping - * The mapping to inspect. - * - * @return string[] - * The keys defined in the mapping's schema. - */ - protected static function inferKeys(Mapping $mapping): array { - $definition = $mapping->getDataDefinition(); - assert($definition instanceof MapDataDefinition); - - $definition = $definition->toArray(); - assert(array_key_exists('mapping', $definition)); - return array_keys($definition['mapping']); - } - } diff --git a/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ValidKeysConstraintValidator.php b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ValidKeysConstraintValidator.php index 90c2ff76a946..3f6026ee685b 100644 --- a/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ValidKeysConstraintValidator.php +++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ValidKeysConstraintValidator.php @@ -4,6 +4,9 @@ namespace Drupal\Core\Validation\Plugin\Validation\Constraint; +use Drupal\Core\Config\Schema\Mapping; +use Drupal\Core\Config\Schema\SequenceDataDefinition; +use Drupal\Core\TypedData\MapDataDefinition; use Symfony\Component\Validator\Constraint; use Symfony\Component\Validator\ConstraintValidator; use Symfony\Component\Validator\Exception\UnexpectedTypeException; @@ -36,10 +39,21 @@ public function validate(mixed $value, Constraint $constraint) { return; } - $invalid_keys = array_diff( - array_keys($value), - $constraint->getAllowedKeys($this->context) - ); + $mapping = $this->context->getObject(); + assert($mapping instanceof Mapping); + $resolved_type = $mapping->getDataDefinition()->getDataType(); + + $valid_keys = $constraint->getAllowedKeys($this->context); + $dynamically_valid_keys = $mapping->getDynamicallyValidKeys(); + $all_dynamically_valid_keys = array_merge(...array_values($dynamically_valid_keys)); + + // Statically valid: keys that are valid for all possible types matching the + // type definition of this mapping. + // For example, `block.block.*:settings` has the following statically valid + // keys: id, label, label_display, provider, status, info, view_mode and + // context_mapping. + // @see \Drupal\KernelTests\Config\Schema\MappingTest::providerMappingInterpretation() + $invalid_keys = array_diff(array_keys($value), $valid_keys, $all_dynamically_valid_keys); foreach ($invalid_keys as $key) { $this->context->buildViolation($constraint->invalidKeyMessage) ->setParameter('@key', $key) @@ -47,6 +61,146 @@ public function validate(mixed $value, Constraint $constraint) { ->setInvalidValue($key) ->addViolation(); } + // Dynamically valid: keys that are valid not for all possible types, but + // for the actually resolved type definition of this mapping (in addition to + // the statically valid keys). + // @see \Drupal\Core\Config\Schema\Mapping::getDynamicallyValidKeys() + if (!empty($all_dynamically_valid_keys)) { + // For example, `block.block.*:settings` has the following dynamically valid + // keys when the block plugin is `system_branding_block`: + // - use_site_logo + // - use_site_name + // - use_site_slogan + // @see \Drupal\KernelTests\Config\Schema\MappingTest::providerMappingInterpretation() + $resolved_type_dynamically_valid_keys = $dynamically_valid_keys[$resolved_type] ?? []; + // But if the `local_tasks_block` plugin is being used, then the + // dynamically valid keys are: + // - primary + // - secondary + // And for the `block.settings.search_form_block` plugin the dynamically + // valid keys are: + // - page_id + // To help determine which keys are dynamically invalid, gather all keys + // except for those for the actual resolved type of this mapping. + // @see \Drupal\Core\Config\Schema\Mapping::getPossibleTypes() + $other_types_valid_keys = array_diff($all_dynamically_valid_keys, $resolved_type_dynamically_valid_keys); + $dynamically_invalid_keys = array_intersect(array_keys($value), $other_types_valid_keys); + foreach ($dynamically_invalid_keys as $key) { + $this->context->addViolation($constraint->dynamicInvalidKeyMessage, ['@key' => $key] + self::getDynamicMessageParameters($mapping)); + } + } + } + + /** + * Computes message parameters for dynamic type violations. + * + * @param \Drupal\Core\Config\Schema\Mapping $mapping + * A `type: mapping` instance, with values. + * + * @return array + * An array containing the following message parameters: + * - '@unresolved_dynamic_type': unresolved dynamic type + * - '@resolved_dynamic_type': resolved dynamic type + * - '@dynamic_type_property_path': (relative) property path of the condition + * - '@dynamic_type_property_value': value of the condition + * + * @see \Drupal\Core\Validation\Plugin\Validation\Constraint\ValidKeysConstraint::$dynamicInvalidKeyMessage + */ + protected static function getDynamicMessageParameters(Mapping $mapping): array { + $definition = $mapping->getDataDefinition(); + assert($definition instanceof MapDataDefinition); + $definition = $definition->toArray(); + assert(array_key_exists('mapping', $definition)); + + // The original mapping definition is used to determine the unresolved type. + // e.g. if $unresolved_type is … + // 1. `editor.settings.[%parent.editor]`, then $resolved_type could perhaps + // `editor.settings.ckeditor5`, `editor.settings.unicorn`, etc. + // 2. `block.settings.[%parent.plugin]`, then $resolved_type could perhaps + // be `block.settings.*`, `block.settings.system_branding_block`, etc. + $parent_data_def = $mapping->getParent()->getDataDefinition(); + $unresolved_type = match (TRUE) { + $parent_data_def instanceof MapDataDefinition => $parent_data_def->toArray()['mapping'][$mapping->getName()]['type'], + $parent_data_def instanceof SequenceDataDefinition => $parent_data_def->toArray()['sequence']['type'], + default => throw new \LogicException('Invalid config schema detected.'), + }; + $resolved_type = $definition['type']; + + // $unresolved_type must be a dynamic type and the resolved type must be + // different and not be dynamic. + // @see \Drupal\Core\Config\TypedConfigManager::buildDataDefinition() + assert(strpos($unresolved_type, ']')); + assert($unresolved_type !== $resolved_type); + assert(!strpos($resolved_type, ']')); + + $message_parameters = [ + '@unresolved_dynamic_type' => $unresolved_type, + '@resolved_dynamic_type' => $resolved_type, + ]; + + $config = $mapping->getRoot(); + // Every config object is a mapping. + assert($config instanceof Mapping); + // Find the relative property path where this mapping starts. + assert(str_starts_with($mapping->getPropertyPath(), $config->getName() . '.')); + $property_path_mapping = substr($mapping->getPropertyPath(), strlen($config->getName()) + 1); + + // Extract the expressions stored in the dynamic type name. + $matches = []; + // @see \Drupal\Core\Config\TypedConfigManager::replaceDynamicTypeName() + $result = preg_match("/\[(.*)\]/U", $unresolved_type, $matches); + assert($result === 1); + // @see \Drupal\Core\Config\TypedConfigManager::replaceExpression() + $expression = $matches[1]; + // From the expression, extract the instructions for where to retrieve a value. + $instructions = explode('.', $expression); + + // Determine the property path to the configuration key that has determined + // this type. + // @see \Drupal\Core\Config\TypedConfigManager::replaceExpression() + $property_path_parts = explode('.', $property_path_mapping); + // @see \Drupal\Core\Config\Schema\Mapping::getDynamicallyValidKeys() + assert(!in_array('%type', $instructions, TRUE)); + + // The %key instruction can only be used on its own. In this case, there is + // no need to fetch a value, only the string that was used as the key is + // responsible for determining the mapping type. + if ($instructions === ['%key']) { + $key = array_pop($property_path_parts); + array_push($property_path_parts, '%key'); + $resolved_property_path = implode('.', $property_path_parts); + return $message_parameters + [ + '@dynamic_type_property_path' => $resolved_property_path, + '@dynamic_type_property_value' => $key, + ]; + } + + // Do not replace variables, do not traverse the tree of data, but instead + // resolve the property path that contains the value causing this particular + // type to be selected. + while ($instructions) { + $instruction = array_shift($instructions); + // Go up one level: remove the last part of the property path. + if ($instruction === '%parent') { + array_pop($property_path_parts); + } + // Go down one level: append the given key. + else { + array_push($property_path_parts, $instruction); + } + } + $resolved_property_path = implode('.', $property_path_parts); + $message_parameters += [ + '@dynamic_type_property_path' => $resolved_property_path, + ]; + + // Determine the corresponding value for that property path. + $val = $config->get($resolved_property_path)->getValue(); + // @see \Drupal\Core\Config\TypedConfigManager::replaceExpression() + $val = is_bool($val) ? (int) $val : $val; + return $message_parameters + [ + '@dynamic_type_property_value' => $val, + ]; } } diff --git a/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php index b55d0757944f..c9613a28712f 100644 --- a/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php +++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php @@ -303,10 +303,10 @@ public function testBlockMigration() { $this->assertCount(6, $messages); $this->assertSame($messages[0]->message, 'Schema errors for block.block.block_1 with the following errors: 0 [dependencies.theme.0] Theme 'bluemarine' is not installed.'); $this->assertSame($messages[1]->message, "d6_block:visibility: The block with bid '13' from module 'block' will have no PHP or request_path visibility configuration."); - $this->assertSame($messages[2]->message, 'Schema errors for block.block.aggregator with the following errors: block.block.aggregator:settings.block_count missing schema, block.block.aggregator:settings.feed missing schema, 0 [settings.block_count] 'block_count' is not a supported key., 1 [settings.feed] 'feed' is not a supported key.'); + $this->assertSame($messages[2]->message, 'Schema errors for block.block.aggregator with the following errors: block.block.aggregator:settings.block_count missing schema, block.block.aggregator:settings.feed missing schema, 0 [settings.feed] 'feed' is not a supported key., 1 [settings] 'block_count' is an unknown key because plugin is aggregator_feed_block (see config schema type block.settings.*).'); $this->assertSame($messages[3]->message, 'Schema errors for block.block.book with the following errors: block.block.book:settings.block_mode missing schema, 0 [settings.block_mode] 'block_mode' is not a supported key.'); - $this->assertSame('Schema errors for block.block.forum with the following errors: block.block.forum:settings.block_count missing schema, 0 [settings.block_count] 'block_count' is not a supported key.', $messages[4]->message); - $this->assertSame('Schema errors for block.block.forum_1 with the following errors: block.block.forum_1:settings.block_count missing schema, 0 [settings.block_count] 'block_count' is not a supported key.', $messages[5]->message); + $this->assertSame('Schema errors for block.block.forum with the following errors: block.block.forum:settings.block_count missing schema, 0 [settings] 'block_count' is an unknown key because plugin is forum_active_block (see config schema type block.settings.*).', $messages[4]->message); + $this->assertSame('Schema errors for block.block.forum_1 with the following errors: block.block.forum_1:settings.block_count missing schema, 0 [settings] 'block_count' is an unknown key because plugin is forum_new_block (see config schema type block.settings.*).', $messages[5]->message); } } diff --git a/core/tests/Drupal/KernelTests/Core/TypedData/ValidKeysConstraintValidatorTest.php b/core/tests/Drupal/KernelTests/Core/TypedData/ValidKeysConstraintValidatorTest.php index e3877465e59c..fdef1566e187 100644 --- a/core/tests/Drupal/KernelTests/Core/TypedData/ValidKeysConstraintValidatorTest.php +++ b/core/tests/Drupal/KernelTests/Core/TypedData/ValidKeysConstraintValidatorTest.php @@ -2,7 +2,9 @@ namespace Drupal\KernelTests\Core\TypedData; -use Drupal\Core\TypedData\DataDefinition; +use Drupal\block\Entity\Block; +use Drupal\Core\TypedData\MapDataDefinition; +use Drupal\Core\TypedData\TraversableTypedDataInterface; use Drupal\KernelTests\KernelTestBase; use Symfony\Component\Validator\Exception\UnexpectedTypeException; @@ -16,20 +18,116 @@ */ class ValidKeysConstraintValidatorTest extends KernelTestBase { + /** + * The typed config under test. + * + * @var \Drupal\Core\TypedData\TraversableTypedDataInterface + * + * @see \Drupal\Core\Config\TypedConfigManagerInterface::get() + */ + protected TraversableTypedDataInterface $config; + + /** + * {@inheritdoc} + */ + protected function setUp(): void { + parent::setUp(); + // Install the Block module and create a Block config entity, so that we can + // test that the validator infers keys from a defined schema. + $this->enableModules(['system', 'block']); + /** @var \Drupal\Core\Extension\ThemeInstallerInterface $theme_installer */ + $theme_installer = $this->container->get('theme_installer'); + $theme_installer->install(['stark']); + + $block = Block::create([ + 'id' => 'branding', + 'plugin' => 'system_branding_block', + 'theme' => 'stark', + ]); + $block->save(); + + $this->config = $this->container->get('config.typed') + ->get('block.block.branding'); + } + + /** + * Tests detecting unsupported keys. + * + * @see \Drupal\Core\Validation\Plugin\Validation\Constraint\ValidKeysConstraint::$invalidKeyMessage + */ + public function testSupportedKeys(): void { + // Start from the valid config. + $this->assertEmpty($this->config->validate()); + + // Then modify only one thing: generate a non-existent `foobar` setting. + $data = $this->config->toArray(); + $data['settings']['foobar'] = TRUE; + $this->assertValidationErrors('block.block.branding', $data, + // Now 1 validation error should be triggered: one for the unsupported key. + // @see \Drupal\system\Plugin\Block\SystemBrandingBlock::defaultConfiguration() + // @see \Drupal\system\Plugin\Block\SystemPoweredByBlock::defaultConfiguration() + [ + 'settings.foobar' => "'foobar' is not a supported key.", + ], + ); + } + + /** + * Tests detecting unknown keys. + * + * @see \Drupal\Core\Validation\Plugin\Validation\Constraint\ValidKeysConstraint::$dynamicInvalidKeyMessage + */ + public function testUnknownKeys(): void { + // Start from the valid config. + $this->assertEmpty($this->config->validate()); + + // Then modify only one thing: the block plugin that is being used. + $data = $this->config->toArray(); + $data['plugin'] = 'system_powered_by_block'; + $this->assertValidationErrors('block.block.branding', $data, + // Now 3 validation errors should be triggered: one for each of the + // settings that exist in the "branding" block but not the "powered by" + // block. + // @see \Drupal\system\Plugin\Block\SystemBrandingBlock::defaultConfiguration() + // @see \Drupal\system\Plugin\Block\SystemPoweredByBlock::defaultConfiguration() + [ + 'settings' => [ + "'use_site_logo' is an unknown key because plugin is system_powered_by_block (see config schema type block.settings.*).", + "'use_site_name' is an unknown key because plugin is system_powered_by_block (see config schema type block.settings.*).", + "'use_site_slogan' is an unknown key because plugin is system_powered_by_block (see config schema type block.settings.*).", + ], + ], + ); + } + /** * Tests the ValidKeys constraint validator. */ public function testValidation(): void { // Create a data definition that specifies certain allowed keys. - $definition = DataDefinition::create('any') + $definition = MapDataDefinition::create('mapping') ->addConstraint('ValidKeys', ['north', 'south', 'west']); + $definition['mapping'] = [ + 'north' => ['type' => 'string', 'requiredKey' => FALSE], + 'east' => ['type' => 'string', 'requiredKey' => FALSE], + 'south' => ['type' => 'string', 'requiredKey' => FALSE], + 'west' => ['type' => 'string', 'requiredKey' => FALSE], + ]; + // @todo Remove this line in https://www.drupal.org/project/drupal/issues/3403782 + $definition->setClass('Drupal\Core\Config\Schema\Mapping'); - /** @var \Drupal\Core\TypedData\TypedDataManagerInterface $typed_data */ - $typed_data = $this->container->get('typed_data_manager'); + /** @var \Drupal\Core\TypedData\TypedDataManagerInterface $typed_config */ + $typed_config = $this->container->get('config.typed'); + // @see \Drupal\Core\Config\TypedConfigManager::buildDataDefinition() + // @see \Drupal\Core\TypedData\TypedDataManager::createDataDefinition() + $definition->setTypedDataManager($typed_config); // Passing a non-array value should raise an exception. try { - $typed_data->create($definition, 2501)->validate(); + // TRICKY: we must clone the definition because the instance is modified + // when processing. + // @see \Drupal\Core\Config\Schema\Mapping::processRequiredKeyFlags() + $typed_config->create(clone $definition, 2501)->validate(); $this->fail('Expected an exception but none was raised.'); } catch (UnexpectedTypeException $e) { @@ -37,21 +135,21 @@ public function testValidation(): void { } // Empty arrays are valid. - $this->assertCount(0, $typed_data->create($definition, [])->validate()); + $this->assertCount(0, $typed_config->create(clone $definition, [])->validate()); // Indexed arrays are never valid. - $violations = $typed_data->create($definition, ['north', 'south'])->validate(); + $violations = $typed_config->create(clone $definition, ['north', 'south'])->validate(); $this->assertCount(1, $violations); $this->assertSame('Numerically indexed arrays are not allowed.', (string) $violations->get(0)->getMessage()); // Arrays with automatically assigned keys, AND a valid key, should be // considered invalid overall. - $violations = $typed_data->create($definition, ['north', 'south' => 'west'])->validate(); + $violations = $typed_config->create(clone $definition, ['north', 'south' => 'west'])->validate(); $this->assertCount(1, $violations); $this->assertSame("'0' is not a supported key.", (string) $violations->get(0)->getMessage()); // Associative arrays with an invalid key should be invalid. - $violations = $typed_data->create($definition, ['north' => 'south', 'east' => 'west'])->validate(); + $violations = $typed_config->create(clone $definition, ['north' => 'south', 'east' => 'west'])->validate(); $this->assertCount(1, $violations); $this->assertSame("'east' is not a supported key.", (string) $violations->get(0)->getMessage()); @@ -61,7 +159,18 @@ public function testValidation(): void { 'south' => 'Atlanta', 'west' => 'San Francisco', ]; - $violations = $typed_data->create($definition, $value)->validate(); + $violations = $typed_config->create(clone $definition, $value)->validate(); + $this->assertCount(0, $violations); + + // If, in the mapping definition, some keys do NOT have + // `requiredKey: false` set, then they MUST be set. In other + // words, all keys are required unless they individually + // specify otherwise. + // First test without changing the value: no error should occur because all + // keys passed to the ValidKeys constraint have a value. + unset($definition['mapping']['south']['requiredKey']); + unset($definition['mapping']['east']['requiredKey']); + $violations = $typed_config->create(clone $definition, $value)->validate(); $this->assertCount(0, $violations); } @@ -94,4 +203,43 @@ public function testValidKeyInference(): void { $config->validate(); } + /** + * Asserts a set of validation errors is raised when the config is validated. + * + * @param string $config_name + * The machine name of the configuration. + * @param array $config_data + * The data associated with the configuration. Note: This configuration + * doesn't yet have to be stored. + * @param array<string, string|string[]> $expected_messages + * The expected validation error messages. Keys are property paths, values + * are the expected messages: a string if a single message is expected, an + * array of strings if multiple are expected. + */ + protected function assertValidationErrors(string $config_name, array $config_data, array $expected_messages): void { + $violations = $this->container->get('config.typed') + ->createFromNameAndData($config_name, $config_data) + ->validate(); + + $actual_messages = []; + foreach ($violations as $violation) { + $property_path = $violation->getPropertyPath(); + + if (!isset($actual_messages[$property_path])) { + $actual_messages[$property_path] = (string) $violation->getMessage(); + } + else { + // Transform value from string to array. + if (is_string($actual_messages[$property_path])) { + $actual_messages[$property_path] = (array) $actual_messages[$violation->getPropertyPath()]; + } + // And append. + $actual_messages[$property_path][] = (string) $violation->getMessage(); + } + } + ksort($expected_messages); + ksort($actual_messages); + $this->assertSame($expected_messages, $actual_messages); + } + } -- GitLab