Add validation constraints to core.extension

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

Most tests are failing because in TestBase setup() we do

$this->container->get('theme_installer')->install(['stark']);

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. I think we could add a check in ThemeInstaller before saving the key in the config, something like this-

if (!isset($installed_themes[$key])) {
        $extension_config
        ->set("theme.$key", 0)
        ->save(TRUE);
 }

, but I am not really sure about this.

• @phenaproxima thinks that ExtensionExists constraint doesn't make a ton of sense for core.extension see (3441503-#18) for more details.
• Also discussed this with @phenaproxima in slack and concluded that if we want to use ExtensionExists constraint for core.extension then ExtensionExistsConstraintValidator should be completely refactored and use the ModuleExtensionList and ThemeExtensionList, which can report on existing-but-not-yet-installed things.

Tests are passing now

I feel that ExtensionExists should be whether the extension exists and we should have a separate constraints - the other constraint should be called ExtensionInstalled. However this is unfortunate because we already have ExtensionExists and it's current meaning is that it is installed.

Not sure what to do.

This is a difficult question. In my opinion ExtensionExists/ExtensionInstalled makes sense, but renaming this and after that reusing the name for the other meaning is difficult. Can we use ExtensionPresent to indicate that should be present on the file system?

Believe this needs work for the feedback mentioned in #16? Also is MR 7107 needed anymore?

ModuleHandler already uses exists to check for installed. So the naming is kinda consistent.

If we need to seperate why not;

ExtentionAvailable, which would be consistent with things like; 'update available' or perhaps 'moduel available to install'. Doesn't feel that weird to me.

ExtentionAvailable makes a lot of sense to me.

Extracted the validator so naming makes sense. Split tests also and did a rebase so we run the right tests.

Latest MR appears to have pipeline issues. Can 1 MR be closed or hidden also to make it super clear what should be reviewed

Oops, thanks, juggling a lot of issues, will cycle back.

Closed MR, and fixed phpstan issue.

Left 1 question and 1 comment on the MR.

Did all the promotions, time to go home :)

all failures unrelated.

Using config inspector I see we are at 100% now.

Believe all feedback has been addressed

rebased, only to see if the unrelated failures have been fixed.

The big issue left is the new constraint - left a big comment on the MR.

Struggling with the datadefiniotion to test. Giving up for now. :)

That took a while, this typed data is a rollercoaster :x

Think i have working tests now prooving stuff works. Yay!

Next up, see if it can be rewritten to use the Composite constraint.

Not sure who made ExtensionNameConstraint but that message is useless. "The value is not valid.". That could easily be something more sane :x

Test are running, but im quite confident they will end up green.

Added __clone to ExecutionContext since that makes it possible to collect messages and then combine them in the original context (see #3461720: Create AtLeastOneOf constraint where that is also used for AtLeastOneOf)

Build has PHPCS issue. Also added some comments to the MR.

Than you so much for the review :)

Went through your (thorough) review, this made the a few things way better. I have a response to two of your suggestions which i think might have issues if we do that.

I think i adressed the last comment and made it quite a bit simpler code.

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.

2 additional remarks.

Think i addressed all feedback

Looks great!

We need to address the comment I've just added to the MR... there's a very very awkward thing with profiles. In fact the comment does not quite cover the complexity here. I think we need to detect if the profile is changing and if it is we need to rebuild the module and theme validators as this alters what modules and themes can be discovered.

Changing the install profile on an existing site is a little fraught. Things have change so that you can uninstall an install profile if you are not using any themes or modules that only exist in the install profile - see \Drupal\Core\Extension\InstallProfileUninstallValidator.

We could consider changing an install profile to another install profile to be an invalid change and only allow it to be cleared. Hmmm. Not sure.

Asked for more clarification in the MR comment (and now posted here as well, so that this issue goes back to unread).

Lets start by saying, i dont know much about the systems in play here ;)

