[PP-1] core.extension should be validatable

Draft MR created.

Wim Leers → credited alexpott → .

Closed 📌 Improve core.extension:profile validation Postponed as a duplicate. Crediting @alexpott.

🐛 Install profile is disabled for lots of different reasons and core doesn't allow for that Fixed is in!

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.

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

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 📌 Add validation constraints to system.theme Needs work ) 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.

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

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

If we need to seperate why now;

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.

bbrala → closed merge request !7107

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 📌 Remove the restriction from RecursiveContextualValidator that prevents using custom groups. Needs review 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.

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.