UnroutedUrlTest is failing on dev versions of PHP

It seems to be only on the environments with dev versions of PHP (7.3.x-dev and 7.4.x-dev both) so maybe an upstream change or regression impacting the test? It's been broken since January 4.

This was also on the 7.2.x-dev environment, oddly, because 7.2.x is no longer supported, and shouldnt have had any new commits, yet here it is: https://github.com/php/php-src/commit/2d3d72412a6734e19a38ed10f385227a62...

So that commit probably broke our test.

Also:

https://bugs.php.net/bug.php?id=77423

Is the actual bug report.

This fix is included in 7.4.14 and 7.3.26 https://www.php.net/ChangeLog-7.php#7.4.14 and 8.0.1 https://www.php.net/ChangeLog-8.php#8.0.1

This is related to a CVE, so we definitely need to fix it.

@Mixologic what patch versions of PHP are the 7.4 and 7.3 environments running?

For testing this locally

docker pull php:7.4-alpine
docker run --rm -it -v /path/to/where/you/keep/your/drupal/core:/data php:7.4-alpine sh
cd /data
// Assuming you have core checked out in /path/to/where/you/keep/your/drupal/core/app
./app/vendor/bin/phpunit -c app/core app/core/tests/Drupal/Tests/Core/UnroutedUrlTest.php

Here's the output

PHPUnit 8.5.13 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Core\UnroutedUrlTest
...............F................................................. 65 / 80 ( 81%)
...............                                                   80 / 80 (100%)

Time: 1.4 minutes, Memory: 8.00 MB

There was 1 failure:

1) Drupal\Tests\Core\UnroutedUrlTest::testFromInvalidUri with data set #6 ('base://AKI@&hO@')
Failed asserting that exception of type "InvalidArgumentException" is thrown.

/data/app/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:118
/data/app/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:54
/data/app/vendor/phpunit/phpunit/src/Framework/Assert.php:2887
/data/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:691
/data/app/vendor/phpunit/phpunit/src/Framework/TestSuite.php:601
/data/app/vendor/phpunit/phpunit/src/Framework/TestSuite.php:601
/data/app/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:633
/data/app/vendor/phpunit/phpunit/src/TextUI/Command.php:204
/data/app/vendor/phpunit/phpunit/src/TextUI/Command.php:163

FAILURES!
Tests: 80, Assertions: 81, Failures: 1.

So previously `base://AKI@&hO@` was something parse_url couldn't parse, but now it can and is treated as valid.

So I guess we just remove it? This was introduced in #2418169: Expand and document test coverage in UnroutedUrlTest but there's not really any discussion around why that case was needed.

Correct parse_url() can now parse that value. Not sure of the value of the test, so I agree it can be removed.

Here's the output of parse_url('base://AKI@&hO@') with different versions of PHP to demonstrate.

https://3v4l.org/qQm1i

#2418169: Expand and document test coverage in UnroutedUrlTest just moved the pattern from one place in the data provider to another. The string was being tested before.

Before we commit this, let's blame the line in HEAD^ from that commit to see what the actual original issue was?

This is probably still the right approach but we should be sure.

Committed dc90906 and pushed to 9.2.x. Thanks!

Will backport to 9.1.x and 8.9.x once the bugfix release has taken place.

Hmm based on #11 I think it would have been better to move the string to the correct test for disallowed patterns rather than removing it entirely?

It was already in the disallowed list, but PHP now allows it, per the link in Kim's comment

Sorry; you know what I mean -- shouldn't it still be tested somewhere?

Backported to 8.9.x and 9.1.x

Missed #6 but in the future, you can always check the version of php that was used by the test by going to the artifacts on dispatcher:

e.g: https://www.drupal.org/pift-ci-job/1963069 -> click on 'view results on dispatcher' -> click on 'Build Artifacts' -> click on 'artifacts' -> click on 'runcontainers' -> and there you will see the output of phpinfo: https://dispatcher.drupalci.org/job/drupal_patches/75398/artifact/jenkin...