Mark Editor config schema as fully validatable

I committed the blocker issue, so this isn't postponed anymore.

Something to think about here is upgrade path:

1. Sites might have active config with keys that this MR makes invalid when status=FALSE. Do we want a postupdate function to remove those?
2. There might be contrib/custom modules/profiles with default config that includes these now-invalid keys. Do we want something that runs on module-enable and/or config-import that removes them?

RE #5:

1. Yes, but … see point 2 👍 (and I don't know why I didn't do that in the original issue — shame on me — that's another great reason to have this split off 👍)
2. The correct pattern that @catch has repeatedly pointed me to is to NOT do the actual update logic in a hook_post_update_SOMETHING(), but to do it in ckeditor5_editor_presave() (for CKE5-specific things) or editor_editor_presave() (for generic Editor things) and have the post-update hook merely iterate over all Editor config entities and detect whether it needs an update, and if so, resave it. That way, install profiles are also handled! 👍

Review

1. I actually think it'd be better to split up this issue too: the Editor changes are much wider/broader than the CKEditor 5 changes (which are just marking already validatable things as FullyValidatable)
2. editor.editor.* is being marked as FullyValidatable but … it isn't really.
   

     mapping:
       format:
         type: string
         label: 'Name'
   

   is not yet validatable! We need to validate that if the value is FOO, that a filter.format.FOO config entity exists.

@effulgentsia's feedback from #5 has now been addressed 👍

Next up: #6.2.

#6.2 is now addressed.

Next up (the last thing): #6.1. Created #3416223: Opt in already validatable CKEditor 5 text editor settings + configurable CKEditor 5 plugin's settings to FullyValidatable for that. That should allow this MR to be much smaller, and only touch editor-related changes, not ckeditor5-related changes 👍

Change record created for the new functionality of ConfigExists: https://www.drupal.org/node/3416240

Addressed @phenaproxima's feedback on the MR 👍

Still to be done tomorrow by yours truly: validation for editor.editor.*:image_upload.max_size.

Ready for review again, with everything addressed 🤓

Change record created for type: bytes: https://www.drupal.org/node/3416738.

I think it makes sense to introduce bytes here, because that gives it a use already. Making max_size nullable looks like it some more fallout than expected, looking at the tests that are currently failing still?

Making max_size nullable looks like it some more fallout than expected, looking at the tests that are currently failing still?

But that's necessary, because '' is not a sensible value for "no limit". All green now — it wasn't too complicated, it was just a small oversight on my behalf 🙈

MR is green, and tighter in scope after a thorough self-review.

Issue summary updated, it is now complete. 👍

🥳 Should be RTBC'able now! 😄

Explain why type: bytes makes sense in the issue summary.

I agree with #19, this is rtbcable. In most of these issues we will need signoff from the subsystem maintainer as well - I don't think that is explicitly needed here, since Wim is Wim already :)

Moving this to needs work for the outstanding feedback on the MR.

I'm also wondering if the changes that are outside Editor module could be done in their own issue before this one?

Moving this to needs work for the outstanding feedback on the MR.

Addressed! 🏓

I'm also wondering if the changes that are outside Editor module could be done in their own issue before this one?

There's a few bits we can extract:

• Extracted #3422398: Allow specifying a prefix for ConfigExistsConstraint, to enable using it for references to config entities
• Extracted #3422407: New config schema data type: bytes

#3422398: Allow specifying a prefix for ConfigExistsConstraint, to enable using it for references to config entities landed, rebased MR 👍 3 fewer files changed :)

#3422407: New config schema data type: bytes also landed! Rebased MR again. Another 3 fewer files changed.

Fixed a few details in the issue summary.

I'd like to tighten test coverage slightly more, for maximum confidence that this will not cause regressions.

Did what I promised in #30:

• https://git.drupalcode.org/project/drupal/-/merge_requests/6026/diffs?co...
  makes the kernel test coverage more complete
• https://git.drupalcode.org/project/drupal/-/merge_requests/6026/diffs?co... adds a missing functional JS test, to test end-to-end that editor.editor.*:image_upload, whose settings are presented by the \Drupal\ckeditor5\Plugin\CKEditor5Plugin\Image plugin and still is a good citizen by reusing editor_image_upload_settings_form(), actually is A) correctly validated, B) still can be saved (🙈 in manual testing I discovered that it actually could not be saved in its prior RTBC state, because we only had kernel tests! 🙈)

Passing again 😊🚀

Now I'm confident this will not cause any problems for sites updating to this version 👍

Good remarks :)

Lovely, thank you! 😊

Needs work for one point on the MR.

The other one probably isn't blocking but feels like a good idea.

One trivial change, so re-self RTBC'ing.

The other one probably isn't blocking but feels like a good idea.

I proposed on the MR to do this for all the recent occurrences of this in a single issue, so that it all is consistent, and they all become simple to remove.

Tagged Needs followup for that, I hope you agree :)

I haven't seen this anywhere in Drupal core?

The main example is ViewsConfigUpdater, but also a whole class seems like overkill for modules that might only have one config entity update in an entire major release. Follow-up is fine.

$max_filesize = $settings['max_size']
? Bytes::toNumber($settings['max_size'])
: Environment::getUploadMaxSize();

Yes much better :)

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

This unblocked #2300677: JSON:API POST/PATCH support for fully validatable config entities 🚀

Will definitely create that follow-up 👍

This also enables us removing some serious technical debt: #3425349: [PP-1] Deprecate \Drupal\ckeditor5\Plugin\Editor\CKEditor5::validatePair() and `type: ckeditor5_valid_pair__format_and_editor`. 👍

Follow-up here #3442395: ckeditor5 and editor module test config exports/stubs rely on hook_editor_presave() bc layers.

@catch, thanks for pointing out the followup. I am removing the tag.

The upgrade path added here introduces a fatal error if a filter format has gone missing, I think just because the editor config gets resaved when it might not have been previously, this is in #3463524: editor_post_update_sanitize_image_upload_settings fails on missing text format.