Problem/Motivation

The method Drupal\Core\Routing\RequestContext::fromRequest() should be return $this. The class is overriding the Symfony class Symfony\Component\Routing\RequestContext. See: https://github.com/symfony/symfony/blob/4.4/src/Symfony/Component/Routin....

Steps to reproduce

Proposed resolution

Add the statement return $this; to the method.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#18 3233047-18.patch1.44 KBlongwave
#18 3233047-18-test-only.patch733 byteslongwave
#15 3233047-15.patch1.11 KBdaffie
#15 interdiff-3233047-7-15.txt688 bytesdaffie
#7 3233047.5_7.interdiff.txt585 bytesdww
#7 3233047-7.patch1.12 KBdww
#5 3233047-5.patch1.1 KBdaffie
#5 interdiff-3233047-2-5.txt672 bytesdaffie
#2 3233047-2.patch459 bytesdaffie
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

daffie created an issue. See original summary.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Active » Needs review
FileSize
3233047-2.patch459 bytes

The fix.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

I was going to suggest adding a test, but #3232892: [Symfony 6] Add "static" type hint to the method Drupal\Core\Routing\RequestContext::fromRequest() will ensure that the return type is correct without adding a test (albeit not until PHP 8), so I think this is OK as is.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Needs review

Wow, this is an odd method.

It is named like a factory (from[Something]) but actually mutates the objects rather than returning a new instance.

My first thought was, well this must be unused, but it is used fairly heavily in core, just that we don't chain it.

Instead of adding a test, we could update one of the existing usages to chain, e.g \Drupal\Core\Routing\UrlMatcher::finalMatch would be a good candidate

daffie’s picture

daffie CreditAttribution: daffie commented
FileSize
interdiff-3233047-2-5.txt672 bytes
3233047-5.patch1.1 KB

Add the change to the method \Drupal\Core\Routing\UrlMatcher::finalMatch(). The change is the method only works when Drupal\Core\Routing\RequestContext::fromRequest() returns $this. @larowlan: Good idea.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Needs work 
PHP Parse error:  syntax error, unexpected token "->" in /var/www/html/core/lib/Drupal/Core/Routing/UrlMatcher.php on line 38

Needs () around the new RequestContext().

edit; and this doesn't test this method return value anyway, you want something like ->fromRequest($request)->setContext($context)

dww’s picture

dww
we/he/they
CreditAttribution: dww at TEN7 commented
Status: Needs work » Needs review
Issue tags: +Bug Smash Initiative
FileSize
3233047-7.patch1.12 KB
3233047.5_7.interdiff.txt585 bytes
Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Itty Bitty Byte for Salsa Digital commented
Status: Needs review » Reviewed & tested by the community

Thank you for the updated patch.

1. Read through the issue and comments.

2. Looked at the interdiff and see that the changes in #7 address the feedback in #6.

3. Tests pass and no new tests are to be added based on the discussion above.

4. Reviewed the patch itself and it seems clear and addresses the issue in the issue summary.

5. No obvious way to test this manually so relying on automated tests.

Marking RTBC. Thanks, all!

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Issue credits

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

Committed ed5384a and pushed to 9.3.x. Thanks!

  • larowlan committed ed5384a on 9.3.x 
    Issue #3233047 by daffie, dww, longwave, Kristen Pol: Drupal\Core\...
daffie’s picture

daffie CreditAttribution: daffie commented 
+++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
@@ -35,9 +35,7 @@ public function __construct(CurrentPathStack $current_path) {
-    $context = new RequestContext();
-    $context->fromRequest($request);
-    $this->setContext($context);
+    $context = (new RequestContext())->fromRequest($request)->setContext($context);

Maybe I am missing the point, but is this change not the wrong one? Where does the parameter value for the method setContext() come from? The value of $context only exists after this line. Should it not be:

$context = (new RequestContext())->fromRequest($request);
$this->setContext($context);

The method fromRequest() returns itself. This is an instance of Drupal\Core\Routing\RequestContext. In the call $this->setContext($context); is $this an instance of Drupal\Core\Routing\UrlMatcher.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Fixed » Needs work

Yeah, you're right @daffie

  • larowlan committed 45a503a on 9.3.x 
    Revert "Issue #3233047 by daffie, dww, longwave, Kristen Pol: Drupal\...
daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs work » Needs review
FileSize
interdiff-3233047-7-15.txt688 bytes
3233047-15.patch1.11 KB
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Needs work

Sorry, that was me misreading the code in #6.

But this begs the question: why does #7 not fail? And it seems the answer is that this is dead code.

  public function finalMatch(RouteCollection $collection, Request $request) {

finalMatch is not part of the interface, and this is the only instance of this string in core.

I think we need a followup to remove this method, and to find another suitable candidate to test this change in this issue.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Related issues: +#3238199: \Drupal\Core\Routing\UrlMatcher is dead code

Filed #3238199: \Drupal\Core\Routing\UrlMatcher is dead code

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Needs review
FileSize
3233047-18-test-only.patch733 bytes
3233047-18.patch1.44 KB

PathValidator uses RequestContext::fromRequest() so we can rework that instead.

Also refactored RequestContext::fromRequest() to just return the result of the parent call.

daffie’s picture

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

Looks good to me.

  • catch committed 4c0d316 on 9.3.x 
    Issue #3233047 by daffie, longwave, dww, larowlan, Kristen Pol: Drupal\...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed 4c0d316 and pushed to 9.3.x. Thanks!

Status: Fixed » Closed (fixed)

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