Filter settings schema types are incorrect

+1!

This is technically a duplicate of #3401837: Add basic validation to config schema definitions, 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 👍

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 🤞

Wow, that worked! 😃

Actually … I think this needs an update path + test.

My suggestion in #8 only works because it affects new text formats, and that happens to be the only thing being tested.

We need to ensure everything is in a consistent state, which means making sure all FilterFormat config entities contain the missing data. Fortunately, it's simple enough to populate those 2 property paths for each filter in each format.

Thanks to filter_post_update_sort_filters() and \Drupal\filter\Entity\FilterFormat::preSave() in #3017054: Consistently sort filter formats to simplify config exports there is a very recent example in the same area 👍 Back to you, @claudiu.cristea 😊

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 #3401837: Add basic validation to config schema definitions 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.

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?

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

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 #3395562: Add validation constraints to filter.settings
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 #3401837: Add basic validation to config schema definitions.

Thanks for the explanation.

Committed d8c4e4c and pushed to 11.x. Thanks!

Also published the change record.

Thanks so much!

Pushed both unblocked issues forward:

1. #3395562-18: Add validation constraints to filter.settings
2. #3401837-21: Add basic validation to config schema definitions

Adding credit for duplicate, #2472927: Remove generic sequence schema for filter plugins,