Hmm, so my first thought would be that it should do discovery without any cache, but ending up at that not being really a thing. Lets see if i understand correctly.

We need to validate the extention, BUT, since profile is also changed it might have different modules available. So even if we say; when the profile changes, we reload the list or rescan for available modules. BUT this validation is before the changes have persisted. So it gets worse, in the fact that it needs to scan for the future situation.

In my head i have a few options;

1. Make an extended version of this validator that also checks for value of profile, use that to check if the value has changed, if so, do a clean discovery of the available modules and use that for validation.
2. Use your suggestion that allows for modules that are also profile to be installed. But that could lead to trying to install a module that is name as a profile efven though it doesnt exist?
3. Don't allow profile changes, but that seems kinda hard.

Re profile changes - we already only really allow the profile to be set by the installer and unset though module uninstallation after checking the modules - see \Drupal\Core\Extension\InstallProfileUninstallValidator. In any real way we don't actually support making changes to core.extension directly via config tools. So the test that I've commented on is pretty funky. I do think that 1 is probably the best solution for module and theme validation. I.e if the profile has changed use a fresh discovery object. Wrt. to enforcing install profile change rules... I'm not sure. I guess we can leave it alone until it causes us problems.

"I'm not sure. I guess we can leave it alone until it causes us problems."

Not sure what you mean with leaving it alone. Ignore the weird test case where profile is changed and module is changed?

Re

"I'm not sure. I guess we can leave it alone until it causes us problems."

What I mean is we can ignore trying to validate profile beyond a profiles existence for now. I think we should not be changing that test in this issue. The fact we are shows we've got stuff to fix.

Think i might've found a way by adding special case for core.extension. Lets see how tests go.

Form slack

alexpott
  16 minutes ago
Yeah but it doesn’t work




4:53
The problem is profile supplied modules
4:53
Changing a profile changes which directories module and theme discovery looks in (edited) 


bbrala
  15 minutes ago
Oh yeah


alexpott
  15 minutes ago
The install profile setting is a fundemental setting that is extremely powerful.


bbrala
  14 minutes ago
Grmbl, ok back to the drawing board.
This wouls fail when there is an eztention that would only become available when the profile is loaded. Right?


alexpott
  14 minutes ago
Yes
4:56
We need to detect profile change and then use fresh extension discovery objects with the correct profile directory in the constructor.  In this case we can not use the discovery from the container because we need to look at what will happen when the install profile is changing.
4:57
Also just for fun you’ll have to cope with the case when the install profile value is being unset. Because this is actually the only change possible via the UI or API.
4:58
And by API I mean the module install API - the direct config API should not really be used for core.extension.


bbrala
  10 minutes ago
Guess i'll start designing 2 tests for that then. Should be able to reuse some other profile for that.


alexpott
  7 minutes ago
Well this test is a test of sorts…  testing is hard because we also allow stuff in test land that’s not normally allowed… see \Drupal\Tests\drupal_system_listing_compatible_test\Kernel\SystemListingCrossProfileCompatibleTest


bbrala
  6 minutes ago
Hmm ok
5:05
Ok, then i'll just go back to my first idea, and indeed find out how to reload/reinstantiate the *ExtentionList with the changed profile if applicable and validate against that.
5:06
New root, and no cache i'd assume.
5:08
and install profile it seems


alexpott
  1 minute ago
I think that that is way to go. I don’t think relaod/reinstantiate… just instantiate… see \Drupal\Core\Extension\InstallProfileUninstallValidator::getExtensionDiscovery
5:08
I think using file cache / info parser is fine tbh.


bbrala
  Just now
thanks, that helps. Feels like it is possible :slightly_smiling_face:

Lets see how this works when we forcefully reinstantiate the discovery mechanisms.

If this works, i think i will split it into a seperate constraint maybe. Since then we could just instantiate the new ExtentionList classes every time instead of doing the check. Which would also mean i can just inject the services in the create method with having loads of extra services in the ExtensionAvailableValidator

