From 0a7c3372a2bd5f1dacdd861b51d8100acf71ea5e Mon Sep 17 00:00:00 2001 From: Lee Rowlands <lee.rowlands@previousnext.com.au> Date: Mon, 3 Jul 2023 16:51:23 +1000 Subject: [PATCH] Issue #2920678 by phenaproxima, Wim Leers, dawehner, Gogowitsch, Sam152, dww, longwave, yogeshmpawar, alexpott, borisson_, tstoeckler, jibran, amateescu, larowlan, tedbow: Add config validation for the allowed characters of machine names --- core/config/schema/core.data_types.schema.yml | 10 ++ .../block/config/schema/block.schema.yml | 7 +- .../tests/src/Kernel/BlockValidationTest.php | 14 +++ .../config/schema/block_content.schema.yml | 2 +- .../comment/config/schema/comment.schema.yml | 2 +- .../contact/config/schema/contact.schema.yml | 7 +- .../filter/config/schema/filter.schema.yml | 7 +- .../image/config/schema/image.schema.yml | 2 +- .../media/config/schema/media.schema.yml | 7 +- .../src/Kernel/MediaTypeValidationTest.php | 2 +- .../node/config/schema/node.schema.yml | 7 +- .../config/schema/responsive_image.schema.yml | 2 +- .../search/config/schema/search.schema.yml | 2 +- .../config/schema/shortcut.schema.yml | 9 +- .../src/Kernel/ShortcutSetValidationTest.php | 27 ++++- .../system/config/schema/system.schema.yml | 11 +- .../src/Kernel/Entity/MenuValidationTest.php | 25 ++++- .../config/schema/taxonomy.schema.yml | 7 +- .../tour/config/schema/tour.schema.yml | 2 +- .../tests/src/Kernel/TourValidationTest.php | 34 ++++++ .../user/config/schema/user.schema.yml | 2 +- .../views/config/schema/views.schema.yml | 7 +- .../config/schema/workflows.schema.yml | 2 +- .../Config/ConfigEntityValidationTestBase.php | 105 +++++++++++++++++- .../Core/Config/ConfigSchemaTest.php | 3 +- 25 files changed, 277 insertions(+), 28 deletions(-) create mode 100644 core/modules/tour/tests/src/Kernel/TourValidationTest.php diff --git a/core/config/schema/core.data_types.schema.yml b/core/config/schema/core.data_types.schema.yml index 4476e98d0c7e..d866c01b1ed1 100644 --- a/core/config/schema/core.data_types.schema.yml +++ b/core/config/schema/core.data_types.schema.yml @@ -94,6 +94,16 @@ color_hex: type: string label: 'Color' +# Machine-readable identifier that can only contain certain characters. +machine_name: + type: string + label: 'Machine name' + constraints: + Regex: '/^[a-z0-9_]+$/' + Length: + # @see \Drupal\Core\Config\Entity\ConfigEntityStorage::MAX_ID_LENGTH + max: 166 + # Complex extended data types: # Root of a configuration object. diff --git a/core/modules/block/config/schema/block.schema.yml b/core/modules/block/config/schema/block.schema.yml index d768a9619503..64dda31dbbb7 100644 --- a/core/modules/block/config/schema/block.schema.yml +++ b/core/modules/block/config/schema/block.schema.yml @@ -5,8 +5,13 @@ block.block.*: label: 'Block' mapping: id: - type: string + type: machine_name label: 'ID' + # Blocks have atypical machine names: they allow periods for historical reasons. + # @see https://www.drupal.org/project/drupal/issues/2685917 + # @see https://www.drupal.org/project/drupal/issues/2043527 + constraints: + Regex: '/^[a-z0-9_.]+$/' theme: type: string label: 'Theme' diff --git a/core/modules/block/tests/src/Kernel/BlockValidationTest.php b/core/modules/block/tests/src/Kernel/BlockValidationTest.php index c3c6142e3520..db6f502eab1c 100644 --- a/core/modules/block/tests/src/Kernel/BlockValidationTest.php +++ b/core/modules/block/tests/src/Kernel/BlockValidationTest.php @@ -39,4 +39,18 @@ public function testInvalidPluginId(): void { $this->assertValidationErrors(['plugin' => "The 'non_existent' plugin does not exist."]); } + /** + * Block names are atypical in that they allow periods in the machine name. + */ + public function providerInvalidMachineNameCharacters(): array { + $cases = parent::providerInvalidMachineNameCharacters(); + // Remove the existing test case that verifies a machine name containing + // periods is invalid. + $this->assertSame(['period.separated', FALSE], $cases['INVALID: period separated']); + unset($cases['INVALID: period separated']); + // And instead add a test case that verifies it is allowed for blocks. + $cases['VALID: period separated'] = ['period.separated', TRUE]; + return $cases; + } + } diff --git a/core/modules/block_content/config/schema/block_content.schema.yml b/core/modules/block_content/config/schema/block_content.schema.yml index 77a898f5c055..59cb4744a673 100644 --- a/core/modules/block_content/config/schema/block_content.schema.yml +++ b/core/modules/block_content/config/schema/block_content.schema.yml @@ -5,7 +5,7 @@ block_content.type.*: label: 'Block type settings' mapping: id: - type: string + type: machine_name label: 'ID' label: type: label diff --git a/core/modules/comment/config/schema/comment.schema.yml b/core/modules/comment/config/schema/comment.schema.yml index 7c31ccbba9bb..f53d0730e058 100644 --- a/core/modules/comment/config/schema/comment.schema.yml +++ b/core/modules/comment/config/schema/comment.schema.yml @@ -38,7 +38,7 @@ comment.type.*: label: 'Comment type settings' mapping: id: - type: string + type: machine_name label: 'ID' label: type: label diff --git a/core/modules/contact/config/schema/contact.schema.yml b/core/modules/contact/config/schema/contact.schema.yml index 1f0585db856b..5fb69d63393a 100644 --- a/core/modules/contact/config/schema/contact.schema.yml +++ b/core/modules/contact/config/schema/contact.schema.yml @@ -5,8 +5,13 @@ contact.form.*: label: 'Contact form' mapping: id: - type: string + type: machine_name label: 'ID' + constraints: + Length: + # Contact form IDs are specifically limited to 32 characters. + # @see \Drupal\contact\ContactFormEditForm::form() + max: 32 label: type: label label: 'Label' diff --git a/core/modules/filter/config/schema/filter.schema.yml b/core/modules/filter/config/schema/filter.schema.yml index b45a4755958c..04c61d0f8141 100644 --- a/core/modules/filter/config/schema/filter.schema.yml +++ b/core/modules/filter/config/schema/filter.schema.yml @@ -19,8 +19,13 @@ filter.format.*: type: label label: 'Name' format: - type: string + type: machine_name label: 'Machine name' + # Filter format machine names can be up to 255 characters. + # @see \Drupal\filter\FilterFormatFormBase::form() + constraints: + Length: + max: 255 weight: type: integer label: 'Weight' diff --git a/core/modules/image/config/schema/image.schema.yml b/core/modules/image/config/schema/image.schema.yml index 5368f85f4cdc..8cbd56b6f6b5 100644 --- a/core/modules/image/config/schema/image.schema.yml +++ b/core/modules/image/config/schema/image.schema.yml @@ -5,7 +5,7 @@ image.style.*: label: 'Image style' mapping: name: - type: string + type: machine_name label: type: label label: 'Label' diff --git a/core/modules/media/config/schema/media.schema.yml b/core/modules/media/config/schema/media.schema.yml index b5322c9ccfee..29c56d37502e 100644 --- a/core/modules/media/config/schema/media.schema.yml +++ b/core/modules/media/config/schema/media.schema.yml @@ -20,8 +20,13 @@ media.type.*: label: 'Media type' mapping: id: - type: string + type: machine_name label: 'Machine name' + constraints: + Length: + # Media type IDs are specifically limited to 32 characters. + # @see \Drupal\media\MediaTypeForm::form() + max: 32 label: type: label label: 'Name' diff --git a/core/modules/media/tests/src/Kernel/MediaTypeValidationTest.php b/core/modules/media/tests/src/Kernel/MediaTypeValidationTest.php index c5f2774b2c89..8315284280a7 100644 --- a/core/modules/media/tests/src/Kernel/MediaTypeValidationTest.php +++ b/core/modules/media/tests/src/Kernel/MediaTypeValidationTest.php @@ -24,7 +24,7 @@ class MediaTypeValidationTest extends ConfigEntityValidationTestBase { */ protected function setUp(): void { parent::setUp(); - $this->entity = $this->createMediaType('test'); + $this->entity = $this->createMediaType('test', ['id' => 'test_media']); } } diff --git a/core/modules/node/config/schema/node.schema.yml b/core/modules/node/config/schema/node.schema.yml index 1caa3576e766..c33ab3ee2331 100644 --- a/core/modules/node/config/schema/node.schema.yml +++ b/core/modules/node/config/schema/node.schema.yml @@ -16,8 +16,13 @@ node.type.*: type: label label: 'Name' type: - type: string + type: machine_name label: 'Machine-readable name' + constraints: + # Node type machine names are specifically limited to 32 characters. + # @see \Drupal\node\NodeTypeForm::form() + Length: + max: 32 description: type: text label: 'Description' diff --git a/core/modules/responsive_image/config/schema/responsive_image.schema.yml b/core/modules/responsive_image/config/schema/responsive_image.schema.yml index 8fae6175938b..32c5d2dac8f4 100644 --- a/core/modules/responsive_image/config/schema/responsive_image.schema.yml +++ b/core/modules/responsive_image/config/schema/responsive_image.schema.yml @@ -5,7 +5,7 @@ responsive_image.styles.*: label: 'Responsive image style' mapping: id: - type: string + type: machine_name label: 'Machine-readable name' label: type: label diff --git a/core/modules/search/config/schema/search.schema.yml b/core/modules/search/config/schema/search.schema.yml index 64d75b8db9a9..1f47b02c0afc 100644 --- a/core/modules/search/config/schema/search.schema.yml +++ b/core/modules/search/config/schema/search.schema.yml @@ -72,7 +72,7 @@ search.page.*: label: 'Search page' mapping: id: - type: string + type: machine_name label: 'ID' label: type: label diff --git a/core/modules/shortcut/config/schema/shortcut.schema.yml b/core/modules/shortcut/config/schema/shortcut.schema.yml index 7d9420ba1212..6fceb5741e27 100644 --- a/core/modules/shortcut/config/schema/shortcut.schema.yml +++ b/core/modules/shortcut/config/schema/shortcut.schema.yml @@ -5,8 +5,15 @@ shortcut.set.*: label: 'Shortcut settings' mapping: id: - type: string + type: machine_name label: 'ID' + # Shortcut set IDs are specifically limited to 23 characters, and allow + # dashes but not underscores. + # @see \Drupal\shortcut\ShortcutSetForm::form() + constraints: + Regex: '/^[a-z0-9-]+$/' + Length: + max: 23 label: type: label label: 'Label' diff --git a/core/modules/shortcut/tests/src/Kernel/ShortcutSetValidationTest.php b/core/modules/shortcut/tests/src/Kernel/ShortcutSetValidationTest.php index a27f3980d186..dd9dbc1bc0d1 100644 --- a/core/modules/shortcut/tests/src/Kernel/ShortcutSetValidationTest.php +++ b/core/modules/shortcut/tests/src/Kernel/ShortcutSetValidationTest.php @@ -26,10 +26,35 @@ protected function setUp(): void { $this->installEntitySchema('shortcut'); $this->entity = ShortcutSet::create([ - 'id' => 'test', + 'id' => 'test-shortcut-set', 'label' => 'Test', ]); $this->entity->save(); } + /** + * Shortcut set IDs are atypical: they allow dashes and disallow underscores. + */ + public function providerInvalidMachineNameCharacters(): array { + $cases = parent::providerInvalidMachineNameCharacters(); + + // Remove the existing test case that verifies a machine name containing + // dashes is invalid. + $this->assertSame(['dash-separated', FALSE], $cases['INVALID: dash separated']); + unset($cases['INVALID: dash separated']); + // And instead add a test case that verifies it is allowed for shortcut + // sets. + $cases['VALID: dash separated'] = ['dash-separated', TRUE]; + + // Remove the existing test case that verifies a machine name containing + // underscores is valid. + $this->assertSame(['underscore_separated', TRUE], $cases['VALID: underscore separated']); + unset($cases['VALID: underscore separated']); + // And instead add a test case that verifies it is disallowed for shortcut + // sets. + $cases['INVALID: underscore separated'] = ['underscore_separated', FALSE]; + + return $cases; + } + } diff --git a/core/modules/system/config/schema/system.schema.yml b/core/modules/system/config/schema/system.schema.yml index 5d2a5c3fc7cc..3de5d4348124 100644 --- a/core/modules/system/config/schema/system.schema.yml +++ b/core/modules/system/config/schema/system.schema.yml @@ -212,8 +212,15 @@ system.menu.*: label: 'Menu' mapping: id: - type: string + type: machine_name label: 'ID' + # Menu IDs are specifically limited to 32 characters, and allow dashes but not + # underscores. + # @see \Drupal\menu_ui\MenuForm::form() + constraints: + Regex: '/^[a-z0-9-]+$/' + Length: + max: 32 label: type: label label: 'Label' @@ -229,7 +236,7 @@ system.action.*: label: 'System action' mapping: id: - type: string + type: machine_name label: 'ID' label: type: label diff --git a/core/modules/system/tests/src/Kernel/Entity/MenuValidationTest.php b/core/modules/system/tests/src/Kernel/Entity/MenuValidationTest.php index 1c1ebd752b68..7cb041759b2d 100644 --- a/core/modules/system/tests/src/Kernel/Entity/MenuValidationTest.php +++ b/core/modules/system/tests/src/Kernel/Entity/MenuValidationTest.php @@ -19,10 +19,33 @@ protected function setUp(): void { parent::setUp(); $this->entity = Menu::create([ - 'id' => 'test', + 'id' => 'test-menu', 'label' => 'Test', ]); $this->entity->save(); } + /** + * Menu IDs are atypical: they allow dashes and disallow underscores. + */ + public function providerInvalidMachineNameCharacters(): array { + $cases = parent::providerInvalidMachineNameCharacters(); + + // Remove the existing test case that verifies a machine name containing + // dashes is invalid. + $this->assertSame(['dash-separated', FALSE], $cases['INVALID: dash separated']); + unset($cases['INVALID: dash separated']); + // And instead add a test case that verifies it is allowed for menus. + $cases['VALID: dash separated'] = ['dash-separated', TRUE]; + + // Remove the existing test case that verifies a machine name containing + // underscores is valid. + $this->assertSame(['underscore_separated', TRUE], $cases['VALID: underscore separated']); + unset($cases['VALID: underscore separated']); + // And instead add a test case that verifies it is disallowed for menus. + $cases['INVALID: underscore separated'] = ['underscore_separated', FALSE]; + + return $cases; + } + } diff --git a/core/modules/taxonomy/config/schema/taxonomy.schema.yml b/core/modules/taxonomy/config/schema/taxonomy.schema.yml index 1b472752ab12..3e24333784b2 100644 --- a/core/modules/taxonomy/config/schema/taxonomy.schema.yml +++ b/core/modules/taxonomy/config/schema/taxonomy.schema.yml @@ -22,8 +22,13 @@ taxonomy.vocabulary.*: type: label label: 'Name' vid: - type: string + type: machine_name label: 'Machine name' + # Vocabulary machine names are specifically limited to 32 characters. + # @see \Drupal\taxonomy\VocabularyForm::form() + constraints: + Length: + max: 32 description: type: label label: 'Description' diff --git a/core/modules/tour/config/schema/tour.schema.yml b/core/modules/tour/config/schema/tour.schema.yml index d8ffed213379..05fcb5b0581e 100644 --- a/core/modules/tour/config/schema/tour.schema.yml +++ b/core/modules/tour/config/schema/tour.schema.yml @@ -5,7 +5,7 @@ tour.tour.*: label: 'Tour settings' mapping: id: - type: string + type: machine_name label: 'ID' label: type: label diff --git a/core/modules/tour/tests/src/Kernel/TourValidationTest.php b/core/modules/tour/tests/src/Kernel/TourValidationTest.php new file mode 100644 index 000000000000..42b0d28640d1 --- /dev/null +++ b/core/modules/tour/tests/src/Kernel/TourValidationTest.php @@ -0,0 +1,34 @@ +<?php + +namespace Drupal\Tests\tour\Kernel; + +use Drupal\KernelTests\Core\Config\ConfigEntityValidationTestBase; +use Drupal\tour\Entity\Tour; + +/** + * Tests validation of tour entities. + * + * @group tour + */ +class TourValidationTest extends ConfigEntityValidationTestBase { + + /** + * {@inheritdoc} + */ + protected static $modules = ['tour']; + + /** + * {@inheritdoc} + */ + protected function setUp(): void { + parent::setUp(); + + $this->entity = Tour::create([ + 'id' => 'test', + 'label' => 'Test', + 'module' => 'system', + ]); + $this->entity->save(); + } + +} diff --git a/core/modules/user/config/schema/user.schema.yml b/core/modules/user/config/schema/user.schema.yml index b778163bf5c0..2e61a61599ad 100644 --- a/core/modules/user/config/schema/user.schema.yml +++ b/core/modules/user/config/schema/user.schema.yml @@ -108,7 +108,7 @@ user.role.*: label: 'User role settings' mapping: id: - type: string + type: machine_name label: 'ID' label: type: label diff --git a/core/modules/views/config/schema/views.schema.yml b/core/modules/views/config/schema/views.schema.yml index 0d38b89d1bf4..1939f88fb8ac 100644 --- a/core/modules/views/config/schema/views.schema.yml +++ b/core/modules/views/config/schema/views.schema.yml @@ -70,8 +70,13 @@ views.view.*: label: 'View' mapping: id: - type: string + type: machine_name label: 'ID' + constraints: + Length: + # View IDs are specifically limited to 128 characters. + # @see \Drupal\views_ui\ViewAddForm::form() + max: 128 label: type: label label: 'Label' diff --git a/core/modules/workflows/config/schema/workflows.schema.yml b/core/modules/workflows/config/schema/workflows.schema.yml index 587a2d5f5b53..72a47a353e2b 100644 --- a/core/modules/workflows/config/schema/workflows.schema.yml +++ b/core/modules/workflows/config/schema/workflows.schema.yml @@ -3,7 +3,7 @@ workflows.workflow.*: label: 'Workflow' mapping: id: - type: string + type: machine_name label: 'ID' label: type: label diff --git a/core/tests/Drupal/KernelTests/Core/Config/ConfigEntityValidationTestBase.php b/core/tests/Drupal/KernelTests/Core/Config/ConfigEntityValidationTestBase.php index 3512517f2554..2aa8cedb8744 100644 --- a/core/tests/Drupal/KernelTests/Core/Config/ConfigEntityValidationTestBase.php +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigEntityValidationTestBase.php @@ -39,6 +39,106 @@ protected function setUp(): void { $this->container = $this->container->get('kernel')->getContainer(); } + /** + * Ensures that the entity created in ::setUp() has no validation errors. + */ + public function testEntityIsValid(): void { + $this->assertInstanceOf(ConfigEntityInterface::class, $this->entity); + $this->assertValidationErrors([]); + } + + /** + * Returns the validation constraints applied to the entity's ID. + * + * If the entity type does not define an ID key, the test will fail. If an ID + * key is defined but is not using the `machine_name` data type, the test will + * be skipped. + * + * @return array[] + * The validation constraint configuration applied to the entity's ID. + */ + protected function getMachineNameConstraints(): array { + $id_key = $this->entity->getEntityType()->getKey('id'); + $this->assertNotEmpty($id_key, "The entity under test does not define an ID key."); + + $data_definition = $this->entity->getTypedData() + ->get($id_key) + ->getDataDefinition(); + if ($data_definition->getDataType() === 'machine_name') { + return $data_definition->getConstraints(); + } + else { + $this->markTestSkipped("The entity's ID key does not use the machine_name data type."); + } + } + + /** + * Data provider for ::testInvalidMachineNameCharacters(). + * + * @return array[] + * The test cases. + */ + public function providerInvalidMachineNameCharacters(): array { + return [ + 'INVALID: space separated' => ['space separated', FALSE], + 'INVALID: dash separated' => ['dash-separated', FALSE], + 'INVALID: uppercase letters' => ['Uppercase_Letters', FALSE], + 'INVALID: period separated' => ['period.separated', FALSE], + 'VALID: underscore separated' => ['underscore_separated', TRUE], + ]; + } + + /** + * Tests that the entity's ID is tested for invalid characters. + * + * @param string $machine_name + * A machine name to test. + * @param bool $is_expected_to_be_valid + * Whether this machine name is expected to be considered valid. + * + * @dataProvider providerInvalidMachineNameCharacters + */ + public function testInvalidMachineNameCharacters(string $machine_name, bool $is_expected_to_be_valid): void { + $constraints = $this->getMachineNameConstraints(); + + $this->assertNotEmpty($constraints['Regex']); + $this->assertIsString($constraints['Regex']); + + $id_key = $this->entity->getEntityType()->getKey('id'); + if ($is_expected_to_be_valid) { + $expected_errors = []; + } + else { + $expected_errors = [$id_key => 'This value is not valid.']; + } + + $this->entity->set( + $id_key, + $machine_name + ); + $this->assertValidationErrors($expected_errors); + } + + /** + * Tests that the entity ID's length is validated if it is a machine name. + */ + public function testMachineNameLength(): void { + $constraints = $this->getMachineNameConstraints(); + + $max_length = $constraints['Length']['max']; + $this->assertIsInt($max_length); + $this->assertGreaterThan(0, $max_length); + + $id_key = $this->entity->getEntityType()->getKey('id'); + $this->entity->set( + $id_key, + mb_strtolower($this->randomMachineName($max_length + 2)) + ); + $this->assertValidationErrors([ + $id_key => 'This value is too long. It should have <em class="placeholder">' . $max_length . '</em> characters or less.', + ]); + } + /** * Data provider for ::testConfigDependenciesValidation(). * @@ -159,11 +259,6 @@ public function providerConfigDependenciesValidation(): array { * @dataProvider providerConfigDependenciesValidation */ public function testConfigDependenciesValidation(array $dependencies, array $expected_messages): void { - $this->assertInstanceOf(ConfigEntityInterface::class, $this->entity); - - // The entity should have valid data to begin with. - $this->assertValidationErrors([]); - // Add the dependencies we were given to the dependencies that may already // exist in the entity. $dependencies = NestedArray::mergeDeep($dependencies, $this->entity->getDependencies()); diff --git a/core/tests/Drupal/KernelTests/Core/Config/ConfigSchemaTest.php b/core/tests/Drupal/KernelTests/Core/Config/ConfigSchemaTest.php index 602ca3624e3f..05e7b54c79a0 100644 --- a/core/tests/Drupal/KernelTests/Core/Config/ConfigSchemaTest.php +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigSchemaTest.php @@ -176,7 +176,7 @@ public function testSchemaMapping() { $expected['class'] = Mapping::class; $expected['definition_class'] = '\Drupal\Core\TypedData\MapDataDefinition'; $expected['unwrap_for_canonical_representation'] = TRUE; - $expected['mapping']['name']['type'] = 'string'; + $expected['mapping']['name']['type'] = 'machine_name'; $expected['mapping']['uuid']['type'] = 'uuid'; $expected['mapping']['uuid']['label'] = 'UUID'; $expected['mapping']['langcode']['type'] = 'string'; @@ -185,7 +185,6 @@ public function testSchemaMapping() { $expected['mapping']['status']['label'] = 'Status'; $expected['mapping']['dependencies']['type'] = 'config_dependencies'; $expected['mapping']['dependencies']['label'] = 'Dependencies'; - $expected['mapping']['name']['type'] = 'string'; $expected['mapping']['label']['type'] = 'label'; $expected['mapping']['label']['label'] = 'Label'; $expected['mapping']['effects']['type'] = 'sequence'; -- GitLab