Add config validation for the allowed characters of machine names

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.

This MR looks great and very close!

Marked 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 📌 Create test that reports % of config entity types (and config schema types) that is validatable Postponed 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

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 📌 Make assertions using ConfigEntityValidationTestBase::assertValidationErrors() clearer Fixed . 👍 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 🏎️

larowlan → closed merge request !4209

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 😜

Automatically closed - issue fixed for 2 weeks with no activity.