Filter schema types are incorrect

Fix title

+1!

This is technically a duplicate of 📌 Provide guidance to config schema developers: detect broken config schema types Needs work , but there I'm trying to add automatic detection of this very problem to ensure it can happen never again (reviews + help appreciated 🤓).

But that issue is likely to take a long time to land. I think it makes sense to fix core's schema regardless of deeper configuration schema system DX/architecture improvements 👍

I came here from 💬 Slick filter schema is incomplete Fixed (weird issue there)

We don't need a new type: filter_settings_base.

I was think that it's good to have a base schema for a particular *kind* of configs

The change of filter.settings.* from type: sequence to type: mapping implies all keys must now be defined.

Which means every @Filter plugin with settings now MUST define filter.settings.<PLUGIN ID> if they want to

Consequence is this validation error now appearing for (all?) CKEditor 5 functional JS tests:

'filter_url_length' is not a supported key.

But why?

1. In Drupal core, the only config entity validation that occurs anywhere in Drupal core
2. The Text Format & Editor UI itself generates configuration that does not conform the config schema: \Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5TestBase::createNewTextFormat() uses the UI to generate a new text format, resulting in filters.filter_url being set to:
   

   [
     'status' => 0,
     'weight' => '0',
     'settings' => [
       'filter_url_length' => 72,
     ],
   ]
   

3. ✅ The settings value complies with type: filter_settings.filter_url 👍
4. ❌ But that entire value (the entire array) should comply with type: filter, which it does NOT: id and provider are missing. That's why config schema was unable to resolve type: filter_settings.[%parent.id] to filter_settings.filter_url and instead fell back to filter_settings.* 😱
5. Or annotated in the debugger:

AFAICT we have two choices:

1. tighten scope: drop the change to filter_settings.*, to allow it to continue to accept arbitrary garbage values
2. broaden scope: fix \Drupal\filter\FilterFormatFormBase

Better to fix all the broken things that are connected IMO, so attached a suggested interdiff that AFAICT should fix it 🤞

I went with #8.2 as it avoids adding any arbitrary sequence but used fields of type value. Let's see the bot

Fixing IS

Wow, that worked! 😃

There are a couple of remaining failures in the javascript test coverage, they seem to be related (as they fail in ckeditor / filter things).
But I agree with @Wim Leers, this issue is great!

Removing tags that have been implemented already.

Debugging the failing Functional JS CKEditor 5 tests…

e2ac97aff5ebcf0b7a226d1749036b8fd06f2c98 caused this regression. We still need the changes in FilterFormatFormBase that I proposed in #8.

The reason: FilterFormat::preSave() does not get called until after validation happens.

@claudiu.cristea independently discovered and diagnosed this issue after I opened the broader scoped 📌 Provide guidance to config schema developers: detect broken config schema types Needs work to forever detect and prevent this bug pattern from getting reintroduced.

I only helped with debugging the failing CKEditor 5 JS tests (see #8 for details) and provided a trivial suggestion … that happened to work on the first try :D

@claudiu.cristea also provided an update path + tests, to automatically fix all of filter.format.* config objects on all sites, to add the missing information (id + provider).

This looks excellent, so RTBC'ing 😊

I'm triaging RTBC issues → . I read the IS, the comments and the MR. I didn't find any unanswered questions.

I left a comment on the MR. I would have made the change myself but maybe someone here has a better one line summary.

And, correct me if I am wrong but I also think it prudent to have a change record.

Sorry, but back to NW for two items.

@quietone, thank you. I’ve applied the suggestion but I don’t see the need for a CR, we’re fixing here a bug. No idea what I could write in a CR

Are we sure we don't need a change record?

--- a/core/modules/media/config/schema/media.schema.yml
+++ b/core/modules/media/config/schema/media.schema.yml
@@ -119,7 +119,7 @@ media.source.field_aware:
       label: 'Source field'
 
 filter_settings.media_embed:
-  type: filter
+  type: mapping
   label: 'Media Embed'
   mapping:
     default_view_mode:

If contrib or custom code that adds a filter setting schema has made the same mistake, we need them to update from filter to mapping, no?

If contrib or custom code that adds a filter setting schema has made the same mistake, we need them to update from filter to mapping, no?

Yes, they should all change. You're right that they likely all copied the wrong pattern from Drupal core. CR to announce this: https://www.drupal.org/node/3419181 → .

And if existing code has made that mistake, will there be any side effects anywhere if we commit this?

No, zero side effects in production. This only:

1. blocks making filter settings validatable in 📌 Add validation constraints to filter.settings Fixed
2. impacted https://www.drupal.org/project/config_inspector → .

To prevent such circular config schema types from being introduced accidentally in the future in core/contrib, I created 📌 Provide guidance to config schema developers: detect broken config schema types Needs work .

Thanks for the explanation.

Committed d8c4e4c and pushed to 11.x. Thanks!

Also published the change record.

longwave → closed merge request !5559

Thanks so much!

Pushed both unblocked issues forward:

1. #3395562-18: Add validation constraints to filter.settings →
2. #3401837-21: Validate config schema definitions to prevent nonsensical definitions →

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

quietone → credited alexpott → .

quietone → credited berdir → .

quietone → credited cburschka → .

quietone → credited jgeryk → .

quietone → credited subhojit777 → .

Adding credit for duplicate, 🐛 Remove generic sequence schema for filter plugins Closed: duplicate ,