Make NodeType config entities fully validatable

help and description need validation constraints too. It's not that because they're optional that we accept any value for them.

I'd like to see explicit test coverage for trying to set preview_mode to null, because I'm not sure if that would be cast to 0 during validation at some point? 🤔

They're type: text … do we want invisible control characters (think \x08 for backspace: https://www.asciitable.com/) to be accepted?

Opened #3379102: Add validation constraint to `type: label` + `type: text`: disallow control characters for this.

Making help and description not nullable implies making them required? 😅

Yes, but IIRC \Drupal\Core\Validation\Plugin\Validation\Constraint\NotNullConstraintValidator::validate() automatically maps '' to NULL 🤓

#9: Oops, you're right! I was confusing it with StringItem which does implement ComplexDataInterface. Since this is (typed) config, this is StringData instead. My bad!

But … I don't think defaulting help and description to the empty string makes sense? That means we'd end up with node.type.*.yml files containing:

name: Article
type: article
help: ''
description: ''
…

Wouldn't it then be clearer to have this instead?

name: Article
type: article
help: null
description: null
…

IOW: I think NotBlank would be conceptually more clear than NotNull. Requiring a value (i.e. not NULL) but then accepting the empty string feels rather odd to me? 🥸

I'm questioning whether that default config makes sense.

To allow other Drupal contributors to actually grok #13:

@phenaproxima wrote in Slack, in response to #12:

OK, I don’t see how it doesn’t make sense. These are just optional strings. null would be legit, as would '' in my opinion

So I don’t really care how we approach them, but I cannot for the life of me see what other constraints would be useful.

to which I responded this:

The point is that description: '' (the empty string) makes it seem like there’s an actual description present, but there isn’t. All we’ve done is satisfy the requirement that it’s a string.

So on the one hand it’s a “required” value (because we’re not allowing null, which is why you changed it to '' in your latest commit).

This conveys “every Node Type MUST have a description”. Okay! 👍

But on the other hand we allow a value that is completely pointless, because the empty string is NOT a reasonable description! 👎

That’s why I’m saying it would be clearer to have description: null, because that at least makes it clear that it is indeed optional.

Any time something is optional, it should allow null. That’s literally why \Drupal\Core\TypedData\TypedDataManager::getDefaultConstraints() has:

// Add the NotNull constraint for required data.
    if ($definition->isRequired()) {
      $constraints['NotNull'] = [];
    }

IMHO the correct solution is:

1. ADD NotBlank → empty string is not allowed, because that’s nonsensical
2. REMOVE NotNull
3. Modify the logic of \Drupal\node\Entity\NodeType::getDescription() (the Entity API-level representation) to return the empty string if the config-level representation contains null, to avoid a BC break.

FYI this is itself also a blocker, for #3379091: Make NodeType config entities fully validatable.

