Problem/Motivation

Method "Symfony\Component\Routing\RequestContextAwareInterface::getContext()" will return "RequestContext" as of its next major version. Doing the same in implementation "Drupal\Core\Routing\AccessAwareRouter" will be required when upgrading.

Method "Symfony\Component\Routing\RequestContextAwareInterface::getContext()" will return "RequestContext" as of its next major version. Doing the same in implementation "Drupal\Core\Routing\UrlGenerator" will be required when upgrading.

Method "Symfony\Component\Routing\RequestContextAwareInterface::getContext()" will return "RequestContext" as of its next major version. Doing the same in implementation "Drupal\Core\Render\MetadataBubblingUrlGenerator" will be required when upgrading.

Steps to reproduce

Proposed resolution

Add the "RequestContext" return type hint.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#14 interdiff.3233031.10-14.txt1.06 KBlongwave
#14 3233031-14.patch5.65 KBlongwave
#10 interdiff_9-10.txt503 bytesmurilohp
#10 3233031-10.patch5.05 KBmurilohp
#9 3233031-9.patch4.76 KBlongwave
#4 3233031-4.patch2.06 KBdaffie
#4 interdiff-3233031-2-4.txt470 bytesdaffie
#2 3233031-2.patch1.6 KBdaffie

Comments

daffie created an issue. See original summary.

daffie’s picture

daffie commented
Status: Active » Needs review
StatusFileSize
new 3233031-2.patch1.6 KB

I am not sure that the fix will work. In all 3 classes we are already have the use-statement: use Symfony\Component\Routing\RequestContext as SymfonyRequestContext;. My question will the Symfony deprecation code accept "SymfonyRequestContext" instead of the expected "RequestContext".

daffie’s picture

daffie commented

I do not think we can do this in a 9.x branch as there over 170 time the method with the name "getContext()" is overridden in contrib. I do not know how many of those are overriding the method Symfony\Component\Routing\RequestContextAwareInterface::getContext(). Maybe be careful and commit this in D10.0. See: http://grep.xnddx.ru/search?text=public%20function%20getContext%28&filen....

daffie’s picture

daffie commented
StatusFileSize
new interdiff-3233031-2-4.txt470 bytes
new 3233031-4.patch2.06 KB

Adding the return type hint to Drupal/Core/Routing/NullGenerator::getContext().

daffie’s picture

daffie commented
Version: 9.3.x-dev » 10.0.x-dev

Moving this to the 10.0 branch.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented 
  1. +++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
@@ -76,7 +76,7 @@ public function setContext(SymfonyRequestContext $context) {
+  public function getContext(): SymfonyRequestContext {
     if ($this->router instanceof RequestContextAwareInterface) {
       return $this->router->getContext();
     }

    This is breaking the contract and returning void if the instanceof check does not pass. 

  2. +++ b/core/lib/Drupal/Core/Routing/NullGenerator.php
@@ -65,7 +65,7 @@ public function setContext(SymfonyRequestContext $context) {
+  public function getContext(): SymfonyRequestContext {
   }

    This is breaking the contract because it always returns void.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Upstream seems to deal with this by keeping a copy of the context inside the object so it can return it if it's not available from elsewhere.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review
Related issues: +#3248859: AccessAwareRouter fails PHPStan-0
StatusFileSize
new 3233031-9.patch4.76 KB

This is similar to #3248859: AccessAwareRouter fails PHPStan-0, the instanceof checks here are wrong I think - AccessAwareRouter should receive an instance of RouterInterface and then it never has to worry about whether the router implements interfaces or not, they are all combined into RouterInterface.

murilohp’s picture

murilohp commented
StatusFileSize
new 3233031-10.patch5.05 KB
new interdiff_9-10.txt503 bytes

Hey @longwave I'm uploading a new patch removing the use statement that is not required anymore, I hope this time the build doesn't fail.

Thanks!

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Thank you!

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue tags: +PHPStan-0
daffie’s picture

daffie commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Routing/AccessAwareRouter.php
@@ -69,18 +67,14 @@ public function __call($name, $arguments) {
   public function getContext() {

Can we change this line to: public function getContext(): SymfonyRequestContext {. To fix the problem as stated in the IS.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review
StatusFileSize
new 3233031-14.patch5.65 KB
new interdiff.3233031.10-14.txt1.06 KB

Addressed #13 and found another case where we need to add the return type in a test.

daffie’s picture

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

The change to requiring the Symfony\Component\Routing\RouterInterface in the Drupal\Core\Routing\AccessAwareRouter makes that all previous optional calls are now no longer optional. The router object is now allways of the type RequestContextAwareInterface, RouterInterface and UrlGeneratorInterface.

All the code changes look good to me.
For me it is RTBC.

  • catch committed 3eb8a34 on 10.0.x 
    Issue #3233031 by daffie, longwave, murilohp: [Symfony 6] Add "...
catch’s picture

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

Debated whether we need a change record, but given this is a constructor change, it's 10.0.x only, and the likelihood of a router not implementing RouterInterface is extremely low, I think it'd be noise rather than useful.

Committed 3eb8a34 and pushed to 10.0.x. Thanks!

Status: Fixed » Closed (fixed)

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