Problem/Motivation

Follow up to #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning

In that issue we caught exceptions thrown by \Composer\Semver\Semver::satisfies() and just return false.

This means any malformed version constraint in the new core_version_requirement key will just result in core_incompatible === FALSE in info.

The original reason I put this logic in was because we were actually using the existing core key and unknown values would not throw an exception but just make the module uninstallable. So if we changed this behavior and throw and exception this would mean any current site that had bad value in some info.yml fail some would suddenly not be able to use the modules form page.

When we switched from using the existing core to core_version_requirement we really need to catch this exception because the chance of existing site having the core_version_requirement key is very low.

Further it could cause a problem that developer might miss. For instance if you are updating your module that has some some modules but you currently don't have all the sub modules enabled you could accidentally change the value core_version_requirement in one of the sub-modules, say if you had the file open to just change the title or description but didn't have it enabled. Since there is no exception thrown you could easily commit that change.

Proposed resolution

Remove \Drupal\Core\Extension\InfoParserDynamic::satisfies() entirely and just call \Composer\Semver\Semver::satisfies() directly instead.

This would thrown an exception if there was a malformed value in core_version_requirement and the developer would be alerted to fix this.

Remaining tasks

Patch, review

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

None needed.

CommentFileSizeAuthor
#10 3078001-10.patch716 bytesGábor Hojtsy
#5 3078001-5.patch5.15 KBtedbow
#5 interdiff-5.txt1.46 KBtedbow
#5 3078001-5-8.7.patch5.08 KBtedbow
#2 3078001-2-8.7.patch3.92 KBtedbow
#2 3078001-2.patch3.99 KBtedbow
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Active » Needs review
FileSize
3078001-2.patch3.99 KB
3078001-2-8.7.patch3.92 KB

Patches

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented 
+++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
@@ -82,20 +59,15 @@ public function parse($filename) {
-      try {
-        $core_version_constraint = isset($parsed_info['core_version_requirement']) ? $parsed_info['core_version_requirement'] : $parsed_info['core'];
...
-      }
-      catch (\UnexpectedValueException $exception) {
-        $parsed_info['core_incompatible'] = TRUE;
-      }

We actually didn't need this try/catch because the static::satisfies() already catches the exception 🤷‍♂️

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Looks good, but this needs an explicit test that shows that invalid values do trigger a particular expected exception.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Priority: Normal » Major
Status: Needs work » Needs review
FileSize
3078001-5-8.7.patch5.08 KB
interdiff-5.txt1.46 KB
3078001-5.patch5.15 KB
  1. Ok here is the test.
  2. Changing this to Major because if doesn't get in before the 8.7.7 release this will change functionality
  3. Uploading 8.7.x patch also
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests 
+++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
@@ -465,4 +460,31 @@ public function testInvalidProfile() {
+   * Tests the exception for an unparsable 'core_version_requirement' value .

The space before the period can be fixed on commit.

  • catch committed 1f820f8 on 8.8.x 
    Issue #3078001 by tedbow, Wim Leers: Don't catch exception for invalid '...

  • catch committed 9a95bad on 8.7.x 
    Issue #3078001 by tedbow, Wim Leers: Don't catch exception for invalid '...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.8.x-dev » 8.7.x-dev
Status: Reviewed & tested by the community » Fixed

Fixed the space and committer/pushed to 8.7/8.8

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
FileSize
3078001-10.patch716 bytes

I was about to fix this on commit, so doing a followup commit :)

  • Gábor Hojtsy committed 190f027 on 8.8.x 
    Issue #3078001 by tedbow, Wim Leers: Followup minor test fix to not...

  • Gábor Hojtsy committed 680c365 on 8.7.x 
    Issue #3078001 by tedbow, Wim Leers: Followup minor test fix to not...

Status: Fixed » Closed (fixed)

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