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 📌 [PP-1] core.extension should be validatable Postponed .

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

In this commit I have added fix from 📌 [PP-1] core.extension should be validatable Postponed . 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.

This needs another rebase.

Rebased and updated one deprecation message to 11.2.0 since we moved past 11.1.0

Appears to have a pipeline issue. Would you say the 2 threads are good to resolve? If they are and you can't I can do it for you.

Fixed the phpstan issue.

Also;

1st comment is Wim telling what this brings us, which is not an issue to resolve.

@quietone did leave feedback, but later wanted to commit so feel like she didnt feal to strong about it. Keeping is open to get extra attention from the committer here.

Actually I think this can go back to rtbc

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased.

ThemeAdminUpdateTest is failing.

Fixed the tet error. But getting phpstan errors on function t(...) which is weird, since it is fine o_O

Tried rebasing, updating 11.x in fork, not trying empty commit. Weird...

Head is broken on phpstan.

Head fixed, still a test failing, but it seems so unrelated...

Rebased, all green. yay.

Before

After

Resolved what threads I could but restoring status

Credits

Left some comments on the MR, thanks for working on this one