Add validation constraints to system.theme

Looking great! Exciting for all these. Can the MR be updated for 11.x though

Wim Leers → made their first commit to this issue’s fork.

99% ready! :)

Follow-up 🐛 [PP-1] Remove `system.theme` entry from \Drupal\Core\Config\Schema\SchemaCheckTrait::$ignoredPropertyPaths Active .

This needs the same upgrade path approach as 📌 Add validation constraints to core.menu.schema.yml Needs work - install profiles routinely ship with this file, and if they only specify the front end theme, I would expect that to fail validation? So we need to auto-fix it when config is saved, but also trigger a deprecation when that happens outside of the upgrade path.

Good call here too 🫣

omkar.podey → made their first commit to this issue’s fork.

Right now \Drupal\FunctionalTests\Core\Recipe\StandardRecipeInstallTest::testStandard is failing and I think it is because of the changes in core/modules/system/config/schema/system.schema.yml as we have a empty profile in the test and stark theme isn't installed.

So what should we do here ?, just install stark at the start (which seems difficult without the container) and i was wondering would we encounter a similar problem while using recipes on a real site using the empty profile.

kunal.sachdev → made their first commit to this issue’s fork.

The test is failing because in RecipeRunner::installTheme() we do

\Drupal::service('theme_installer')->install([$theme]);

and in ThemeInstaller we have

$extension_config
        ->set("theme.$key", 0)
        ->save(TRUE);

which also tries to validate the config and it breaks for ExtensionExistsContraint with error message 'Theme stark is not installed', which is true.

That's why I wonder if ExtensionExists really makes sense in the context of core.extension.

The core.extension config is the source of truth about what extensions are installed. ExtensionExists is basically validating against that, at the end of the day, and therefore it doesn't make a ton of sense for core.extension to have that constraint, does it?

@phenaproxima I think this comment ( #18 📌 Add validation constraints to system.theme Needs work ) was meant for 📌 Add validation constraints to core.extension Needs review .

yash.rode → made their first commit to this issue’s fork.

In this commit I have added fix from 📌 Add validation constraints to core.extension Needs review . We do not need to block this issue on it as whichever gets merged first is fine.

Coming along, but I think we can tighten it up a bit.

Looking pretty good; a few more suggestions. I think we also need test coverage of the changes to ExtensionExistsConstraintValidator.

Added test coverage for the changes to ExtensionExistsConstraintValidator. Also we have update path and its test so removing the related issue tags.

I have very few remaining complaints here! And frankly, they're pretty minor.

Ship it.

I left suggestions and some questions in the MR and I tried to resolve many of the threads.

I would accept some of the comment suggestions but I have to leave. So, setting to needs work for comments.

Addressed @quietone's feedback. Everything was comment changes, but I decided to also make the event subscriber final, merely to be explicit about the intention. Since that's not a substantive change, I'm going to tentatively restore RTBC here in the hopes that tests will pass.

There are Merge conflicts now.