Problem/Motivation

In order for a contrib project to be testable on Drupal 9 all the modules need to be marked as D9 compatible. This makes sense for the *real* modules provided by the project but it does not make sense for test modules.

Proposed resolution

Allow modules that are in the Testing package to default to the Drupal version under test.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/a - well we could do one but it doesn't feel that important.

CommentFileSizeAuthor
#4 3096609-4.patch2.67 KBalexpott
#4 2-4-interdiff.txt1.46 KBalexpott
#2 3096609-2.patch2.3 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 3096609-2.patch2.3 KB

Status: Needs review » Needs work

The last submitted patch, 2: 3096609-2.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 2-4-interdiff.txt1.46 KB
new 3096609-4.patch2.67 KB

Here's a fix. \Drupal\Tests\system\Functional\Form\ModulesListFormWebTest::testModulesListFormWithInvalidInfoFile() creates an invalid info .yml file without a package key. Nice because it's not mandatory.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
@@ -63,6 +63,11 @@ public function parse($filename) {
           $parsed_info['core_version_requirement'] = \Drupal::VERSION;
         }
+        elseif (isset($parsed_info['package']) && $parsed_info['package'] === 'Testing') {
+          // Modules in the testing package are exempt as well. This makes it
+          // easier for contrib to use test modules.
+          $parsed_info['core_version_requirement'] = \Drupal::VERSION;
+        }

Isn't this going to break on the lines below for modules that still do have a core: 8.x key, as would then result in getting an exception on incompatible core vs. core_version_requirement key?

What about only doing this if those testing modules do not yet have a core nor core_version_requirement key instead of forcing it? If we do force then we need to unset core too.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review

@Berdir context is everything :) this is inside another if...

      if (!isset($parsed_info['core']) && !isset($parsed_info['core_version_requirement'])) {
        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.
          $parsed_info['core_version_requirement'] = \Drupal::VERSION;
        }
        elseif (isset($parsed_info['package']) && $parsed_info['package'] === 'Testing') {
          // Modules in the testing package are exempt as well. This makes it
          // easier for contrib to use test modules.
          $parsed_info['core_version_requirement'] = \Drupal::VERSION;
        }
        else {
          // Non-core extensions must specify core compatibility.
          throw new InfoParserException("The 'core' or the 'core_version_requirement' key must be present in " . $filename);
        }
      }
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Oh, I did look at the whole function to make sure it is above the other check, but didn't see that it is inside this condition.

I think this is fine then, not sure if it needs a change record or documentation updates?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue tags: +Needs change record updates

I'll update https://www.drupal.org/node/3070687 once this has been committed.

  • catch committed 9c32d31 on 9.0.x 
    Issue #3096609 by alexpott, Berdir: Allow contrib test modules to not...

  • catch committed 8f6359b on 8.9.x 
    Issue #3096609 by alexpott, Berdir: Allow contrib test modules to not...
catch’s picture

catch
he/him
Primary language English
commented
Version: 9.0.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to 9.0.x/8.9.x, moving to 8.8.x for backport once the freeze is over. I'm in favour of getting this into 8.8 to make contrib porting easier.

  • catch committed c4b4c15 on 8.8.x 
    Issue #3096609 by alexpott, Berdir: Allow contrib test modules to not...
catch’s picture

catch
he/him
Primary language English
commented
Status: Patch (to be ported) » Fixed

Cherry-picked to 8.8.x, thanks! Also updated the change record and linked it to this issue.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Related issues: +#3101322: Error due to Drupal 9 support

I'm not so sure this actually fixes anything. See #3101322: Error due to Drupal 9 support.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@heddn this change is not part of 8.8.x yet. It will be released in 8.8.2

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Fixed » Needs work

CR needs update. It says this made it into 8.8.1.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Needs work » Needs review
Issue tags: -Needs change record updates

Can someone review my updates to the CR and mark this fixed if they agree with them?

heddn’s picture

heddn
Primary language English
Location Illinois
commented

It is confusing though, because this was committed 8 days ago and 8.8.1 was released only 2 days ago. Or at least I'm confused.

heddn’s picture

heddn
Primary language English
Location Illinois
commented

Berdir mentioned that 8.8.1 was a security only release, ergo it doesn't have anything but the security changes in it. That's the answer to #18.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Fixed

Updates to CR look good.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2313917: Core version key in module's .info.yml doesn't respect core semantic versioning

Woah, really good catch! 😊

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.