Install profiles do not support multiple core branch compatibility

Should this be postponed on https://www.drupal.org/project/drupal/issues/2313917 first, so that it can expect semver compat strings too.

I think this issue should be fixed in a minor release only; while it's definitely a bug, there's a small risk of breaking an existing profile that's valid other than having the wrong key here. So since that's a small but nonzero potential disruption, we won't backport it to the production branch.

The blocker has been fixed.

Interestingly if you apply #3072702: Core extensions should not need to specify the new core_version_requirement in *.info.yml files so that 9.0.x can be installed and try and install Drupal on 9.0.x using Drush you get "Unable to install modules: module 'minimal' is incompatible with this version of Drupal core."

Note I get the same problem if I try via the UI...

I think maybe profiles need to support the core_version_requirement: VERSION too.

Not sure if this is duplicate with, or blocked by #3087416: Core version key in profile's .info.yml doesn't respect core semantic versioning, but that seems to be the issue to explicitly address @alexpott's concern in #13:

I think maybe profiles need to support the core_version_requirement: VERSION too.

Meanwhile, the patches here are nonsense patches to prove we have a problem, so 'Needs review' isn't accurate.

Given my (limited) understanding of all these issues, seems the simplest path forward is to put this one back on hold, concentrate effort at #3087416: Core version key in profile's .info.yml doesn't respect core semantic versioning, and that'll fix this bug. Re-calling it postponed for now, but maybe duplicate is better.

Cheers,
-Derek

p.s. See also #3087420: [PP-1] Update core's profile info.yml files to the new version compatibility syntax for the follow-up to actually change the core profiles to use the new info key. Not clear to me why we're not planning to include that directly as part of #3087416 but it's there if folks want to keep it separate.

I think this is the issue that describes the original bug we need to solve in all core branches for profiles, so I don't think it makes sense to postpone it.

Clearer title; 8.x was just an example in the original title.

Near as I can tell, originally, this issue described a bug in how the extension handler checked the 'core' key for profiles, and it seems like due to this bug, they chose not to enable 'core_version_requirement' for profiles. Locally, and from reading the code, The only thing I see that prevents core_version_requirement from working for profiles is the fact that we explictly check for and throw an exception in InfoParserDynamic if we see that key in a profile, but I assume there must be some other problem that it will cause that is why we added that check and exception in the first place.

When I tested the possibility to install Drupal 9, I noticed that profiles do not work with core_version_requirement,
I thought 2 new issues were needed - 1 to make a decision if/how to get profiles to support it, and another follow up issue to implement that decision.
Yet somehow I missed this existing issue in the process...

Here's a patch to enable core_version_requirement for profiles. And it adds the VERSION support seen in #3072702: Core extensions should not need to specify the new core_version_requirement in *.info.yml files so that 9.0.x can be installed. Both patches are necessary to have an installable Drupal.

I guess we could only remove the profile restriction here and then set core_version_requirement everywhere in 9.0.x and 8.9.x in #3072702: Core extensions should not need to specify the new core_version_requirement in *.info.yml files so that 9.0.x can be installed. So this patch could be as attached.

1. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -34,11 +34,6 @@ public function parse($filename) {
   -      if ($parsed_info['type'] === 'profile' && isset($parsed_info['core_version_requirement'])) {
   -        // @todo Support the 'core_version_requirement' key in profiles in
   -        //   https://www.drupal.org/node/3070401.
   -        throw new InfoParserException("The 'core_version_requirement' key is not supported in profiles in $filename");
   -      }
   

   Yay for addressing the @todo in the intended issue 🥳

2. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -46,6 +41,9 @@ public function parse($filename) {
   +        if ($parsed_info['core_version_requirement'] === 'VERSION') {
   +          $parsed_info['core_version_requirement'] = \Drupal::VERSION;
   +        }
   
   +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
   @@ -439,12 +439,11 @@ public function providerCoreIncompatibility() {
   -core_version_requirement: ^8
   +core_version_requirement: VERSION
   

   I was going to ask why we even want this. Why not set core_version_requirement: ^8.8 in the 8.8.x branch, core_version_requirement: ^8.9 in the 8.9.x branch and core_version_requirement: ^9 in the 9.0.x branch?

   Therein lies the answer: we don't want to update this all the time. Specifying core_version_requirement: VERSION instead and making that magic work is a nicer solution.

   But … this is a special time. The only reason we need to commit three different values to three different branches is that we're on the verge of making the shift to Drupal 9.

   Once core_version_requirement: ^9 is committed to the 9.0.x branch, we won't need to make further changes. I think doing that would be preferable, because it doesn't introduce new magic. We're introducing this magic in an issue that is critical and trying to unbreak Drupal 9's automated testing. I think the core_version_requirement: VERSION feature deserves an issue with a dedicated discussion.

   Besides, we don't even need to commit core_version_requirement: ^8.9 to the 8.9.x branch — we can commit core_version_requirement: ^8.8 to both the 8.8.x and 8.9.x branches, and commit core_version_requirement: ^9 to the 9.0.x branch.

+++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
@@ -456,9 +455,8 @@ public function testInvalidProfile() {
         'invalid_profile.info.txt' => $profile,
...
+    $info = $this->infoParser->parse(vfsStream::url('profiles/fixtures/invalid_profile.info.txt'));

🤓 We still need to rename this from invalid_profile to just profile, but that's a nit.

We first need to agree about the approach here.

Once core_version_requirement: ^9 is committed to the 9.0.x branch, we won't need to make further changes. I think doing that would be preferable, because it doesn't introduce new magic. We're introducing this magic in an issue that is critical and trying to unbreak Drupal 9's automated testing. I think the core_version_requirement: VERSION feature deserves an issue with a dedicated discussion.

VERSION is not magic it just keeps us from doing a lot of unnecessary work. See #3072702: Core extensions should not need to specify the new core_version_requirement in *.info.yml files so that 9.0.x can be installed - also Drupal 10 with happen and we don't want to be always upping this thing.

I agree with you in the general case, but not for install profiles. The danger with allowing this VERSION magic constant is that all contrib/custom install profiles use that too, which allows them to claim (pretend, really) to be compatible with all Drupal majors ever. That's overshooting our goal here.

I do completely agree with you that for extensions in Drupal core, this VERSION magic constant is valuable. But for that I have a simpler counterproposal: #3072702-55: Core extensions should not need to specify the new core_version_requirement in *.info.yml files so that 9.0.x can be installed — which you already indicated you like :)

We'll see where this takes us!

This is being fixed as part of #3072702: Core extensions should not need to specify the new core_version_requirement in *.info.yml files so that 9.0.x can be installed.

So going back to how this issue was originally opened i.e. #2. We should consider either re-opening or filing a new issue.

That comment says:

1 changes just \Drupal\FunctionalTests\Installer\DistributionProfileTest to use a nonsense key for core. This should still pass
1 changes minimal.info.yml, testing.info.yml, and demo_umami.info.yml to use 7.x. I am not sure what will break here but I think \Drupal\FunctionalTests\Installer\InstallerTest::testInstaller will pass which it shouldn't.

So the tests should that the first part of that is right if you change core something meaningless like:

+++ b/core/tests/Drupal/FunctionalTests/Installer/DistributionProfileTest.php
@@ -22,7 +22,7 @@ protected function prepareEnvironment() {
-      'core' => \Drupal::CORE_COMPATIBILITY,
+      'core' => 'some-other-version-but-really-nonsense',

Drupal will continue to install. That's because we have no validation that on that value.

However the second part is wrong with the patch that did

+++ b/core/profiles/demo_umami/demo_umami.info.yml
@@ -2,7 +2,7 @@ name: 'Demo: Umami Food Magazine (Experimental)'
-core: 8.x
+core: 7.x

in install profiles you get fails like

1) Drupal\FunctionalTests\Installer\InstallerTest::testInstaller
Exception: Drupal\Core\Extension\MissingDependencyException: Unable to install modules: module 'testing' is incompatible with this version of Drupal core.
Drupal\Core\Extension\ModuleInstaller->install()() (Line: 91)

Which is exactly as it should be.