Those services were a little involved to instantiate. Pulling in a lot of stuff from the container. I could then remove the test changes, but i did need to update a theme test that was loading a module from a profile that is not installed.

Think this is working as expected now thanks to the guidance of alex.

Note: testfailure is unrealated and does pass locally.

but i did need to update a theme test that was loading a module from a profile that is not installed.

Does this signal the need for an upgrade path, or is this because we do weird things with the testing profile?

This signals the test is trying to set a module from outside its installed profile. Should be a weirdness with the teatsetup as talked about with alex

The current approach is not going to work unfortunately. Because the ExtensionList objects interact with state and cache so we're in danger of affected the live running of a site if we validate a core.extension object prior to it being changed. I think we have to follow InstallProfileUninstallValidator's example and use a lower level ExtensionDiscovery object to determine if modules and themes exist if the profile has changed.

Ok fair enough.

Updated to propere extension discovery.

Had to add a testing env check because there are different rules there what modules you can install :(

All good now i think (and green tests)

There's a little bit more we need to cope with.

We need to test what happens when the profile is NULL - that’s the starting state. And when it is complete removed - that’s what happens when the install profile is uninstalled.

In the case that the profile is not set I think we'll need to set the directories to something that cannot exist - like we do in \Drupal\Core\Extension\InstallProfileUninstallValidator::getExtensionDiscovery

Also I think that \Drupal\Core\Extension\Plugin\Validation\Constraint\ExtensionAvailableConstraintValidator::$extensionDiscovery should be an array of extension discoveries keyed by profile as this is easy to reason about. !in_array($profile, $this->extensionDiscovery->getProfileDirectories()) looks interesting and fragile.

Added tests for different situations and verfied install_proffile parameter is all types we discussed. Unfortunately i couldnt get unit tests working, so ended up added an extra method to disable the fact it is inside a test.

Applying MR and using configuration inspector went from 5% to fully validated. Amazing!

There's 1 thread open but overall believe this is good to go.

A recent core release has shown how dangerous mixing private self and protected statics with self:: is. Let's not repeat the mistake...

Addressed my own MR comment.

Back to rtbc.
Since this is not a final class I guess the use of a private function was not really appropriate to start with?

Some good improvements, thanks alex.

I'm checking all RTBC issues to ensure their new tests use attributes. Because these changes are minor and the tests are passing I'm going to leave the issue at RTBC.

Thank you for this! I think this might belong in the extension system queue.

I'm not quite sure i want to pull this out of rtbc yet, but I do have several questions.

1. Most important, what happens if we get this wrong? This feels particularly gnarly and looks like it could break things spectacularly if we don't do this correctly.

I ask because this is dealing with extension discovery and that is very complicated and very crucial.

2. Is there a write up on why we want to make everything fully validateable? I would like to understand better the goals. I think it's #3427641: [meta] Config validation for a more reliable Drupal + reliable Recipes from the start

3. If someone has invalid config now what happens? Can they do anything before fixing it? Will their site break? Does it interfere with config export and import?

4. For this config in particular, is there a way to do it like a deprecation with messaging instead of a hard constraint right away?

I remember invalid permissions being a particularly tricky config issue to solve for some sites, since this is install and uninstall time i think the total is even higher.

I think I've asked enough questions that I've convinced myself to pull this from rtbc.

Finally there are a few threads on the MR that have not been addressed since it read at to rtbc.

Sorry I didn't see this sooner.

Just rebased to see if the new deprecation for Kernel tests missing the #[RunTestsInSeparateProcesses] attribute triggers as expected. #3546029: Ensure that #[RunTestsInSeparateProcesses] attribute is added to all Kernel tests

Sorry for the noise.

Thanks!

For 1:

However… does it really make sense to run a site with core.extension that is incorrect/nonsensical/lying?

No, but don't we want to give people a chance to clean it up so why wouldn't we follow the normal deprecation pathway?

Wouldn't you want to be warned while developing, to prevent it being deployed to production?

Yes, does the deprecation warning not show up?

Note that this MR is not making core.extension:profile strict, only core.extension:module and core.extension:theme. And modules and themes should really exist on disk, which is all that ExtensionAvailableConstraintValidator is checking and its test coverage seems solid?

Yes, but we rely on profile extension discovery here so if we got something wrong the validator will think an extension doesn't exist when the system actually knows about it. That is what concerns me here, there was a bit of churn around that point as far as I can see.

(@borisson_ shared in-person the anecdote that in his Drupal dayjob, they NEVER uninstall modules and remove it from the codebase in a single deployment for clients. Because that'd make it impossible to run the uninstall hooks!)

Neither do I in general, I've certainly seen it done unintentionally, and this would break more than just not running the uninstall hook right?
Since Drupal now thinks the module is installed, but it's not and config import can't run your site will blow up, right? So this makes the catch 22 break until you add the files back and run the import again. Where previously you just missed the uninstall hook which you could fix in the same way but your site would load at least.

For 2:
I'll read that issue in more depth when I have a moment.

For 3:
My concern is turning this on in a minor without deprecating for notice to give people a chance to resolve the issues.

I talked with @alexpott and @Wim Leers at drupalcon about this.

Config validation does not run during normal operation of a site. And these checks are also replicated in the installer.
When using a normal workflow to do imports and exports of config, the importer already has several special checks for the core.extensions and it will already yell if a file is invalid.

This is replicating a lot of that logic (but not all).

If you can point us to a place where the validation happens during import/normal runtime that affects this - please set this back to needs work. I think all the other points in this MR have been answered so putting it back in rtbc.

replicated in the installer.

This is not quite correct - they are replicated in the ConfigImporter - see all the code in \Drupal\Core\EventSubscriber\ConfigImportSubscriber

Ok I think I was able to test this in a way that helps me answer my concern.

Here is what I did to set up a config database and config for testing:

1. Fresh install of 11.x
2. Export database with all default modules installed
3. Uninstall page_cache
4. Export config

Test 1

1. Checkout 11.x
2. Delete page_cache module from disk
3. Import database
4. Clear cache
5. Run config import, no error

Test 2

1. Checkout this branch
2. Delete page_cache module from disk
3. Import database
4. Clear cache
5. Run config import, no error

Test 3

1. Checkout 11.x
2. Delete page_cache module from disk
3. Import database
4. Add page_cache to extensions manually
5. Clear cache
6. Run config import, error that page_cache is missing

Test 4

1. Checkout this branch
2. Delete page_cache module from disk
3. Import database
4. Add page_cache to extensions manually
5. Clear cache
6. Run config import, error that page_cache is missing

As far as I can tell the behavior I was concerned about does not happen and there seems to be no behavior difference between 11.x and this branch so we can remove my objection for that bit.

I also took a look through the validation pretty carefully and as far as I can tell that bit is correct too.

There are deprecation failures.

Tried fixing the tests by adding #[RunTestsInSeparateProcesses].

There's one test failure remaining that is weird and is also happening on my local against HEAD, so it's likely unrelated.

Drupal\Tests\layout_builder\FunctionalJavascript\BlockFilterTest::testBlockFilter
Failed asserting that actual size 3 matches expected size 2.

core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFilterTest.php:110

BlockFilterTest seems unrelated, since it was annotation only i'm setting back to RTBC

Well BlockFilterTest passes now, so I guess it is just super flakey, even locally.

Committed 690bbe4 and pushed to 11.x. Thanks!

This added a usage of the contact module which is being removed from core. I don't see contact mentioned here, so I assume we just picked a random module? See #3557138: Remove use of Contact from ExtensionAvailableConstraintValidatorTest

Yeah was a random module.