Core extensions should not need to specify the new core_version_requirement in *.info.yml files so that 9.0.x can be installed

tedbow created an issue. See original summary.

Postponing until #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning is fixed

This issue may need to be rescoped/retitled since the currently proposed resolution on #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning is to provide a new key instead, but we'll have the same end goal of making core modules follow the D9-ready best practice.

#3++

#2313917: Core version key in module's .info.yml doesn't respect core semantic versioning was committed
Update summary and title match the solution there.

Changed proposed solution. I am not actually sure we should make a change.

I think the time to leverage the new feature in existing core is when we have removed all known deprecations from a module and are confident that it will work on 9, then we can change it to ^8 || ^9 for the 8.9 cycle, perhaps. Not sure it really does anything beyond act as an example for people to follow, because core modules will always work on the version of core they ship with, nonwithstanding undetected bugs.

Yeah, I don't think that makes much sense?

Core modules will always be compatible with whatever state the 9.x branch happens to be in because we always test core as a whole, but they will also quickly (at least with 9.1+) become 9.x only.

IMHO the only reason to use ^8 || ^9 would be as a transition during 8.9/9.0 development, so make it easier to port changes, but once/before 9.0 is released, I'd update that to ^9 only, not sure if updating on minor releases (^9.1, ^9.2, ..) is something we want to do. It would match the reality, because core code is always immediately updated to use new API's and you can never expect to use any core module in an older core version. But it's also a lot of overhead and would result in backport conflicts?

So considering that, maybe it should actually be core_version_requirement: VERSION, matching "version: VERSION", that we'd replace to indicate that a core module is always only compatible with the exact version it is shipped as?

We then also wouldn't have to keep it up to date manually, and there wouldn't be any actual change in info.yml files between minor and even major versions, making it easier to keep patches relevant and backportable (e.g. new test modules)

Great and super interesting observations, @Berdir!

I like #9 a lot.

About the problem that contrib will copy this, which currently happens a lot in library definitions maybe we could enforce that only core modules are allowed to use this definition and throw an error if we find anything else using it? Based on the extension being in /core or not or a nicer similar check.

I like #12 a lot.

Can't wait to see what @Berdir says next :D

Just want to relate this to #3005229: Provide optional support for using composer.json for dependency metadata

Not sure when that issue will get in. If it does get in then by 10.0.0 would won't have core_version_requirment in info.yml files.

If we expect that issue to get by 8.8.0 we could just switch to using composer.json for core and module dependencies. Of course we couldn't use VERSION for drupal/core in the composer.json because this would not work with composer outside of core.

So, this issue is actually the very critical-est of critical issues as 9.0.x is uninstallable until we have a solution for this. See #3087408: Update D9 core's *.info.yml files. I've closed that as a duplicate and am bumping this one to critical in its place. Also adding issue credits from the other issue.

Listed the possible solutions so far in the IS.

VERSION is parity with what we already have, and negates the requirement for us to ever have to revisit this problem again, so +1 here.

Putting anything with "8" in it anywhere in the Drupal *9* branch IMO risks end users who less clued into all the inner workings here filing bogus bug reports. And creates more things to clean up later, when 8 and 9 start to diverge.

Can't wait to see what @Berdir says next :D

I say this ALL THE TIME! :D

Forgot to mention in my IS update why it's critical. :P

> Can't wait to see what @Berdir says next :D

What was mentioned in the slack thread today is that VERSION doesn't actually prevent anyone from copying a module of any version and overwriting it and it would be "compatible". But the same is true with "version: VERSION" that we have as well in the .info.yml files. They magically become whatever version you put them in.

And for releases, we could possibly even overwrite it with the actual version during the package process?

VERSION might make sense for right now, just to get the 9.0.x branch running.

I guess we shouldn't replace core: 8.x in views yml files.........

+++ b/core/tests/Drupal/Tests/Core/Test/TestDiscoveryTest.php
@@ -314,13 +314,13 @@ class FunctionalExampleTest {}
-core: 8.x
+core_version_requirement: ^9
...
-core: 8.x
+core_version_requirement: ^9

