Provide guidance to config schema developers: detect broken config schema types

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'm not a subsystem maintainer, but I like this - it would make schema definitions more robust. I wonder if this is something that we need to add to TypedConfigManager, or somewhere else, but that is an implementation detail.

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.

I don't think we need to add this tag. It just meant MR needs rebase and rebase is done

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

Thanks for the rebase though! :D

P.S.: another issue relating to the poor config schema DX today: 📌 [PP-1] Validate inputs of config schema's TypeResolver: only allow %parent, %type and %key Postponed .

#8, it has taken me a while to get back to this, I have though this trough, I really wanted to suggest to put the validation of the schema in a new service/class, but I think it makes most sense to put it here, where we are adding a lot of the other validation passes as well.

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

Not sure if still needs subsystem review.

But MR appears to be unmergable.

🐛 Filter settings schema types are incorrect Active 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.

This is making more and more sense.

It's extremely complicated logic, and very obscure (as the config schema system tends to be). I'm wondering if we can change the comments/method names in some places to make it as clear as possible to future developers who need to try and come to grips with this stuff.

But the logic itself, I don't have any real complaints about.

🏓

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 😅

Hopefully I didn't hit a nerve :) I meant it's obscure because config schema itself is kind of obscure. Hopefully as it becomes more "useful" (and consistent!) thanks to this work, the obscurity will disappear.

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 👍

A couple of questions but I don't think I have any problems with this in general.

Although I think I understand the code here, I'm not really comfortable RTBCing it; it needs subsystem maintainer review anyway, so that's probably who should give it the green light.

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

I'm out of complaints here.

I think someone else should give the official RTBC since I'm not an expert in the subject matter. But as far as I can tell, this is RTBC.

📌 [PP-1] Validate inputs of config schema's TypeResolver: only allow %parent, %type and %key Postponed landed!

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