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