Add basic validation to config schema definitions

LOTS of failures like this:

Drupal\Tests\history\Kernel\Views\HistoryTimestampTest::testHandlers
LogicException: Config schema type "filter_settings.filter_html" has a circular type reference, where it uses the type "filter_settings.[%parent.id]".

👍

The code needs refining, but I'd first like feedback from a subsystem maintainer before spending many more hours on this (this already took me 2–3 hours).

I would like an explicit +1 that this change would be welcomed.

I wonder if this is something that we need to add to TypedConfigManager, or somewhere else,

What would the other possible location be? 🤔

Maybe the config.storage.schema service, which is used by TypedConfigManager? That would force it to happen at a lower level/earlier time, which may be better?

See #5.

@kostyashupenko I know. But the point is that this MR should NOT chase HEAD. See #10 and #5.

Thanks for the rebase though! :D

FYI the actual fixes that this MR makes are also in #3404431: Filter settings schema types are incorrect. This issue aims to add infrastructure to prevent similar problems.

#3404431: Filter settings schema types are incorrect landed

All feedback except one piece has been addressed, and tests are now passing!

Moving on to the last piece of feedback… which is also the only @todo I had added 👍

The last piece of feedback/the @todo I had added was to not just run this validation for the filter_settings.filter_html config schema type, but for ALL config schema types.

Doing this seemed to work fine locally, but caused 254 test failures on GitLab CI. They all look like this:

LogicException: Config schema type "core.entity_view_display.*.*.*.third_party.layout_builder" has a circular type reference, where it uses the type "[%parent.%parent.%type].third_party.[%key]".

Turns out that #2942661: Sections should have third-party settings introduced the ability to define third party settings on Layout Builder Sections. So the layout_builder.section type got this addition:

    third_party_settings:
      type: sequence
      label: 'Third party settings'
      sequence:
        type: '[%parent.%parent.%type].third_party.[%key]'

(Copy/pasted from type: config_entity.) But this is wrong; it should have used the same pattern as for example field formatter settings (which are stored in EntityViewDisplay config entities aka core.entity_view_display.*.*.*) getting third party settings; they used type: field.formatter.third_party.[%key]. Similarly, here we needed:

        type: 'layout_builder.section.third_party.[%key]'

Why is this necessary? Because:

1. Layout Builder stores its settings as third party settings on Entity View Displays, at core.entity_view_display.*.*.*.third_party.layout_builder", which is only one of many possible third party settings on entity view display config entities — all config entities allow arbitrary third party settings using [%parent.%parent.%type].third_party.[%key]
2. That is a mapping containing a sections key that is a list of Layout Builder Sections (i.e. layout_builder.section)
3. And because #2942661 added third party settings to type: layout_builder.section using that same [%parent.%parent.%type].third_party.[%key], the grandparent/two levels up (%parent.%parent) does not refer to the root of the config entity and uses its type (core.entity_view_display.*.*.*), but instead it refers to … layout_builder.section itself! 👈 this is the circular reference! ⚠️

Next up: test coverage. This needs test coverage using a pattern similar to the one in \Drupal\KernelTests\Config\Schema\MappingTest::testInvalidMappingKeyDefinition().

Tried to write tests.

The more representative test coverage pattern to copy is actually \Drupal\Tests\ckeditor5\Kernel\CKEditor5PluginManagerTest::testInvalidPluginDefinitions().

Writing test coverage for this is very complicated because of the hardcoded assumptions in ExtensionDiscovery. See \Drupal\Tests\ckeditor5\Kernel\CKEditor5PluginManagerTest::mockModuleInVfs(), where I previously had to deal with this. 😬 To be continued tomorrow.

🏓

It's very obscure, complicated logic

It's not obscure, but it is complicated. To understand the logic, you have to understand how config schema type definitions. Once you understand that, it should no longer be obscure. And in fact, this logic should help make config schema in general less obscure for the next person 😅

Hahaha, not at all — no worries! 😄 What you're saying in #25 is the same as what I said in #24. We agree that knowledge about the config schema system is obscure (hard-won) knowledge, that we're trying to make more accessible in this issue by removing some of it 👍

The tests were hard to write — I was able to copy most of the pattern in \Drupal\Tests\ckeditor5\Kernel\CKEditor5PluginManagerTest::mockModuleInVfs() but not everything. Which also led me to push #2961541 forward again — see #2961541-26: ExtensionDiscovery fails to scan the vfsStream filesystem for modules or profiles.

All feedback addressed 👍

Remaining feedback addressed!

@phenaproxima I'm hoping you can close 3 of the 4 threads 🙏 (one is a committer FYI and can remain open)

Addressed @alexpott's MR feedback. That resulted in an API addition (Extension::getAbsolutePath()) and a change record: https://www.drupal.org/node/3420082.

#3392903: Validate inputs of TypeResolver::resolveExpression(): only allow %parent, %type and %key landed!

I won't have time for this soon though … so unassigning.