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 📌 Mark config schema for CKEditor 5 text editor settings + each configurable CKEditor 5 plugin as FullyValidatable Active 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.

A few requests for comments, but I think this is looking quite good!

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. 👍

Only a couple of questions but otherwise I'm quite happy with this!

🥳 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 :)

All my complaints and points are resolved. +1 RTBC! #shipit

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 ✨ Follow-up for #3324150: allow specifying a prefix for ConfigExistsConstraint, to enable using it for references to config entities Needs review
• Extracted ✨ New config schema data type: bytes Needs review

✨ Follow-up for #3324150: allow specifying a prefix for ConfigExistsConstraint, to enable using it for references to config entities Needs review landed, rebased MR 👍

✨ New config schema data type: bytes Needs review also landed! Rebased MR again. Another 3 fewer files changed.

Everything in here makes sense to me. The weird parts are well-explained and commented. I feel pretty good slapping the RTBC on this!

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 👍

Liking the new test coverage! I have nits, and only one is blocking (an assert that doesn't actually assert anything). But otherwise, happy to restore RTBC once the one thing is fixed.

Good remarks :)

As promised in #33.

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 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!

catch → closed merge request !6026

This unblocked ✨ [PP-2] POST/PATCH config entities via REST for config entity types that support validation Needs work 🚀

Will definitely create that follow-up 👍

This also enables us removing some serious technical debt: 📌 Deprecate \Drupal\ckeditor5\Plugin\Editor\CKEditor5::validatePair() and `type: ckeditor5_valid_pair__format_and_editor` Active . 👍

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

Follow-up here 📌 ckeditor5 and editor module tests rely on hook_editor_presave() bc layers Active .

@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 🐛 editor_post_update_sanitize_image_upload_settings fails Needs review .