+++ b/core/tests/Drupal/Tests/Core/Update/UpdateRegistryTest.php
@@ -37,13 +37,13 @@ protected function setupBasicModules() {
-core: 8.x
+core_version_requirement: ^9
...
-core: 8.x
+core_version_requirement: ^9

Are these intended?

@plach, I'm honestly not sure yet, I started from the patch in #3087408: Update D9 core's *.info.yml files but I'm still failing Functional Tests. Once I get things green here, we'll need to manage those tests, and probably have a test for VERSION etc.

Okay, so the functional tests are failing because the profiles are set to core:9.x but I'm running tests on 8.9.x

Okay, lets try this against 8.9 for now, I just want to get the 'VERSION' functionality working and green. There will have to be a few adjustments for 9.0.x though. We will need to change the profile core keys, there's at least one test in InfoParserUnitTest that will fail on 9.0.x (testing that core: 8.x is a valid module)

Alright, so we just need an explicit test of the VERSION keyword, update to that one test in InfoParser, and then figuring out what we are doing with profiles.

+1 for this approach.

Can we think of any potentially disruptive impacts from core modules no longer implementing the core key? Like does it change metadata in those arrays of extension information that get passed by reference everywhere (such that contrib or custom code might inspect them directly)?

I think we may have added logic around how to resolve a conflict between core and core_version_requirement if they did not match, but I'm unsure whether that logic involved favoring one over the other, or whether it involved just forbidding them both from being added in the same module. Edit: the change record for the original issue explains why it has to be possible to have both at the same time.

To clarify #32, we would presumably drop the core key in 9.0.x, but not sure if we should in 8.9.x or if that has the potential to break something. Edit: And for now we should make the patch that we would backport to 8.8.x.

I'm not sure we need to commit any of this to 8.9, particularly if you are going to diverge the info files anyway by keeping the core key. I suppose as it sits support for 'VERSION' is an API addition that should be in 8.9 if it's in 9.0, but we can't use VERSION and keep the core key anyway, because VERSION translates to 8.9-dev, and you can't have the core key if your core_version_requirement key doesn't include at least one version prior to 8.7.7 (it's buried in the cr)

you can't have the core key if your core_version_requirement key doesn't include at least one version prior to 8.7.7 (it's buried in the cr)

Ahhh that's the code path I was thinking about. So yeah, maybe this is 9.0.x-only then. Moving accordingly. (We can still add the tests and queue the patch against 8.9.x-dev instead of 9.0.x-dev on upload to validate that it would be a viable solution in 8.9.x and therefore presumably won't further break 9.0.x.)

I'm wondering if we shouldn't split out the functionality to allow the VERSION keyword into a seperate issue and commit it to 8.9 and 9.0, as well as resolve the profile situation in 8.9 and 9.0, and then commit the info file changes only to 9.0.

Searching for the error message leads to #3087408: Update D9 core's *.info.yml files instead of here. Google, please index this:

Drupal\Core\Extension\MissingDependencyException: Unable to install modules: module 'system' is incompatible with this version of Drupal core. in Drupal\Core\Extension\ModuleInstaller->install() (line 91 of /app/core/lib/Drupal/Core/Extension/ModuleInstaller.php).

I tried installing Umami with Drupal 9.0.x, and got this error message: (removed and added to the child issue)

Should that be fixed as part of this issue?

Update: same problem with Minimal and Standard profiles. Replacing core: 8.x with

core_version_requirement: VERSION

did not work. (Sorry, I did not save the error message.) Instead, I change it to core: 9.x.

Update: Oh, I see this is already reported in #3087416: Core version key in profile's .info.yml doesn't respect core semantic versioning.

It would be great to have profiles support core_version_requirement like modules and themes do.
I didn't know how big of a change that is going to be so I opened a separate issue #3087416: Core version key in profile's .info.yml doesn't respect core semantic versioning for that.

Since #36 doesn't update the profile core versions, this is needs work, right? With #36 installed, an install is still impossible

In ModuleInstaller.php line 91:

  Unable to install modules: module 'standard' is incompatible with this version of Drupal core.

We won't talk about the fact that it says "module" for a profile, but it seems to me that the ability to do an install should be a basic goal of this issue.

Maybe that's the same as https://www.drupal.org/project/drupal/issues/3087416 ? But if so, it seems it needs to be brought in here IMO.

