Catch potential exception when calling Request::create() in PathBasedBreadcrumbBuilder

The wording seems like maybe a regression was introduced? Think we should have test coverage for the scenario described

Test coverage looks good https://git.drupalcode.org/issue/drupal-3490710/-/jobs/3540197

Committed/pushed to 11.x and cherry-picked back through to 10.3.x, thanks!

catch → closed merge request !10398

This broken HEAD on 10.5.x and 10.4.x with a cheery:

$ vendor/bin/phpunit -c core core/modules/system/tests/src/Unit/Breadcrumbs/PathBasedBreadcrumbBuilderTest.php
PHPUnit 9.6.21 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\system\Unit\Breadcrumbs\PathBasedBreadcrumbBuilderTest
...........                                                       11 / 11 (100%)

Time: 00:00.257, Memory: 12.00 MB

OK (11 tests, 56 assertions)

Remaining direct deprecation notices (2)

  2x: Since symfony/http-foundation 6.3: Calling "Symfony\Component\HttpFoundation\Request::create()" with an invalid URI is deprecated.
    2x in PathBasedBreadcrumbBuilderTest::testBuildWithInvalidPath from Drupal\Tests\system\Unit\Breadcrumbs

Remaining indirect deprecation notices (1)

  1x: Automatic conversion of false to array is deprecated
    1x in PathBasedBreadcrumbBuilderTest::testBuildWithInvalidPath from Drupal\Tests\system\Unit\Breadcrumbs

Unsure why it works on 11.x with Symfony 7.2, where I would expect the deprecation throwing code would be removed, but it does.

As an added weirdness-bonus, the test fails, making the unit-test job fail on GitLab, but the specific error doesn't show up in the report.
I can only find it in the raw output of the unit-test job.

Reverted from 10.5.x and 10.4.x

_tips hat_

So it looks like this is specifically for the case where parse_url() returns FALSE.

I was discussing this over in https://www.drupal.org/project/drupal/issues/3489329#comment-15880688 🐛 Drupal core update + update to symfony/http-foundation causes certain views blocks to cause a "client error" and breaks page display. Active

The PHP docs for parse_url() say "This function may not give correct results for relative or invalid URLs, and the results may not even match common behavior of HTTP clients. If URLs from untrusted input need to be parsed, extra validation is required, e.g. by using filter_var() with the FILTER_VALIDATE_URL filter."

So, technically speaking, maybe Request::create() should not rely on parse_url() for parsing relative URLs, or Drupal core should not rely on Request::create() to parse relative URLs/paths.

However, so far I've only noticed parse_url() returning FALSE for "weird" relative URLs (in my case, requests made by a bot), not something that I would have expected to work. So a quick fix could be to test the relative URL with parse_url() and return NULL, before calling Request::create()

I think this can be fixed on all branches by simply updating the symfony/http-foundation requirement to a more recent version that throws the exception, rather than triggering the deprecation? I.e. bump from 6.4.15 to at least 6.4.16?

My workaround in #20 would only be needed if there is a requirement to support older versions of symfony/http-foundation

We need to do that anyway, makes sense to me.

symfony/http-foundation was updated in 📌 Update Composer dependencies for 10.4.0-RC1 Active so from here I have just committed the fix and test.

Committed and pushed c6143563c31 to 10.5.x and 3116614f706 to 10.4.x. Thanks!

longwave → closed merge request !10485

So now there's only a choice between either running an insecure 10.3 application or live with an 10.3 application having the out of memory loop issue?

What is the point of having a security supported core version if you cannot update to secure symfony dependencies?

So now there's only a choice between either running an insecure 10.3 application or live with an 10.3 application having the out of memory loop issue?

What is the point of having a security supported core version if you cannot update to secure symfony dependencies?

My bad. The loop with the out-of-memory was mentioned in 🐛 DefaultExceptionHtmlSubscriber should not clone the request for 401s Active .

The point more or less remains though. Without this fix in 10.3.x it's a choice between secure dependencies or a pretty broken site.
Granted that out of memory seems to be more of a problem then "just" an exception.

I think we can probably backport this to 10.3.x too.

Seems to have broken HEAD of 11.0.x: https://git.drupalcode.org/project/drupal/-/pipelines/382966/test_report...

Reverted from 11.0.x too.

What's the policy for security updates of dependencies on security-only branches? Assuming it's ok and/or required to do so, the composer update could be applied on that branch

@mfb we can update, but this was quite a minor security fix (at least for us) whereas the function regressions have been quite serious, and there are multiple contexts where this error can be thrown, so I would personally prefer us to keep fixing these bugs and then only update the constraints if we really need to - individual sites can update with composer update whenever they want.