Add config validation for the allowed characters of machine names

1. +++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/MachineNameConstraint.php
   @@ -0,0 +1,19 @@
   +  public $message = 'This value should be a valid machine name.';
   

   What about adding a message how a valid machine name looks like?

2. +++ b/core/tests/Drupal/KernelTests/Core/Validation/ConstraintsTest.php
   @@ -26,20 +26,43 @@ protected function setUp() {
   +  /**
   +   * Data provider for ::testValidation.
   +   */
   +  public function validationsDataProvider() {
   +    return [
   

   It feels like adding tests for everything inside one data provider wouldn't really scale. Maybe keeping them separate is actually better, what do you think?

#8: I agree with having a test class per constraint be the best for discoverability.

I always find the ClassName and ClassNameTest convention very discoverable.

These are exactly my thoughts :)

Here is an adaption of the patch :)

We should convert TypedConfigTest into UuidValidatorTest

I agree, but I thought: this is out of the scope of this issue.

That looks great!

I think the message change was missed in the interdiff. I think the only problem is it implies there can only be one underscore.

+1

+++ b/core/config/schema/core.data_types.schema.yml
@@ -129,7 +135,7 @@ filter:
-      type: string
+      type: machine_name

❤️

This definitely also helps API-First! The IS even indicates that. Tagging as such.

I'm missing one thing though: a change record, which informs contrib/custom module developers about this, so they can update their config schemas.

@Wim Leers
I thought it would be a good idea to add all the constraint things into one issue, which seem to happen in #2869792: [meta] Add constraints to all config entity types for now.

Isn't this just one of many, many implementation issues? i.e. #2869792: [meta] Add constraints to all config entity types is the plan issue, this is a child issue?

I guess I'm not sure what you mean exactly — could you clarify?

@Wim Leers
I think having one change record instead of multiple would be way easier for people to find information.

1. +++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/MachineNameConstraintValidator.php
   @@ -0,0 +1,22 @@
   +    if (!preg_match('/^[a-z0-9_]+$/', $value)) {
   

   This is unfortunately not that simple :( we need to be able to configure this as this is not the same for all machine names. For example, much to my chargrin block IDs can have a dot in...

         '#machine_name' => [
           'exists' => '\Drupal\block\Entity\Block::load',
           'replace_pattern' => '[^a-z0-9_.]+',
           'source' => ['settings', 'label'],
         ],
   

   See \Drupal\block\BlockForm::form()

2. +++ b/core/modules/config/tests/config_test/src/ConfigValidation.php
   @@ -74,7 +74,7 @@ public static function validateGiraffes($string, ExecutionContextInterface $cont
   -    if ($diff = array_diff(array_keys($mapping), ['llama', 'cat', 'giraffe', 'uuid', '_core'])) {
   +    if ($diff = array_diff(array_keys($mapping), ['llama', 'cat', 'giraffe', 'uuid', 'machine_name', '_core'])) {
   

   This test looks a bit annoying if it going to change whenever we add a new validation.

Created #2925689: ConfigValidation class contains code that is brittle and changing for every addition to address #22.2

@alexpott
It still makes sense to have a default format, right?

@dawehner yep I think so - to match what's in \Drupal\Core\Render\Element\MachineName::processMachineName() - ie. [^a-z0-9_]

This adds support for configuring the pattern. I'm curious whether we should update the change record in https://www.drupal.org/node/2906029

I added a change record https://www.drupal.org/node/2954832 for setting up config validation itself.

+++ b/core/modules/responsive_image/config/schema/responsive_image.schema.yml
@@ -5,7 +5,7 @@ responsive_image.styles.*:
-      type: string
+      type: machine_name

Do we need an upgrade path for this schema change?

@jibran How would you upgrade something like this? What we need a change record explaining what module authors can do now.

Let's fix the test failures :) This never worked, I'm glad we have tests.

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/MachineNameConstraint.php
@@ -0,0 +1,21 @@
+  public $message = 'This value should be a valid machine name. Valid machine names contain only alphanumeric characters and underscores.';

Since the replace pattern is configurable, the second part of the message is not necessarily accurate...

Thank you for your review!

Since the replace pattern is configurable, the second part of the message is not necessarily accurate...

You are absolutely right, On the other hand though, much like you can configure the replacePattern, you can also configure the message in your constraint configuration.

Ahh, I never saw a constraint's message being configured, but you're totally right, I don't see why it wouldn't be possible :)

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/MachineNameConstraint.php
@@ -0,0 +1,21 @@
+class MachineNameConstraint extends Constraint {

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/MachineNameConstraintValidator.php
@@ -0,0 +1,22 @@
+class MachineNameConstraintValidator extends ConstraintValidator {

We already have \Drupal\Core\Validation\Plugin\Validation\Constraint\RegexConstraint which is using Symfony's \Symfony\Component\Validator\Constraints\RegexValidator.

So, if we really want this new constraint instead of using the existing Regex (for DX reasons I guess?), then we should at least extend it and let it handle the validation (i.e. remove the custom MachineNameConstraintValidator from this patch).

OTOH, do we really need the new constraint in the first place? :)

Good point! I first though we could need to negate the regex, but Regex allows you to say "$match = false".

but Regex allows you to say "$match = false".

Just saw this issue and couldn't get it to work because I hadn't figured this out.

Suggestion in #35 and #36 seem like a good idea to avoid adding our own constraint and validator.

+++ b/core/tests/Drupal/KernelTests/Core/Validation/MachineNameValidationTest.php
@@ -0,0 +1,59 @@
+    $this->assertCount($violation_count, $violations);

Should we assert the message text here? Because presumably something else could be broken in the config system we might get a different message.

Otherwise looks very good

In addition to #37, I only have a couple of minor observations:

1. +++ b/core/tests/Drupal/KernelTests/Core/Validation/MachineNameValidationTest.php
   @@ -0,0 +1,59 @@
   + * Tests the machine name constraint.
   

   Note that we're no longer testing the machine name constraint, but the machine_name config data type.

2. +++ b/core/tests/Drupal/KernelTests/Core/Validation/MachineNameValidationTest.php
   @@ -0,0 +1,59 @@
   +  public function validationsDataProvider() {
   

   I wonder if it's really useful to instantiate this test twice for this very simple scenario.. I would simply do both assertions in the test method and save a second of the testbot's time. But that's just me, feel free to ignore this point :)

Note that we're no longer testing the machine name constraint, but the machine_name config data type.

Done :)

I wonder if it's really useful to instantiate this test twice for this very simple scenario.. I would simply do both assertions in the test method and save a second of the testbot's time. But that's just me, feel free to ignore this point :)

Can't be bothered right now. It is basically a 32th of a second, due to the parallel execution.

This looks great to me now :)

1. +++ b/core/config/schema/core.data_types.schema.yml
   @@ -82,6 +82,15 @@ uuid:
   +machine_name:
   +  type: string
   +  label: 'Machine name'
   

   We are adding a new data type to config schema, to make this available I think we at least add an empty update hook to clear the cache.

2. +++ b/core/config/schema/core.data_types.schema.yml
   @@ -132,7 +141,7 @@ filter:
   -      type: string
   +      type: machine_name
   

   RE #30: @dawehner: I don't think we need an update hook for this because type is not stored in anywhere in active config.

3. We also need change notice as @dawehner said in #30.

There is a change record already :)

Sorry, missed that.

No worries :)

We are adding a new data type to config schema, to make this available I think we at least add an empty update hook to clear the cache.

Sure, let's do that.

Honestly, next time, in case you want, I'd be super happy if you would fix it too.

> Honestly, next time, in case you want, I'd be super happy if you would fix it too.

I didn't mean to annoy you. I just wanted to have a discussion about it before we agree on it and thank you for agreeing with me and updating the patch. I'll try to be more helpful next time.

I didn't mean to annoy you. I just wanted to have a discussion about it before we agree on it and thank you for agreeing with me and updating the patch. I'll try to be more helpful next time.

Oh sorry, I really didn't wanted to say that. All I wanted to say is: Feel free to provide a patch too :)

+++ b/core/config/schema/core.data_types.schema.yml
@@ -82,6 +82,15 @@ uuid:
+      pattern: '/[^a-z0-9_]+/'

+++ b/core/modules/block/config/schema/block.schema.yml
@@ -5,7 +5,7 @@ block.block.*:
-      type: string
+      type: machine_name

This is not quite right. Because blocks allow dots in there machine name. See \Drupal\block\BlockForm::form() - the block schema needs a custom pattern. Also we need to test this.

+++ b/core/modules/config/tests/config_test/src/ConfigValidation.php
@@ -74,7 +74,7 @@ public static function validateGiraffes($string, ExecutionContextInterface $cont
-    if ($diff = array_diff(array_keys($mapping), ['llama', 'cat', 'giraffe', 'uuid', '_core'])) {
+    if ($diff = array_diff(array_keys($mapping), ['llama', 'cat', 'giraffe', 'uuid', 'machine_name', '_core'])) {

This shouldn't need to change now.

I expanded the validation for menus and entity displays.

TODOs:

• Shortcut sets, switches
• Date formats
• Actually power the UI we have from the config schema

Continue to work on it.

I converted more instances and added test coverage to some of them.

I discussed a bit with @alexpott whether we want to do the UI aspects here. I think we settled to limit the scope of this issue for now and continuously work on that later.

Let's fix the test failures.

1. +++ b/core/modules/shortcut/tests/src/Kernel/ShortcutSetTest.php
   @@ -0,0 +1,52 @@
   +
   +
   

   Should be only one blank line

2. +++ b/core/modules/system/tests/src/Kernel/Menu/MenuConfigTest.php
   @@ -0,0 +1,27 @@
   +  public function testValidation() {
   

   Can we not use the same dataprovider we use in ShortcutSetTest, EntityFormDisplayTest and EntityDisplayTest here?

   This dataprovider is reused in 3 places now and I think we can either copy it in, or refer to it from a trait and/or change the @dataProvider to point to one of the other classes?

3. +++ b/core/modules/views/views.install
   @@ -508,3 +508,10 @@ function views_update_8500() {
   +function system_update_8600() {
   +  // Empty update to cause a cache flush so that config schema cache is rebuilt.
   +}
   

   This should move to a post update, see https://drupal.org/node/2960601

4. +++ b/core/tests/Drupal/KernelTests/Core/Validation/MachineNameValidationTest.php
   @@ -0,0 +1,59 @@
   +  }
   +}
   

   There needs to be a blank line before the last closing brace of a class.

This should move to a post update, see https://drupal.org/node/2960601

I'm not sure about this anymore, see https://www.drupal.org/project/drupal/issues/2743313#comment-12575919

@borisson_ Thanks for your review - here are my answers:

• Ad 1: done
• Ad 2: Not really, because the regexes are different and the data provider contains the information if the validation fails.
• Ad 3: This is beyond my expertise, so I haven't updated this. Also, see #58.
• Ad 4: done

Here is what I changed on top of the above:

1. Some validation messages were wrong: include the fact that dots are allowed for blocks; removed the mention of underscores where they are not allows.
2. Added the dot as an allowed character for core.entity_view_display.mapping.id, because it gets added in \Drupal\Core\Plugin\Context\EntityContextDefinition::fromEntityType. This is why the previous patch failed testing.
3. Converted $this->assertEquals to self::assertEquals because it this is how we should call static methods.
4. Converted $this->assertEqual to self::assertEquals because the former is deprecated.
5. message key: Made very long lines in YAML shorter by using the >- multiline.
6. Removed a few spaces in YAML to conform to 2 spaces per indention level.

I noticed that the empty machine names are considered valid as we just look for invalid characters. As a follow-up issue we might have to add a LengthConstraint.

+++ b/core/modules/views/views.install
@@ -532,3 +532,10 @@ function views_update_8500() {
+/**
+ * Clear caches due to additional config schema types.
+ */
+function system_update_8600() {
+  // Empty update to cause a cache flush so that config schema cache is rebuilt.
+}

Needs to be moved to the system module and should be a post update now as that's what we do to ensure a cache flush. To do this we need to add a function like system_post_update_field_formatter_entity_schema() to core/modules/system/system.post_update.php. The debate from #58 has been resolved.

Ah, got it! There is a new patch attached converting the hook_update() to a hook_post_update() and moved it to the system module. This leads to another advantage: the cache clear will work for both 8.6.x and 8.7.x.

As this issue is marked as 8.7.x-dev, I will ignore the failed test for 8.6.x unless someone objects.

@Gogowitsch yeah this addition is not eligible for 8.6.x anyway.

I am re-uploading the patch from #62. It's unchanged from the perspective of content. The patches differ by line numbers and patch context.

This is to force the test runner to apply it to the 8.8.x branch, so it can be reviewed and merged with more confidence.

1. +++ b/core/config/schema/core.data_types.schema.yml
   @@ -82,6 +82,17 @@ uuid:
   +      match: false
   +      pattern: '/[^a-z0-9_]+/'
   
   +++ b/core/config/schema/core.entity.schema.yml
   @@ -40,8 +40,16 @@ core.entity_view_display.*.*.*:
   +          match: false
   +          pattern: '/[^a-z0-9._]+/'
   
   @@ -98,8 +106,13 @@ core.entity_form_display.*.*.*:
   +          match: false
   +          pattern: '/[^a-z0-9]+/'
   
   +++ b/core/modules/block/config/schema/block.schema.yml
   @@ -5,8 +5,15 @@ block.block.*:
   +          match: false
   +          pattern: '/[^a-z0-9_.]+/'
   
   +++ b/core/modules/shortcut/config/schema/shortcut.schema.yml
   @@ -5,8 +5,13 @@ shortcut.set.*:
   +          match: false
   +          pattern: '/[^a-z0-9-]+/'
   

   Maybe there's a reason for that that I'm missing, but I find this double negative with the regular expression hard to understand.

   I guess this was taken from the replace pattern in the form, but since there's no direct connection to that here, I wonder if it wouldn't be better for future developers to simplify this?

2. +++ b/core/config/schema/core.data_types.schema.yml
   @@ -420,8 +431,13 @@ core.date_format.*:
   +          message: This value should be a valid machine name. Valid machine names contain only alphanumeric characters and underscores.
   

   Should this mention that the string "custom" is not allowed?

3. +++ b/core/modules/block/tests/src/Kernel/BlockConfigSchemaTest.php
   @@ -85,9 +85,36 @@ public function testBlockConfigSchema() {
   -      $this->assertEqual($config->get('id'), $id);
   +      self::assertEquals($config->get('id'), $id);
   ...
   +    self::assertEquals('id', $violations->get(0)->getPropertyPath());
   +    self::assertEquals('This value should be a valid machine name. Valid machine names contain only alphanumeric characters, dots and underscores.', $violations->get(0)->getMessageTemplate());
   

   Any reason for using self:: here over $this->?

Oh, and I guess this introduces some coding standard violations so those need to be fixed at least.

> Any reason for using self:: here over $this->?

I think this is trying to follow the deprecation of the SimpleTest assertion methods, but also I think changing existing assertions is out of scope here and should be left for #2496109: Replace all usages to deprecated methods on KernelTestBase(NG)

Addressed #69 and #70; I negated the regular expressions and simplified the one remaining negative one (as "custom" is easier to look for with a negative match), improved the "custom" validation message, and also renamed the data providers so they follow the standard we use in core.

The regexes could also limit the maximum number of characters; should we add this?

The patch from comment #72 looks very good to me.

@longwave: Regarding the YAML: What was the reason for replacing the multi-line >- strings with single lines? I am fine with either, but find long lines a little harder to read.

It was not consistent before - some were multiline and some were not - and I find the single lines easier to read (up to a point anyway!).

Happy to change them all the other way as long as we are consistent; I also looked at the yaml coding standards but it doesn't seem to specify line lengths.

Also it just struck me that for the ones that allow symbols (underscore, dot, etc) do we have any rules about allowing these as the first character?

Thanks for the quick turnaround!

Patch looks good to me, so setting RTBC. While I personally kind of like the multiline YAML strings they are not used very much in core, so I think going with the single-line strings is a bit less controversial and increases the chance of this landing soon.

Re #75: That's a very good question!!! I just tried and it is actually possible to create a block with the machine name "..." through the UI. I'm not really sure that that's a good idea, but since it "works" that's definitely follow-up material if we want to change that. But good catch!

@tstoeckler: do you think it is worth adding length checks to the regexes as I suggested in #72?

Oh right, totally missed that point, sorry.

Kicking to back to needs review to discuss that.

I think since the point of this issue (at least one specific formulation of it) is to generalize the validation of machine names that is currently done in the UI to the config schema it would only be right to bring the maxlength validation along with it. I can also see that being handled in a follow-up, because the patch is definitely already an improvement over the status quo.

I went through all the places where we use machine name form element in core and tracked the maxlengths. The default is 64, so everything that is not mentioned below has that maxlength:

* Actually it is 32 minus the field prefix (i.e. the value of the "field_prefix" key in the "field_ui.settings" config). So maybe we would have to implement hook_config_schema_alter() to adapt that? That definitely sounds like follow-up material though, it kind of feels like a can of worms...

I can't decide if the best thing to do here is add the length to the regex, or add a separate LengthConstraint to the machine name data types. The regex means that we can handle everything in a single validation, but a separate length constraint would be easier to manage for the more complicated config entities like field_storage and menu.

Also, config entities are already supposed to know their maximum length via the MAX_ID_LENGTH constant, so perhaps we can reuse this and automatically set a length constraint on the machine names? Embedding it in the regex from the constant seems like it would be much more difficult.

As you say, it does feel like this might be better done in a followup, as this is already a significant improvement on its own.

Yes, I think #79 is a great point. I opened #3109137: Add config validation for length of machine names for adding length constraints. So marking this RTBC again (and slightly narrowing the title).

1. +++ b/core/config/schema/core.data_types.schema.yml
   @@ -82,6 +82,14 @@ uuid:
   +      message: This value should be a valid machine name. Valid machine names contain only alphanumeric characters and underscores.
   

   Is this an existing pattern in core? Seems verbose and repetitive. Why not just:

   "This value should be a valid machine name, containing only alphanumeric characters and underscores."

   ?

2. +++ b/core/config/schema/core.data_types.schema.yml
   @@ -424,8 +432,13 @@ core.date_format.*:
   +          message: This value should be a valid machine name. Valid machine names contain only alphanumeric characters and underscores, and cannot be the reserved string "custom".
   
   +++ b/core/config/schema/core.entity.schema.yml
   @@ -22,7 +22,7 @@ core.entity_form_mode.*.*:
      mapping:
        id:
   -      type: string
   +      type: machine_name
          label: 'ID'
        label:
   

   Same

3. +++ b/core/config/schema/core.entity.schema.yml
   @@ -40,8 +40,13 @@ core.entity_view_display.*.*.*:
   +          message: This value should be a valid machine name. Valid machine names contain only alphanumeric characters, underscores and dots.
   

   And here.

   Also, in cases like this, I think it can be a nice UI touch to include examples of the punctuation you're talking about (since not everyone agrees on "dots" vs. "periods" vs. "full-stop" etc). E.g.:

   This value should be a valid machine name, containing only alphanumeric bcharacters, dots (.) and underscores (_).

   Although now that I see it, that does look a little ugly with the final dot. So maybe ignore this part of the review... ;)

4. +++ b/core/config/schema/core.entity.schema.yml
   @@ -71,16 +76,16 @@ field_formatter:
   -       type: string
   -       label: 'Label setting machine name'
   +      type: string
   +      label: 'Label setting machine name'
        settings:
          type: field.formatter.settings.[%parent.type]
          label: 'Settings'
        third_party_settings:
   -       type: sequence
   -       label: 'Third party settings'
   -       sequence:
   -         type: field.formatter.third_party.[%key]
   +      type: sequence
   +      label: 'Third party settings'
   +      sequence:
   +        type: field.formatter.third_party.[%key]
   

   I know you're just fixing the spacing here, but it's technically out of scope from this issue. Might be better to fix this in a follow-up to make the patch smaller?

5. +++ b/core/config/schema/core.entity.schema.yml
   @@ -98,8 +103,12 @@ core.entity_form_display.*.*.*:
   +          message: This value should be a valid machine name. Valid machine names contain only alphanumeric characters.
   

   Same as above.

6. +++ b/core/modules/block/config/schema/block.schema.yml
   @@ -5,8 +5,12 @@ block.block.*:
   +          message: This value should be a valid machine name. Valid machine names contain only alphanumeric characters, dots and underscores.
   

   And here. Sorry...

7. +++ b/core/modules/block/tests/src/Kernel/BlockConfigSchemaTest.php
   @@ -90,4 +90,31 @@ public function testBlockConfigSchema() {
   +      'theme' => 'classy',
   

   We're trying to get Classy out of core. Does this have to test with classy like this? If it doesn't matter, let's use stark instead.

8. +++ b/core/modules/block/tests/src/Kernel/BlockConfigSchemaTest.php
   @@ -90,4 +90,31 @@ public function testBlockConfigSchema() {
   +      'weight' => 00,
   

   00?

9. +++ b/core/modules/block/tests/src/Kernel/BlockConfigSchemaTest.php
   @@ -90,4 +90,31 @@ public function testBlockConfigSchema() {
   +    $this->assertEquals('This value should be a valid machine name. Valid machine names contain only alphanumeric characters, dots and underscores.', $violations->get(0)->getMessageTemplate());
   

   If we change this above, we have to change it here, too.

10. +++ b/core/modules/field_ui/tests/src/Kernel/EntityFormDisplayTest.php
    @@ -276,4 +276,37 @@ public function testOnDependencyRemoval() {
    +  /**
    +   * Test the validation.
    +   *
    +   * @dataProvider providerTestValidation
    +   */
    +  public function testValidation($value, $violation_count) {
    +    $typed_config_manager = \Drupal::service('config.typed');
    +
    +    $violations = $typed_config_manager->createFromNameAndData('core.entity_form_display.node.article.default', [
    +      'targetEntityType' => 'entity_test',
    +      'bundle' => 'entity_test',
    +      'mode' => 'default',
    +      'id' => $value,
    +    ])->validate();
    +    $this->assertCount($violation_count, $violations);
    +  }
    +
    +  /**
    +   * Data provider for ::testValidation.
    +   */
    +  public function providerTestValidation() {
    +    return [
    +      'Valid machine name' => [
    +        'foomachinename',
    +        0,
    +      ],
    +      'Invalid machine name' => [
    +        'invalid machine name',
    +        1,
    +      ],
    +    ];
    +  }
    +
    

    Generally unhelpful comment: This test looks almost identical to ~5 others added in this patch. I don't think there's anything we can do about that, but it strikes me as a bit of a bummer to be duplicating so much code like this. Probably a shared test trait somewhere is too much trouble. But I wanted to point it out in case anyone working on this patch thinks it's worth reconsidering.

11. +++ b/core/modules/shortcut/config/schema/shortcut.schema.yml
    @@ -5,8 +5,12 @@ shortcut.set.*:
    +          message: This value should be a valid machine name. Valid machine names contain only alphanumeric characters and dashes.
    

    Again here

Also, https://www.drupal.org/node/2954832 could use some help. There's malformed HTML in the example for machine names. It's also not clear why that CR wasn't already published, and why this issue is adding to it. Can someone more in-the-know on this take another look?

Thanks!
-Derek

"This value should be a valid machine name, containing only alphanumeric characters and underscores."

I bet it not always true, I recall few forms altering the pattern

@andypost re: #82: Note, there are different regexps (and messages) for a bunch of different cases in the patch. I only put that example in one of my review points, but see the quoted code in points 2, 3, 5, 6 and 11 for different patterns the patch is handling...

@dww Thanks for review, back to needs work to deal with that.

I agree with pretty much all those changes. For #81.10 I think we can try to make an abstract test base class (e.g. similar to Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase) that can carry out config validation and that is extended into concrete classes for each specific type that we want to test.

Closed #2845517: Enforce allowed characters of machine names in config entities. as duplicate

And #2824519: User role machine name created with caps and spaces

Added reroll of patch #72 on Drupall 9.4.x.

Resolved CS issues.

Hah, I wrote something very similar over at https://git.drupalcode.org/project/drupal/-/merge_requests/3047/diffs?co...!

I like this approach better though: it's better to stop using this generic type: string 👍

I'll get this reviewed/moving forward again.

P.S.: changing parent, because this validation constraint would be helpful not only for config entities, but also config in general.

+++ b/core/config/schema/core.data_types.schema.yml
@@ -82,6 +82,14 @@ uuid:
+      message: This value should be a valid machine name. Valid machine names contain only alphanumeric characters and underscores.
+      pattern: '/^[a-z0-9_]+$/'

@larowlan over at https://git.drupalcode.org/project/drupal/-/merge_requests/3047/diffs#no... pointed out that this is not actually correct — he wrote:

to make this as reusable as possible making it customizable is a must, e.g. menus machine names allow a - but not a _

TIL!

I can take this on.

From some basic research, I think it's not far off. It definitely needs to be customizable, since some config entity forms use different logic for machine names (i.e., they'll replace periods in input, whereas other forms won't).

It could make obsolete #2091871: Add an ConfigEntityForm with an exists() method

OK, merge request opened. I don't think we need a new constraint for this; it's something we can implement with the existing Length and Regex constraints.

Most machine names default to allowing lowercase letters and numbers, as well as underscores, with a maximum length of 64. Those are the constraints implemented on the new machine_name data type, and tested in ActionValidationTest.

These things can be overridden. In menus, the id field allows lowercase letters, numbers, and dashes (not underscores), and has a maximum length of 32. This is also tested in MenuValidationTest.

So, not sure what else is needed here. Kicking it into review mode!

Nits on the MR, but more importantly, it fails with this:

You are using the deprecated option "--no-suggest". It has no effect and will break in Composer 3.
> Drupal\Composer\Composer::ensureComposerVersion
Installing dependencies from lock file (including require-dev)
Verifying lock file contents can be installed on current platform.
- Required package "drupal/core" is in the lock file as "10.1.x-dev" but that does not satisfy your constraint "self.version".
- Required package "drupal/core-project-message" is in the lock file as "10.1.x-dev" but that does not satisfy your constraint "self.version".
- Required package "drupal/core-vendor-hardening" is in the lock file as "10.1.x-dev" but that does not satisfy your constraint "self.version".
This usually happens when composer files are incorrectly merged or the composer.json file is manually edited.
Read more about correctly resolving merge conflicts https://getcomposer.org/doc/articles/resolving-merge-conflicts.md
and prefer using the "require" command over editing the composer.json file directly https://getcomposer.org/doc/03-cli.md#require-r

… I think this a DrupalCI issue? 🤔

No idea what the hell is going on with CI, but I addressed your concerns anyway. :)

They were known DrupalCI issues that should since then have been resolved. Re-testing to check if that's true 😊

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

It looks like our excellent progress pace on config validation was indeed killed by an ephemeral DrupalCI issue 😬🤬😞 Let's get it back on track!

This MR looks great and very close! 🤩

Marked Needs work for:

1. Stopping repeating identical copies of testMachineName().
2. Documenting why some cases deviate from the default regex or max length — just @sees would do!
3. Using type: machine_name in more places: if I grep the codebase for label: 'Machine name', I find several more that should be updated here: filter.format.*, taxonomy.vocabulary.*, views.field.machine_name, views.view.*.

Regarding #107:

Using type: machine_name in more places: if I grep the codebase for label: 'Machine name', I find several more that should be updated here: ..., views.field.machine_name, ...

views.field.machine_name is a boolean, not a machine name. Despite the confusing label, it is unrelated to the change we're making here.

Hiding patches in favor of the merge request.

Looks superb!

CR updated.

Let's do this!

Back to Wim to revert one small comment change which is not correct.

#112: done.

If we apply #3324984-34: Create test that reports % of config entity types (and config schema types) that is validatable to get a sense of how much difference this makes for total config validatability — and use #2164373-37: [META] Untie config validation from form validation — enables validatable Recipes, decoupled admin UIs …'s 2023-01-11.txt as a baseline:

• 24.20% → 29.81% average config type validatability
• 🟡 29% → 31% block.block.*
• 🔴 0% → 🟡 20% block_content.type.*
• …
• ℹ️ 35.77% → 38.32% validatable property paths (196 → 210 of 548 property paths — this excludes property paths)

and

• ℹ️ 4.62% → 4.77% validatable (29 → 30 of 628 → 629 config types — excludes base types)
• ℹ️ 25.95% → 26.33% average config type validatability
• ℹ️ 36.74% → 37.11% validatable property paths (1371 → 1386 of 3732 → 3735 property paths — this excludes property paths for base types)

Clearly a solid step in the right direction 😊

I need re-review here after adding some entity types that we forgot.

I didn't mind too much that we didn't get everything. We're very likely to miss things anyway, because core doesn't yet have thorough test coverage for valid configuration.

The main goal IMHO is to make measurable progress, and that was already the case! :) This is definitely better though 👍

git diff 0152bf9b33bc335e1eb4d866712bf62c5c41d170 8c249cd82d98ac23b5b75f5587184b55b06a6f18 (to review the changes since I RTBC'd) look great so 🚀

Added a few comments, partially based on the research previously done in #3109137: Add config validation for length of machine names

Should we also have a test that an entity can be created successfully with the full maximum length?

In some ways these tests are moot beyond testing that the constraint is applied. Like the length constraint is well tested outside of Drupal.

Whoops...

@phenaproxima addressed some concerns, I responded to some on the MR.

I think we need to review all the ones where the max length is the default 64. Adding constraints that don't respect the real limit will cause us problems in the future. I created a block with the ID claro_messagesclaro_messagesclaro_messagesclaro_messagesclaro_messagesclaro_messagesclaro_messagesclaro_messagesclaro_messagesclaro_messagesclaro_messages and could edit the block just fine.

#120: Very interesting! claro_messagesclaro_messagesclaro_messagesclaro_messagesclaro_messagesclaro_messagesclaro_messagesclaro_messagesclaro_messagesclaro_messagesclaro_messages is 155 characters, which is indeed less than the 166 that \Drupal\Core\Config\Entity\ConfigEntityStorage::MAX_ID_LENGTH allows and which is checked by \Drupal\Core\Config\Entity\ConfigEntityStorage::save().

But '#maxlength' => 64, is present in BlockForm, so … how did you create that? 🤔

@phenaproxima said this about that in chat:

adam.hoenich 1 day ago
@wim.leers About alexpott’s feedback in https://www.drupal.org/project/drupal/issues/2920678, I don’t understand what he’s asking. The ones that are max 64 characters are that way because I checked to confirm that they are supposed to be that way (which is to say, they don’t override defaults).
adam.hoenich  1 day ago
And of course Alex can create a block with a ridiculous length — the forms aren’t validating these constraints, FFS.
adam.hoenich  1 day ago
So what we have here, I think, is mismatches between the form API and its machine_name element think, and what database schema says. To me, DB schema is the final word.
adam.hoenich  1 day ago
Since that’s an absolute hard limit

But I think @longwave and @alexpott are right that despite the max lengths imposed by forms that are listed in #78, forms can be bypassed, so the only safe choice for a default is 166 aka \Drupal\Core\Config\Entity\ConfigEntityStorage::MAX_ID_LENGTH. Otherwise we will never realistically be able to enable config validation because existing configuration will be not just considered as invalid in some key-value pairs, but on its most fundamental one: its identifier! Which then has wild repercussions for everything else: all references to that config from other config (and code) would have to be updated. That's a BC nightmare.

So choosing to be pragmatic makes sense to me. I hope @phenaproxima agrees 😊

But I think @longwave and @alexpott are right that despite the max lengths imposed by forms that are listed in #78, forms can be bypassed, so the only safe choice for a default is 166 aka \Drupal\Core\Config\Entity\ConfigEntityStorage::MAX_ID_LENGTH. Otherwise we will never realistically be able to enable config validation because existing configuration will be not just considered as invalid in some key-value pairs, but on its most fundamental one: its identifier! Which then has wild repercussions for everything else: all references to that config from other config (and code) would have to be updated. That's a BC nightmare.

I agree with @Wim Leers here, we shouldn't look at the validation in the forms but at the data level.
The open Merge Request should be rebased and then I think we can get this in.

Merged in all 10.1.x commits of the last ~2 months. No conflicts 🥳

Updated issue summary with updated validatability impact analysis (previous one was in #113, almost 3 months ago): this significantly increases the average validatability of config entity types (from 24.16% to 31.24%!) because almost every config type contains at least one type: machine_name! 🤩

I was already confident in #122, so to rtbc this goes.

Needs a rebase on 11.x

Thanks everyone, this is going to be great!

There was an unresolved thread with a suggestion that looked right to me, so I clicked the "Apply suggestion" button in the GitLab UI.

I then rebased this to the latest 11.x branch (as of commit 570710ad), with no conflicts to resolve, and pushed it. What I don't have perms for is to edit the MR to change the target branch. Either @phenaproxima or a core committer must do that.

Not done reviewing, but this was RTBC so it's probably ready.

Thanks again,
-Derek

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Thanks, @dww!

The failures appear all to be be due to #3349293: Make assertions using ConfigEntityValidationTestBase::assertValidationErrors() clearer. 👍 Yay for clearer test assertions! (Also, this means that automatic re-testing appears to be broken 🤷‍♀️)

Ah, too bad — @dww, that didn't quite work 😅 You merged in commits from 11.x, but the MR is still flagged as targeting 11.x. This is a huge failure on GitLab's behalf: it's impossible for us to change the target branch 😬

Also, just creating a branch from 11.x fails because it doesn't yet exist in this issue fork. So we need to push that in from origin first… despite the GitLab UI actually showing 11.x:

(Yes, DX issues aplenty.)

I tried to learn some new git skills (thanks @larowlan! https://drupal.community/@larowlan/110471613823751109) to hopefully accelerate future painful "change target branch" parties like this one:

wim.leers at MacBookPro-WimLeers in ~/core on machine-name-validation*
$ git l
*   2a4e9295d7312723b9899e87dc98bdc8f3db9833 (HEAD -> machine-name-validation) Merge remote-tracking branch 'origin/10.1.x' into machine-name-validation
|\  
| * da2eaec5af853991cf0b2d9baeac961c5acf806e Issue #3278493 by mglaman, rabbitlair, mherchel, Purencool, dww, benjifisher, lauriii, andy-blum, AdamPS, jwilson3, 
<snip>

Now let's rebase this onto 11.x:

git rebase --onto 11.x da2eaec5af853991cf0b2d9baeac961c5acf806e

(that's the last upstream aka non-merge commit from the previous target branch)

wim.leers at MacBookPro-WimLeers in ~/core on machine-name-validation*
$ git rebase --onto 11.x da2eaec5af853991cf0b2d9baeac961c5acf806e
Committed <a href="https://git.drupalcode.org/project/drupal/commit/605dab0">605dab0</a> and pushed to HEAD. Thanks!

*** Does it need a change record? ***
*** Should it be in the release notes? ***
Committed <a href="https://git.drupalcode.org/project/drupal/commit/375f75d">375f75d</a> and pushed to HEAD. Thanks!
<snip>
Successfully rebased and updated refs/heads/machine-name-validation.

And then check it out as the new branch to create an MR for and push:

wim.leers at MacBookPro-WimLeers in ~/core on machine-name-validation*
$ git checkout -b 2920678-machine-name-validation-11x
Switched to a new branch '2920678-machine-name-validation-11x'
wim.leers at MacBookPro-WimLeers in ~/core on 2920678-machine-name-validation-11x*
$ git push --set-upstream drupal-2920678 HEAD
Enumerating objects: 527, done.
Counting objects: 100% (527/527), done.
Delta compression using up to 10 threads
Compressing objects: 100% (193/193), done.
Writing objects: 100% (439/439), 45.24 KiB | 11.31 MiB/s, done.
Total 439 (delta 320), reused 312 (delta 227), pack-reused 0
remote: Resolving deltas: 100% (320/320), completed with 61 local objects.
remote: 
remote: View merge request for 2920678-machine-name-validation-11x:
remote:   https://git.drupalcode.org/project/drupal/-/merge_requests/4209
remote: 
To git.drupal.org:issue/drupal-2920678.git
   9daaabee1e..cb0e85a688  HEAD -> 2920678-machine-name-validation-11x
branch '2920678-machine-name-validation-11x' set up to track 'drupal-2920678/2920678-machine-name-validation-11x'.

P.S.: I've been unable to figure out how to make this generate a new branch automatically. Maybe @larowlan can shed some light on that…
P.P.S.: this took me more than 40 mins of fighting git to figure out 😬

Finally, @dww mentioned in #128 that he applied one agreed upon suggestion from the original MR. Re-applying that.

Adjusted test expectations to be more precise, necessary since #3349293 — see #130.

Moving this back to rtbc after the rebase and branch switching.

Actually moving status

As per #47 blocks allow dots in their machine name, but I don't see an override for the default regex in block.schema.yml.

I haven't checked the other entity types, but this is one special case that I was previously aware of.

Darn, great catch! Looks like we missed that one 🙈

Addressed #47 / #138 👍 (Explicit test coverage added.)

test failures in the MR seem legit to the task at hand.

Done!

Turns out that the tightening of test coverage I did in bd31ada0 made it apparent that we were missing explicit test coverage for the atypical behavior in Menu and ShortcutSet too — not just in Block.

LGTM now!

Issue credits

Committed 0a7c337 and pushed to 11.x. Thanks!

Nice work folks 🏎️

Thanks for the feature, but this needs to be added to the config schema reference page https://www.drupal.org/docs/drupal-apis/configuration-api/configuration-... otherwise it will mostly remain unused, so resetting to NW and tagging as needs documentation for now.

TIL that documentation even exists 😅 I've never once in my life found that page! 🤯

@Wim Leers, that's what happens when one knows too much :-)

@fgm Or when I've grown to assume docs are missing/inconsistent/incomplete and just figure it out by grepping the codebase 😇 I wouldn't say I know too much, I'd say my search-fu has been trained a LOT 😜