Yes, but that can't happen until we are able to run the testbot on the 9.0.x branch. We are simply prepping as far as we can to get ready. 9.0.x is not supposed to be installable yet, and won't be for a few days at least.

It sounds like #3070401: Install profiles do not support multiple core branch compatibility actually needs to be critical as well. I've promoted it.

We can possibly hack it for this issue to get a green test run (once CI is working for 9.0.x) by using 9.x temporarily until we support profiles properly.

#37 is also actually something we should do I think so that 8.9.x has the same API. But I think we can wait to split out a separate issue until CI is working and we can verify whether or not this and #3070401: Install profiles do not support multiple core branch compatibility will be enough to fix HEAD.

Devil's advocate: why even again use the pattern core_version_requirement: VERSION like we did for asset library definitions if we can easily do something simpler here: for any extension that lives in core/… we exempt that from having to specify that key?

It's confusing, arguably even useless and nonsensical information in all of core's *.info.yml files anyway. All of the extensions in core are by definition compatible with this version of core!

@alexpott told me he thinks #55 is a good point and likes where it's going. So I'll turn #55 into a proper patch. Stay tuned.

I truly like #55, but the devil's devil's advocate in me says:

People look at core for direction on how to do things in custom and contrib. So if there's nothing specified, people will be confused.

But, based on the first sentence above, lots of folks do this in custom and contrib:

version: VERSION

The only other reason I can see to keep explicitly defining this is for if we ever try to split up core into smaller chunks so you can get different core modules separately from "core-core" (whatever that means). I know we're trying to do this for components. Maybe there's an issue somewhere about doing the same so you can explicitly composer require drupal/book ^11 or whatever. Not sure why we'd ever want that, but I'm sure someone from the composer-fixes-everything crew thinks that'd be Slick(tm). ;) If/when we ever support something like that, we can revisit if/how this would need to be handled.

So, for now, +1 to #55. That seems way better (simpler, easier to maintain, less magic) than the alternatives.

Cheers,
-Derek

p.s. And while I was writing the comment, #56 and #57 happened, so yay. ;)

