Problem/Motivation

Running core/tests/Drupal/KernelTests/Core/Path/PathValidatorTest.php on PHP 8.1 triggers a deprecation.

Steps to reproduce

Run core/tests/Drupal/KernelTests/Core/Path/PathValidatorTest.php

Proposed resolution

Fix the CLI request method return a string value. There is no way that \Symfony\Component\Routing\RequestContext::$method can be anything other than a string.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#8 3241296-7.patch1.3 KBandypost
#8 interdiff.txt734 bytesandypost
#7 3241296-6.patch1.08 KBandypost
#7 interdiff.txt1.07 KBandypost
#3 3241296-3.patch1.12 KBandypost
#3 interdiff.txt1.24 KBandypost
#2 3241296-2.patch688 bytesalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 3241296-2.patch688 bytes

Before

PHPUnit 9.5.10 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.1.0RC3-dev
Configuration: /Users/alex/dev/sites/drupal8alt.dev/phpunit.xml
Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

Testing Drupal\KernelTests\Core\Path\PathValidatorTest
.                                                                   1 / 1 (100%)

Time: 00:01.811, Memory: 10.00 MB

OK (1 test, 27 assertions)

Unsilenced deprecation notices (1)

  1x: strtoupper(): Passing null to parameter #1 ($string) of type string is deprecated
    1x in PathValidatorTest::testGetUrlIfValidWithoutAccessCheck from Drupal\KernelTests\Core\Path

After

PHPUnit 9.5.10 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.1.0RC3-dev
Configuration: /Users/alex/dev/sites/drupal8alt.dev/phpunit.xml
Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

Testing Drupal\KernelTests\Core\Path\PathValidatorTest
.                                                                   1 / 1 (100%)

Time: 00:01.706, Memory: 10.00 MB

OK (1 test, 27 assertions)
andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt1.24 KB
new 3241296-3.patch1.12 KB

Found this while digging issue https://symfony.com/blog/new-in-symfony-5-1-simpler-request-context

The failure is because \Symfony\Component\Routing\RequestContext::setMethod() makes strtoupper() also the method expects non-nullable string in SF 4/5/6 https://github.com/symfony/symfony/blob/6.0/src/Symfony/Component/Routin...

I find better to prevent passing NULL as it's not supposed by \Symfony\Component\Routing\RequestContext::getMethod() and expect the default NULL as default string.

@alexpott I wondered why next$this->assertEquals($method, $requestContext->getMethod()); compares NULL with '' without notices on 8.1rc3

andypost’s picture

andypost
he/him
Primary language Russian
commented
Component: base system » request processing system

proper component

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#2822190: PathValidator validates based on a RequestContext leaked from the current request, resulting in false negatives during CLI requests and POST submissions, +#2901572: Fix 'Drupal.Commenting.PostStatementComment' coding standard

@alexpott This code comments to NULL and FALSE been added #2901572: Fix 'Drupal.Commenting.PostStatementComment' coding standard but test values exists since #2822190-40: PathValidator validates based on a RequestContext leaked from the current request, resulting in false negatives during CLI requests and POST submissions you, @WimLeers @catch recalls...

andypost’s picture

andypost
he/him
Primary language Russian
commented

The test added by @WIm Leers #2822190-40: PathValidator validates based on a RequestContext leaked from the current request, resulting in false negatives during CLI requests and POST submissions

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt1.07 KB
new 3241296-6.patch1.08 KB
andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt734 bytes
new 3241296-7.patch1.3 KB

I think this was missed in initial test implementation

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 3241296-2.patch688 bytes

@andypost all #8 is test $method being equal to a string twice. Yes at some point the FALSE is going to produce a deprecation but atm it does not.

Let's go back to #2 and deal with the FALSE when and if a deprecation occurs for that.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

$this->assertEquals($method, $requestContext->getMethod()); we be true without notices when the values are 0, '', NULL, FALSE ... it's assertEquals and not assertSame

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

As stated in the issue summary

There is no way that \Symfony\Component\Routing\RequestContext::$method can be anything other than a string.

because of the strtoupper() and the way \Symfony\Component\Routing\RequestContext() works.

daffie’s picture

daffie commented
Status: Needs review » Needs work 
  1. +++ b/core/tests/Drupal/KernelTests/Core/Path/PathValidatorTest.php
@@ -59,9 +60,15 @@ public function testGetUrlIfValidWithoutAccessCheck() {
+        $requestContext = new RequestContext();

    Why are we doing this change instead of $requestContext = \Drupal::service('router.request_context');? 

  2. +++ b/core/tests/Drupal/KernelTests/Core/Path/PathValidatorTest.php
@@ -59,9 +60,15 @@ public function testGetUrlIfValidWithoutAccessCheck() {
-        $this->container->set('router.request_context', new RequestContext());
+        $requestContext = new RequestContext();
+        $this->container->set('router.request_context', $requestContext);

    Is this change really necessary? If so, can we then remove the first line of this test with: $requestContext = \Drupal::service('router.request_context');. That variable is not used anywhere.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@daffie I think you reviewed #8 and not #9.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
daffie’s picture

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

@alexpott: Yes ,you are right. I reviewed the wrong patch.

The patch from #9 looks good to me.

  • catch committed 397d199 on 9.3.x 
    Issue #3241296 by andypost, alexpott: core/tests/Drupal/KernelTests/Core...
catch’s picture

catch
he/him
Primary language English
commented

Committed 397d199 and pushed to 9.3.x. Thanks!

catch’s picture

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

Status: Fixed » Closed (fixed)

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