Problem/Motivation

Follow-up to #3134417-78: Add \Drupal::getMajorVersion()

Running core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php on PHP 8

PHP Warning:  A non-numeric value encountered in /var/www/html/web/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php on line 301

Warning: A non-numeric value encountered in /var/www/html/web/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php on line 301
PHPUnit 9.6.8 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Core\Extension\InfoParserUnitTest

Proposed resolution

Explode string on 2 parts and cast leftover part to int to remove stability suffix

Remaining tasks

review/commit

User interface changes

None.

CommentFileSizeAuthor
#13 3365970-13.patch821 bytesandypost
#2 3365970-2.patch789 bytesandypost

Comments

andypost created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs review
StatusFileSize
new 3365970-2.patch789 bytes
spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs review » Reviewed & tested by the community
Related issues: +#3365880: TestBot throws uncaught PHP Warning on 11.x-dev only

Works for me.

dww’s picture

dww
we/he/they
commented
Status: Reviewed & tested by the community » Needs review

IMHO, this is masking a bug. Core’s version string, even in main branch when it exists (or 11.x) for now, should never be a 2-digit version. See the branch alias issue. I’d call this works as designed.

spokje’s picture

spokje
Primary language Dutch
commented

Seeing that it causes an (uncaught) PHP warning, that (AFAICT) we expect to cause tests to fail (#3365880: TestBot throws uncaught PHP Warning on 11.x-dev only), my humple opinion is that it's broken, which can't be works as designed.

But happy to let other, bigger brains decide on this :)

dww’s picture

dww
we/he/they
commented

I agree there’s a problem. My view is the problem comes from using “11.0-dev” as the VERSION string in the branch, not what this test is doing.

spokje’s picture

spokje
Primary language Dutch
commented

Absolutely a fair point.

We both agree there's a problem, we just don't agree on the solution.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Lets get this in the committers eyes.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Probably it could use better fix but I have no idea how to improve it

catch’s picture

catch
he/him
Primary language English
commented

#3364646: Add a branch alias for 11.x would be the one - if we can resolve that, we might not need the workaround here.

andypost’s picture

andypost
he/him
Primary language Russian
commented

The -dev suffix will annoy anyway

dww’s picture

dww
we/he/they
commented

But that’s the point, if VERSION was already 10.2.0-dev in the “main” branch, this test wouldn’t have any trouble. 2 is already an int.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 3365970-13.patch821 bytes

Maybe this way it will work better

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Seems like a good compromise to me. Unless we ever name the branch "main" right?

  • catch committed a1334300 on 11.x 
    Issue #3365970 by andypost, Spokje, dww: Fix minor version parsing in...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, thanks!

Status: Fixed » Closed (fixed)

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