+++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
@@ -39,8 +39,18 @@ public function parse($filename) {
+      if (strpos($filename, 'core/') === 0) {

One issue with this - do we not have incompatible modules in core/tests? Easy fix if we do...

Otherwise +1 from me too.

#59: yep :) And #61 looks good to me. I incorporated it verbatim. \Drupal\Tests\system\Functional\Module\DependencyTest::testCoreCompatibility() uses the system_incompatible_core_version_test module in core which is an example of this.

The interdiff compares #53 to the 9.0.x patch. The interdiff-d9-d8.txt interdiff shows what we need to change for that to be committable to 8.8.x and 8.9.x.

For BC reasons, I don't think we should remove the core key from the .info files in 8.8 or 8.9 (see #32)

I think we should have one patch with all the needed functionality that we commit to 8.9 and 9.0 based on passing 8.9 tests, and then one patch that goes into 9.0 only on top of it that makes 9.0 pass tests. (just my opinion though).

Then we can get to work :-)

+++ b/core/tests/Drupal/FunctionalTests/Installer/DistributionProfileExistingSettingsTest.php
@@ -29,7 +29,7 @@ protected function prepareEnvironment() {
-      'core' => \Drupal::CORE_COMPATIBILITY,
+      'core_version_requirement' => 'VERSION',

I missed these 😭

For BC reasons, I don't think we should remove the core key from the .info files in 8.8 or 8.9 (see #32)

What's the BC break here? #62 and #64 are not disallowing the core key. They're just dropping it for extensions that ship with Drupal core. https://www.drupal.org/node/3070687 indeed explains why we want both to be allowed/supported simultaneously, but that is specifically for contrib and custom modules, because they need to be able to declare compatibility with Drupal 8.7 and 8.8 simultaneously (and 8.9, and 9.0).

#32 was only asking if we can do this, it was not saying we cannot do this.

I think we should have one patch with all the needed functionality that we commit to 8.9 and 9.0 based on passing 8.9 tests, and then one patch that goes into 9.0 only on top of it that makes 9.0 pass tests. (just my opinion though).

This I do agree with: I'm not sure it's worth committing this to Drupal 8.8, I think committing only to 8.9 + 9.0 is preferable. Simply because it is unnecessary for 8.8. It's also unnecessary for 8.9, but the goals for 8.9 and 9.0 is to be identical except for dropping BC layers. Therefore this should be committed to both 8.9 and 9.0.

The 'BC break' is minor, only that
\Drupal::service('info_parser')->parse('core/modules/action/action.info.yml')['core'] has always returned '8.x' and is now unset and NULL. If we gained anything from making the change, I wouldn't care. I just don't think we gain anything in the 8.9 LTS from changing the info file arrays that justifies the change.

Happy to be overruled by the RMs or anybody really, just my 1/50th of a currency unit.

1. Any test using Updater::getUpdaterFromDirectory() results in a call to \Drupal\Core\Extension\InfoParserDynamic::parse() with an absolute file path instead of a Drupal root-relative file path. That is an inconsistency that should be fixed IMHO, but that's certainly out of scope here. Made the "is core extension" detection also work for absolute paths. 👍
2. StableBaseThemeUpdateTest + BaseThemeDefaultDeprecationTest + ConfigInstallProfileUnmetDependenciesTest interestingly copy a core/… extension into a test-specific sites/… directory. Solution: inject the core_version_requirement key.

Note that on point 1: I am using DRUPAL_ROOT here instead of $container->getParameter('app_root') because InfoParserDynamic is not a service. I think the proper solution is to add a constructor with an optional $app_root argument. Otherwise unit tests such as Drupal\Tests\Core\Extension\ThemeExtensionListTest::testRebuildThemeDataWithThemeParents() will trigger Use of undefined constant DRUPAL_ROOT - assumed 'DRUPAL_ROOT' (this will throw an Error in a future version of PHP). I'll leave that for somebody else to fix while I sleep.

So the requirements for commit here are:

1. Patch surfaces real fails for 9.0.x (rather than "build successful") and ideally the tarball can be installed manually
2. Patch passes on 8.9.x
3. If the patch requires any deprecations (maybe related to the behavior mentioned in #66? TBD) then those deprecations should also be backportable to 8.8.x, I think?

Ugh, #73's D8 patch was wrong 😭 The interdiff was correct, but it was applied relative to #62, not #63.

_{I don't know who queued a 9.0 test for the 8.8+8.9 patch in #73, but that's definitely going to fail.}

Updating the issue summary per #57 & #59.

Re #77 if you click on the test it tells you who queued it.

I queued it because #74 asks for a patch that passes on 8.9 and surfaces real failures on 9.0, so somehow this needs to be distilled into one commit that goes on both, and then additional commits that go to 9.0 to bring all the tests green. I'm just trying to figure out what those patches look like.

#82.2: Oh! Yes! That will allow you to converge on a single patch instead of the 9.0.x vs 8.8.x_and_8.9.x patches I've been rolling. Much better 👏

#82.1 + #82 + #85 look great! #85 shows why I said we couldn't "just inject the app root": there are a few cases where the info parser is instantiated as a non-service.

$app_root = \Drupal::hasService('app.root') ? (string) \Drupal::service('app.root') : DRUPAL_ROOT;

for maximum BC - just in case the container's not around.

I don't see the harm in that. Aiming for maximum BC is warranted in cases like this I think.

+++ b/core/modules/system/tests/src/Functional/Update/StableBaseThemeUpdateTest.php
@@ -73,17 +74,32 @@ protected function setUp() {
+  /**
+   * Adds a core_version_requirement to an info.yml file contents.
+   *
+   * @param string $info_yml_file_contents
...
+   *
+   * @return string
+   *   The updated .info.yml file contents.
+   */
+  private static function addCoreVersionRequirement(string $info_yml_file_contents) {
+    $info = Yaml::decode($info_yml_file_contents);
+    $info['core_version_requirement'] = '*';
+    return Yaml::encode($info);
+  }

Can we document what the method does a little more clearly? I.e., it adds a requirement of * which means "any version" (right?). At least in an inline comment that justifies why this is the right choice for the test.

Also, what is the value of marking the method private instead of protected when it's called from a protected method? All private does in this case (vs. protected) is make it so that if someone subclasses the thing, they'll get a fatal error until they implement their own version of this method or override all the methods that called it to not call it anymore.

We should actually adopt a policy about the use of private in core before adding it here and there in some issues to some but not all APIs of a certain type. Maybe there's a case for all non-base-class tests should be final or something, to help indicate that the tests themselves are internal and don't support subclassing, but if so, we should make that change consistently across the test suite in a scheduled coding standards change patch following a core policy change, not randomly here and there. (Policy issue: #3019332: Use final to define classes that are NOT extension points. Edit: Removed out-of-scope rambling.)

+++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
@@ -34,13 +55,19 @@ 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");
+      if (strpos($filename, 'core/') === 0 || strpos($filename, $this->root . '/core/') === 0) {
+        // Core extensions do not need to specify core compatibility: they are
+        // by definition compatible so a sensible default is used. Core modules
+        // are allowed to provide these for testing purposes.
+        if (!isset($parsed_info['core']) && !isset($parsed_info['core_version_requirement'])) {
+          $parsed_info['core_version_requirement'] = \Drupal::VERSION;
+        }
       }
-      if (!isset($parsed_info['core']) && !isset($parsed_info['core_version_requirement'])) {
-        throw new InfoParserException("The 'core' or the 'core_version_requirement' key must be present in " . $filename);
+      else {
+        // Non-core extensions must specify core compatibility.
+        if (!isset($parsed_info['core']) && !isset($parsed_info['core_version_requirement'])) {
+          throw new InfoParserException("The 'core' or the 'core_version_requirement' key must be present in " . $filename);
+        }

Is this also fixing the lack of support for the key from profiles? (The @todo is being removed.) If so we'd need added tests for that which I don't see in the patch. (It's possible I missed them scanning through the patch as I've not done a close review. Maybe we should add a -do-not-test.patch of the bits that aren't just removing the core key?) If not we'd want to leave in the @todo.

1. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -11,11 +11,32 @@
   +  public function __construct(string $app_root = NULL) {
   +    if ($app_root === NULL) {
   +      // @todo Require $app_root argument.
   +      $app_root = (string) \Drupal::service('app.root');
   +    }
   +    $this->root = $app_root;
   

   Is the plan to add that to the patch, or should we do a @trigger_eror() in the code path and require the service in the next major? (And if so, would be good to have that backported to 8.8 before beta; if it's too disruptive or doesn't get backported in time, we'll need to handle this however we decide to do 10.0.x deprecations instead.)

2. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -34,13 +55,19 @@ public function parse($filename) {
   +      if (strpos($filename, 'core/') === 0 || strpos($filename, $this->root . '/core/') === 0) {
   

   (Sorry for the piecemeal review comments; still going over what's in the more recent patches.)

   Do we do a check like this elsewhere to determine if something's a core module? The info parser itself doesn't seem to yet before this patch. It seems like "is a core module" is something that should have its own API extracted.

   Weird potential edgecase time: what if someone's site is installed in a subdirectory called core/ or a contrib module has a core/ directory within its namespace? (Asking because InfoParserFilename::parse() says the filename can be use either relative or absolute path, without defining in so many words how many varieties of "relative" work.)

   I don't actually know that Drupal doesn't already blow up if a module or site uses its own core directory, but if Drupal works under those scenarios, then we could add test coverage proving that the info parser still works for those scenarios after this change. Edit: or rather, that it doesn't identify those things as core extensions.

Down to eight 9.0.x fails with a green 8.9.x patch, not bad. https://dispatcher.drupalci.org/job/drupal_patches/13442/ A couple look related to the l.d.o issue but not sure about the several others related to the update path.

1) Drupal\Tests\system\Functional\Update\UpdatePathTestBaseFilledTest::testUpdateHookN
Failed asserting that 8000 matches expected 8001.

🤔Looks like a hook_update_last_removed() expectation inconsistency or something? Could be related to the above discussion of the updater, and if so it'd probably still be in scope for this issue. (OTOH if it's a separate bug for something elsewhere let's split an issue out.)

1. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -11,11 +11,32 @@
     */
   
   +++ b/core/modules/config/tests/src/Functional/ConfigInstallProfileUnmetDependenciesTest.php
   @@ -68,6 +68,10 @@ protected function copyTestingOverrides() {
   +    $info_yml_path = $dest . DIRECTORY_SEPARATOR . 'testing_config_overrides.info.yml';
   +    $info = Yaml::decode(file_get_contents($info_yml_path));
   +    $info['core_version_requirement'] = '*';
   +    file_put_contents($info_yml_path, Yaml::encode($info));
   

   Oh, same comment about a comment about the * version here. I wonder if we should put it in a trait or something? VersionAgnosticTestInfoFileTrait or something.

2. +++ b/core/tests/Drupal/FunctionalTests/Installer/DistributionProfileTranslationTest.php
   @@ -32,7 +32,7 @@ protected function prepareEnvironment() {
   -      'core' => \Drupal::CORE_COMPATIBILITY,
   +      'core_version_requirement' => '*',
   
   +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerExistingConfigTestBase.php
   @@ -36,7 +36,7 @@ protected function prepareEnvironment() {
   -      'core' => \Drupal::CORE_COMPATIBILITY,
   +      'core_version_requirement' => '*',
   
   +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerNonEnglishProfileWithoutLocaleModuleTest.php
   @@ -33,7 +33,7 @@ protected function prepareEnvironment() {
   -      'core' => \Drupal::CORE_COMPATIBILITY,
   +      'core_version_requirement' => '*',
   

   Ditto.

89 - addressed with new trait
90 - Yes, this allows the new key for profiles.

+++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
@@ -439,12 +464,11 @@ public function providerCoreIncompatibility() {
-   * Test a profile info file with the 'core_version_requirement' key.
+   * Test a profile info file.
    */
-  public function testInvalidProfile() {
+  public function testProfile() {
     $profile = <<<PROFILE_TEST
-core: 8.x
-core_version_requirement: ^8
+core_version_requirement: '*'
 name: The Perfect Profile
 type: profile
 description: 'This profile makes Drupal perfect. You should have no complaints.'
@@ -456,9 +480,8 @@ public function testInvalidProfile() {

@@ -456,9 +480,8 @@ public function testInvalidProfile() {
         'invalid_profile.info.txt' => $profile,
       ],
     ]);
-    $this->expectException('\Drupal\Core\Extension\InfoParserException');
-    $this->expectExceptionMessage("The 'core_version_requirement' key is not supported in profiles in vfs://profiles/fixtures/invalid_profile.info.txt");
-    $this->infoParser->parse(vfsStream::url('profiles/fixtures/invalid_profile.info.txt'));
+    $info = $this->infoParser->parse(vfsStream::url('profiles/fixtures/invalid_profile.info.txt'));
+    $this->assertFalse($info['core_incompatible']);

Test is here

92.1 - Would like to kick an actual deprecation and @trigger_error to a follow up. The app.root service itself is likely to be deprecated early in Drupal 9 in #3074585: [Symfony 5] Replace app.root and site.path string services with container parameters, and I'd like to get that one settled before we worry about trying to set up a deprecation around it.

92.2 - I think this is okay. relative vs absolute is a matter of file_get_contents(), so it should be either absolute or relative from the Drupal root, which precludes either scenario you described from triggering this, I believe.

93 - I believe this is a deeper problem that should be kicked into it's own issue.

94.1 - I made a trait with better documentation.

94.2 - I documented why we are using '*' in the tests in the places you mentioned.

+++ b/core/modules/config/tests/src/Functional/ConfigInstallProfileUnmetDependenciesTest.php
@@ -69,9 +70,11 @@ protected function copyTestingOverrides() {
+    // The core profile does not require a core_version_requirement key, but
+    // this copy will require it.
+    $info = file_get_contents($info_yml_path);
+    file_put_contents($info_yml_path, $this->addCoreVersionRequirement($info));

I think we can omit the inline comment here now since we're using the trait method, which is self-documenting.

Edit: Ignore me; I should actually read the code snippets I'm commenting on. :P

Here's an untested attempt to bulk pull out the info file changes. I'm not sure what this will do, but it might pass on 8.9 :-)

interdiff included, though I don't know how useful it is.

1. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -11,11 +11,32 @@
   +      \Drupal::hasService('app.root') ? (string) \Drupal::service('app.root') : DRUPAL_ROOT;
   

   ick, but yeah understand

2. +++ b/core/tests/Drupal/KernelTests/Core/Theme/BaseThemeDefaultDeprecationTest.php
   @@ -91,6 +92,21 @@ protected function setUpFilesystem() {
   +  private static function addCoreVersionRequirement(string $info_yml_file_contents) {
   

   any reason this doesn't use the trait?

Assuming #98 passes on 8.9, I think it (along with any cleanup anybody wants to do) should be committed to 8.9 and #96 should be committed to 9.0.x, or we could commit #98 to 9.0 and 8.9 and then commit a diff of #96 and #98 to 9.0. Then we should be able to spin off a couple issues to fix the remaining test failures.

#102 looks good to avoid the trait. If this comes back green I think we should commit (to 9.0.x, then leave this open to discuss the 8.9.x backport a bit more) and open issues for the remaining test failures.

Opened #3087991: Drupal\Tests\system\Functional\Update\UpdatePathTestBaseFilledTest::testUpdateHookN Failed asserting that 8000 matches expected 8001..

• #89: sure, protected works too.
• #90: yes. This incorporates #3070401: Install profiles do not support multiple core branch compatibility in full, including its test coverage (see \Drupal\Tests\Core\Extension\InfoParserUnitTest::testProfile).
• #92.1: @alexpott added a follow-up for this in #102.
• #92.2: the received path is guaranteed to be either Drupal root-relative or an absolute path. Otherwise it simply won't work. (And yes, serving Drupal 8 from http://example.com/core does not work. It has not worked since before Drupal 8 shipped. I reported that in 2015, perhaps even 2014 or 2013. It was never fixed because … nobody does that, so it's not prioritized.) EDIT: I see @mikelutz made the same argument in #96 🙂
• #93: agreed with @mikelutz that this is out of scope here: this issue is not about making the 9.0.x branch get to 0 fails. It's about getting it to run at all. Like @alexpott wrote in #53: We still need another issue to actually fix the outstanding 9.0.x fails but I think they will require 9.0.x only fixes.
• #94.1 + #94.2 + #96: it feels wrong to add a trait for this. This is a super edge-casey need. To add more API surface for this just for the sake of DRY … I have my doubts about that. 🤔
• #96 RE #92.1: but #3074585: [Symfony 5] Replace app.root and site.path string services with container parameters is about container parameters, @xjm's question in #92.1 is about this new constructor argument. #3074585 is not even touching InfoParser(Dynamic). I don't think it makes sense to defer to that issue.
• #102: Clever, the use of vfs://core/… 🤓And matches the way the virtual file system is intended to be used! 👏 I'm less of a fan of removing ConfigInstallProfileUnmetDependenciesTest's logic in favor of hardcoding core_version_requirement: '*' in a *.info.yml file but … the harm is minimal and the simplicity is undeniable. So: 👍

Patch review

1. +++ b/core/modules/system/tests/src/Functional/Update/StableBaseThemeUpdateTest.php
   @@ -69,17 +67,17 @@
   -    $vfs_root = vfsStream::setup('root');
   +    $vfs_root = vfsStream::setup('core');
        $vfs_root->addChild(vfsStream::newDirectory($this->siteDirectory));
        $site_dir = $vfs_root->getChild($this->siteDirectory);
   ...
          'themes' => [
            'test_stable' => [
   -          'test_stable.info.yml' => $this->addCoreVersionRequirement(file_get_contents(DRUPAL_ROOT . '/core/tests/fixtures/test_stable/test_stable.info.yml')),
   

   🤓 A consequence is that this will result in vfs://core/sites/simpletest/LOCK_ID/themes/test_stable/test_stable.info.yml. So: a sites directory in a core directory. This A) is weird, B) does not match the reality of a non-VFS Drupal core. We should ensure that the virtual file system matches the actual Drupal core file layout.

   Easily remedied though!

2. +++ b/core/modules/system/tests/src/Functional/Update/StableBaseThemeUpdateTest.php
   @@ -67,7 +67,7 @@ protected function setUp() {
   -    $vfs_root = vfsStream::setup('root');
   +    $vfs_root = vfsStream::setup('core');
   
   @@ -135,7 +135,7 @@ class VfsInfoParser extends InfoParser {
   -    return parent::parse("vfs://root/$filename");
   +    return parent::parse("vfs://core/$filename");
   
   +++ b/core/tests/Drupal/KernelTests/Core/Theme/BaseThemeDefaultDeprecationTest.php
   @@ -91,6 +92,21 @@ protected function setUpFilesystem() {
   +  private static function addCoreVersionRequirement(string $info_yml_file_contents) {
   

   Ideally these would use a similar approach: ideally both would use vfs://core. And … we can! This means the last private static function addCoreVersionRequirement also disappears 🥳

3. +++ b/core/modules/system/tests/src/Functional/Update/StableBaseThemeUpdateTest.php
   @@ -118,8 +116,8 @@
      public function __construct(string $root, string $type, CacheBackendInterface $cache, InfoParserInterface $info_parser, ModuleHandlerInterface $module_handler, StateInterface $state, ConfigFactoryInterface $config_factory, ThemeEngineExtensionList $engine_list, $install_profile) {
   
   @@ -137,7 +135,7 @@
      public function parse($filename) {
   

   🤓 Note that in principle we don't need this VfsThemeExtensionList test class anymore nor VfsInfoParser . Because the InfoParser now finally accepts an app root, so there is no more need to override either to force a different app root!

   We had no choice but to add these in #3065545: Deprecate base theme fallback to Stable, in principle we should now be able to remove them. Although that would sort of be out of scope.

   Except we can't, because we're dealing with the Drupal test runner kernel's container and the "normal" container, and if we're not careful vfs won't exist yet (which would trigger file_exists(): Unable to find the wrapper "vfs" - did you forget to enable it when you configured PHP?).

   So … long story short, while I think this paves the path for further clean-up and simplification, I think that's out of scope here. But we definitely should take another hard look at extension discovery, extension info file parsing, and extension lists. (If you want even more detail, see #3065545-26: Deprecate base theme fallback to Stable.)

Remaining failures → other issues

The remaining failures in #105 are out-of-scope for this issue. Most of the failures are the responsibility of #3016471: Make localization file download major version agnostic. For the only other failure, @catch already opened #3087991: Drupal\Tests\system\Functional\Update\UpdatePathTestBaseFilledTest::testUpdateHookN Failed asserting that 8000 matches expected 8001..

The goal of this issue is to get Drupal 9.0.x test runs running again. We've reached that point. We've reviewed approaches in detail. So … let's ship this! 🚢

+1 on committing this to 9.0.x and moving on to the other test failures.

Yes I think we should avoid a change record here since it's likely to be more confusing than not, so untagging for that.

This is in good shape to unblock the other issues.

Committed e37031e and pushed to 9.0.x. Thanks!

Moving to 8.9.x for the backport discussion - my view is that we should backport the changes to minimise the likelihood of patch conflicts even though we don't need to for any other reason. No-one should be trying to parse out version compatibility from the .info.yml files in contrib/custom code and that's the only change that anyone will notice.

+++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
@@ -11,11 +11,33 @@
+      \Drupal::hasService('app.root') ? (string) \Drupal::service('app.root') : DRUPAL_ROOT;

What is the point of this line without any assignment?

ugh, #118 that may have been a bad copy paste on my part. That definitely should be an assignment.

I still think we need a CR for 'Profiles now support the core_version_requirement key'?

I've updated the existing change record at https://www.drupal.org/node/3070687

The inline comments got lost for a few of the places in the patch, but 'tis a trivial docs followup.

Are the composer.lock changes intentional?

What's left here?

As I sit here, writing a bugfix test that requires a new test module, I realize that yes, we should backport this, and yes, we should backport it to 8.8, because I can't currently create a new test module that is compatible with 8.8 and 9.0, and I see that happening more than once with bug fixes on 8.8 over the next 6 months.

Symbolically RTBCing, since it comes down to a RM decision anyway.

Edit: actually, I might be able to use core_version_requirement: * to get one patch to work with both, but it would still warrant a 9.0.x follow-up to remove it, and I'd rather avoid the follow-up or the unnecessary key being there forever.

I'm in favour of backporting this back to 8.8 given the test module issue, that's a good reason to do so.

Committed/pushed to 8.9.x and cherry-picked back to 8.8.x, thanks!