Problem/Motivation

\Drupal\Core\Extension\InfoParserDynamic::parse() calls \Composer\Semver\Semver::satisfies() using the core_version_requirement as input. This value will be entered by users whilst creating extension .info.yml files.

Proposed resolution

Catch the UnexpectedValueException as throw a more useful InfoParserException that points to the file where there is a problem.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/a

CommentFileSizeAuthor
#13 3088081-13.patch2.41 KBalexpott
#13 11-13-interdiff.txt988 bytesalexpott
#11 3088081_9_11.interdiff.txt1 KBdww
#11 3088081-11.patch2.61 KBdww
#9 3088081-9.patch2.63 KBalexpott
#9 2-9-interdiff.txt2.5 KBalexpott
#2 3088081-2.patch2.91 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Active » Needs review
FileSize
3088081-2.patch2.91 KB
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Assigned: Unassigned » tedbow

Since @tedbow was the lead on the core_version_requirement thing, I think it'd be great to get his thoughts on this.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

If we don't do this you get an UnexpectedValueException with the message Could not parse version constraint not: Invalid version string "not"

Which is not very helpful. InfoParserDynamic should catch all possible exceptions and re-throw them with helpful messages saying which key and value and which file.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Needs review » Reviewed & tested by the community

FWIW I think this improvement is solid, and more Gracie in tests is always a good thing. Tentative RTBC assuming tests pass, but I'll also ping Ted and make sure he doesn't have any concerns.

cilefen’s picture

cilefen CreditAttribution: cilefen as a volunteer commented
Title: Improve the error message if an nonsense constraint is used in core_version_requirement » Improve the error message if a nonsense constraint is used in core_version_requirement
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented

This looks good! Thanks!

Status: Reviewed & tested by the community » Needs work

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs work » Needs review
FileSize
2-9-interdiff.txt2.5 KB
3088081-9.patch2.63 KB

Ah there was a test already... let's update that one.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented

Super duper small nit:

+++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
@@ -46,7 +46,12 @@ public function parse($filename) {
+          throw new InfoParserException("The 'core_version_requirement' constraint ({$parsed_info['core_version_requirement']}) is not a valid value in " . $filename);

No need for . $filename since we're already using " and including the value of {$parsed_info['core_version_requirement']} directly in the string.

Would be slightly faster and take less space in the code to do this:

throw new InfoParserException("The 'core_version_requirement' constraint ({$parsed_info['core_version_requirement']}) is not a valid value in $filename");

Otherwise, huge +1 to making this more user-friendly. :)

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
FileSize
3088081-11.patch2.61 KB
3088081_9_11.interdiff.txt1 KB
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs review » Needs work 
+++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
@@ -483,9 +483,9 @@ public function testUnparsableCoreVersionRequirement() {
+    $this->infoParser->parse(vfsStream::url("modules/fixtures/nonsense_core_version_requirement.info.txt"));    $this->infoParser->parse(vfsStream::url('modules/fixtures/unparsable_core_version_requirement.info.txt'));

This line doesn't need the 2 calls to parse(). I think this left from when it was a its own test.

The file nonsense_core_version_requirement.info.txt is not being created. The only reason the test passes is because if the fail does not exist and empty array is return and no exception is thrown.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs work » Needs review
FileSize
11-13-interdiff.txt988 bytes
3088081-13.patch2.41 KB

Addressing #12

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Assigned: tedbow » Unassigned

🤦‍♂️I don't know how I saw #10 but completely overlooked #12. :(

:ashamed: ;)

Sorry,
-Derek

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs review » Reviewed & tested by the community

@dww it happens 😜

#10 was a good catch!

Looking good now!

  • catch committed 1b67651 on 9.0.x 
    Issue #3088081 by alexpott, dww, tedbow, xjm: Improve the error message...

  • catch committed ffa4c37 on 8.9.x 
    Issue #3088081 by alexpott, dww, tedbow, xjm: Improve the error message...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove 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 and 8.9.x, moving to 'to be ported' for when packaging is up and running again.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented

@catch thanks for committing, and thanks everyone for the work on this.
testing #13 on 8.8.x as it still applies

  • catch committed 5b4e237 on 8.8.x 
    Issue #3088081 by alexpott, dww, tedbow, xjm: Improve the error message...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Patch (to be ported) » Fixed

Cherry-picked to 8.8.x, thanks!

Status: Fixed » Closed (fixed)

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