Let’s continue this issue, because there’s nothing stopping us from rebasing that MR on top of a squashed commit of its blocker’s (#3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support) MR! 🤓

If we can get this to green or even RTBC, then it becomes more likely that the blocker (which has been RTBC for exactly 3 weeks now) gets committed! 🤞

(Also: it's clear that #3324140: Convert field_storage_config and field_config's form validation logic to validation constraints is full of dragons 🐲 that jump out of every crevice 🐉 — so getting this one config entity type to fully validatable is much more realistic and would still be a huge leap forward!)

This is impossible to review right now, because I can't create a diff relative to #3379091: Make NodeType config entities fully validatable. To allow for such diffs, this must always:

1. be rebased on top of the latest origin/11.x
2. with its oldest commit being the commit that brings in #3379091

So, did that: started from latest 11.x, cherry-picked #3379091, then cherry-picked all of the individual commits by @phenaproxima one-by-one.

+++ b/core/modules/node/config/schema/node.schema.yml
@@ -26,15 +26,29 @@ node.type.*:
+      nullable: true
+      constraints:
+        NotBlank:
+          allowNull: true
     help:
       type: text
       label: 'Explanation or submission guidelines'
+      nullable: true
+      constraints:
+        NotBlank:
+          allowNull: true
     new_revision:
       type: boolean
       label: 'Whether a new revision should be created by default'
     preview_mode:
       type: integer
       label: 'Preview before submitting'
+      constraints:
+        NotNull: []
+        # These are the values of the DRUPAL_DISABLED, DRUPAL_OPTIONAL, and
+        # DRUPAL_REQUIRED constants.
+        # @see \Drupal\node\NodeTypeForm::form()
+        Choice: [0, 1, 2]

Looks great! 🤩

232 tests fail. Looks like many of them are due to a missing update path. For example when running CKEditor5UpdateCodeBlockConfigurationTest, the failure is:

There should be no errors in configuration 'node.type.article'. Errors:
Schema key 0 failed with: [help] This value should not be blank.

Failed asserting that Array &0 (
    0 => '[help] This value should not be blank.'
) is true.

That update logic should match the one at #3380480: Views should require a label, rather than falling back to an unhelpful ID.

That was fast! 😄 Way faster than I thought possible!

I have zero remarks whatsoever 😦😅🥳

I have only one bit of magnificent output to share:

$ vendor/bin/drush config:inspect --filter-keys=node.type.article --detail --strict-validation
 Legend for Data: 
  ✅❓  → Correct primitive type, detailed validation impossible.
  ✅✅  → Correct primitive type, passed all validation constraints.
 ---------------------------------------------- --------- ------------- --------------------------------- 
  Key                                            Status    Validatable   Data                             
 ---------------------------------------------- --------- ------------- --------------------------------- 
  node.type.article                              Correct   100%          1 errors                         
   node.type.article:                            Correct   Validatable   ✅✅                             
   node.type.article:_core                       Correct   Validatable   ✅✅                             
   node.type.article:_core.default_config_hash   Correct   Validatable   ✅✅                             
   node.type.article:dependencies                Correct   Validatable   ✅✅                             
   node.type.article:description                 Correct   Validatable   ✅✅                             
   node.type.article:display_submitted           Correct   Validatable   ✅✅                             
   node.type.article:help                        Correct   Validatable   This value should not be blank.  
   node.type.article:langcode                    Correct   Validatable   ✅✅                             
   node.type.article:name                        Correct   Validatable   ✅✅                             
   node.type.article:new_revision                Correct   Validatable   ✅✅                             
   node.type.article:preview_mode                Correct   Validatable   ✅✅                             
   node.type.article:status                      Correct   Validatable   ✅✅                             
   node.type.article:type                        Correct   Validatable   ✅✅                             
   node.type.article:uuid                        Correct   Validatable   ✅✅                             
 ---------------------------------------------- --------- ------------- --------------------------------- 

(Fresh install of 11.x branch, https://www.drupal.org/project/config_inspector installed, then switching to this MR, will give you that output.)

Look at that!!! The first 100% validatable config entity type! 🤩🚀

Marking as a postponed RTBC until the blocker lands. Awesome work, @phenaproxima! 🙏

Setting status to postponed since it's blocked on #3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support which needs a rebase.

Re-reviewing this now that [#3364109 is getting closer …

+++ b/core/modules/node/config/schema/node.schema.yml
@@ -26,15 +26,29 @@ node.type.*:
+      nullable: true
+      constraints:
+        NotBlank:
+          allowNull: true

The first line here will be necessary after #3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support lands.

The last 3 lines:

1. IMHO this should be added to type: text (see #3379102: Add validation constraint to `type: label` + `type: text`: disallow control characters for earlier issue) — but then again, this is likely to trigger other issues. Could we instead create a follow-up for doing that, and point at that issue with a @todo from here to ensure we don't forget to update IF that happens? 🙏
2. confirming that this constraint makes sense — ran into that (NotBlank: {allowNull: true}) with another issue: #3404061: When setting `NotNull` constraint on type: required_label, two validation errors appear.

+++ b/core/modules/node/config/schema/node.schema.yml
@@ -26,15 +26,29 @@ node.type.*:
+        NotBlank:
+          allowNull: true
...
+        NotBlank:
+          allowNull: true

Thanks to #3404061: When setting `NotNull` constraint on type: required_label, two validation errors appear having landed, this can now be changed to merely

NotBlank: {}

👍

Whew, this ain't no simple rebase or merge! 😳

Catching this up on months of history and getting rid of the in-flux blocker that was merged in here but has now been merged upstream (see #26) is no easy task.

I came up with this:

$ git log --oneline -n 25 --first-parent
f0984b5f0c1 Fix jsonapi NodeTypeTest
ab22479ac7e Fix base class for REST resource test
d46b907e178 Make the NodeType migration destination set help and description to NULL if empty
9e29c8b053c Make NodeTypeForm explicitly set NULL for empty description and help
4d4123fcb74 Add explicit update path test coverage
732ef4b451f Fix typo
3425dc4dba7 Merge remote-tracking branch 'origin/11.x' into 3379091-make-nodetype-config
a041e788f3e Merge branch '3379091-make-nodetype-config' of git.drupal.org:issue/drupal-3379091 into 3379091-make-nodetype-config
c77d2f18b9e Add update path to Node, hopefully fixing all tests
ccae4f1431b Merge remote-tracking branch 'origin/11.x' into 3379091-make-nodetype-config
cc0c5fb04dc Same for description
6fcb09a569c Make empty help null in all node types shipped with core
b5c7ce6ebb8 Make help and description nullable, but not blank
11da9ff35c8 Merge remote-tracking branch 'origin/11.x' into 3379091-make-nodetype-config
1f4d04a920c Merge in #3379091
5ac40b2cdf9 Merge remote-tracking branch 'origin/11.x' into 3379091-make-nodetype-config
d0d54c1cb97 Fix a couple of broken tests
a4a87d179c1 Merge branch '11.x' into 3379091-make-nodetype-config
266caf476d5 Fix node.type.options_insatll_test
e5cc02f6ee4 Default NodeType::$help and ::$description to empty strings
d4583730a3c Make help and description non-nullable
09073c0e92a Add NotNull check
c32fa582a0f Make preview_mode validatable and add explicit test coverage
5052fcddde7 Issue #3377131 by longwave, smustgrave: File mode check in commit-code-check.sh is too strict
d8ec17df74c Issue #3219667 by quietone, ravi.shankar, longwave, smustgrave: Fix spelling for words used once, beginning with 't' -> 'z', inclusive

gives me the commit hashes of all commits from this MR that I care about (I could also copy/paste them from https://git.drupalcode.org/project/drupal/-/merge_requests/4535/commits, but that is a LOT of clicking and back-and-forth).

Next, create a new branch from upstream 11.x:

git checkout -b 3379091-nodetype-post-required-values

and then do an interactive rebase in which I paste the relevant commits that I want to port into the new branch (which is all of them except the ones that mention "merge"):

pick f0984b5f0c1 Fix jsonapi NodeTypeTest
pick ab22479ac7e Fix base class for REST resource test
pick d46b907e178 Make the NodeType migration destination set help and description to NULL if empty
pick 9e29c8b053c Make NodeTypeForm explicitly set NULL for empty description and help
pick 4d4123fcb74 Add explicit update path test coverage
pick 732ef4b451f Fix typo
pick c77d2f18b9e Add update path to Node, hopefully fixing all tests
pick cc0c5fb04dc Same for description
pick 6fcb09a569c Make empty help null in all node types shipped with core
pick b5c7ce6ebb8 Make help and description nullable, but not blank
pick d0d54c1cb97 Fix a couple of broken tests
pick 266caf476d5 Fix node.type.options_insatll_test
pick e5cc02f6ee4 Default NodeType::$help and ::$description to empty strings
pick d4583730a3c Make help and description non-nullable
pick 09073c0e92a Add NotNull check
pick c32fa582a0f Make preview_mode validatable and add explicit test coverage

and then invert their order (CTRL+T on macOS), to paste them into the interactive rebase.

So then the interactive rebase looks like this:

$ git rebase -i HEAD^^
pick 502c857b92a Issue #3408698 by lauriii, alexpott: Provide alter for the field typ    e UI definitions¬
pick 5aa9704ce3a Issue #3364109 by Wim Leers, effulgentsia, lauriii, phenaproxima, bo    risson_, bircher, alexpott: Configuration schema & required values: add test coverage     for `nullable: true` validation support¬
pick c32fa582a0f Make preview_mode validatable and add explicit test coverage¬
pick 09073c0e92a Add NotNull check¬
pick d4583730a3c Make help and description non-nullable¬
pick e5cc02f6ee4 Default NodeType::$help and ::$description to empty strings¬
pick 266caf476d5 Fix node.type.options_insatll_test¬
pick d0d54c1cb97 Fix a couple of broken tests¬
pick b5c7ce6ebb8 Make help and description nullable, but not blank¬
pick 6fcb09a569c Make empty help null in all node types shipped with core¬
pick cc0c5fb04dc Same for description¬
pick c77d2f18b9e Add update path to Node, hopefully fixing all tests¬
pick 732ef4b451f Fix typo¬
pick 4d4123fcb74 Add explicit update path test coverage¬
pick 9e29c8b053c Make NodeTypeForm explicitly set NULL for empty description and help    ¬
pick d46b907e178 Make the NodeType migration destination set help and description to     NULL if empty¬
pick ab22479ac7e Fix base class for REST resource test¬
pick f0984b5f0c1 Fix jsonapi NodeTypeTest¬

The new MR is green!

What does it contain?

1. All commits @phenaproxima did when he worked on this originally between August 3 and September 20, just with the merge commits omitted (see #27)
2. A commit to remove the ::testNonNullableFields() test coverage @phenaproxima added, because that landed as part of #3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support, where it was named ::testRequiredPropertyValuesMissing() (which tests it in more detail and in a more robust way)
3. A commit to actually use the infra that #3364109 added: i) use the FullyValidatable constraint on NodeType, ii) explicitly declare description and help as having optional values, to allow ::testRequiredPropertyValuesMissing() to pass
4. A commit to simplify the config schema changes: no more need to specify NotNull thanks to specifying FullyValidatable at the root!

I will RTBC this … once there is a change record. 😄

I agree with @Wim Leers, the open MR is looking great and the testcoverage that is added is great as well. The last nitpick should be simple to remove as well.

Do we need the change record for the added strictness to the schema (and will we need one for each entity type we make more strict as we go?), or what is it for?

@sourabhjain That's the second issue in a span of 10 minutes where you applied my suggestion manually locally and pushed it. Why are you doing that? 🤔

I apologize for any inconvenience, @wim-leers. I misunderstood the suggestion, thinking it was a provided task that anyone could work on. I will be more careful in the future and ensure that such misunderstandings don't happen again. Rest assured, everyone at Valuebound will be mindful of this.

Hiding patches

Left a question on the MR, not changing the status

Addressed — keeping at RTBC because it was a trivial change.

Committed to 11.x

Didn't backport to 10.2.x because this has an update path which is not allowed in patch updates.

Published change record.