Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
From issue #3178534: Start running PHPStan on Drupal core (level 0): AccessAwareRouter fails PHPStan level 0 checks since a couple of methods may not respect the return typehint signature.
Proposed resolution
I think we should add
if (!$router instanceof RouterInterface) { throw new \InvalidArgumentException(); }to the constructor. And document the router param and class property to be
\Symfony\Component\Routing\Matcher\RequestMatcherInterface|\Symfony\Component\Routing\RouterInterface
as that is what it is.
And then remove all the other instanceof checks in this class.
And then open a D10 issue to change the typehint to\Symfony\Component\Routing\Matcher\RequestMatcherInterface|\Symfony\Component\Routing\RouterInterfacebecause union types are supported in PHP 8.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | 3248859-15.patch | 3.14 KB | longwave |
Issue fork drupal-3248859
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3248859-accessawarerouter-fails-phpstan-0
changes, plain diff MR !1408
Comments
Comment #2
longwaveUnion types are OR, isn't this an AND - ie. the router has to implement both RequestMatcherInterface and RouterInterface?
Should we create our own
interface RouterInterface extends RequestMatcherInterface, SymfonyRouterInterface {}and use that?Comment #3
mondrakeComment #5
mondrakeComment #6
mondrakeMade some changes
Comment #7
longwavegenerate()belongs to the Symfony interface though, so it doesn't necessarily need to be a Drupal UrlGenerator? Is PHPStan satisfied if we just remove that check?Comment #8
mondrakeKeep things simple, agree
Comment #9
longwaveLooks OK to me. Still wonder if we should introduce our own interface that combines the two, but I guess that is a bigger change.
Comment #10
daffie commentedI am also worried that might cause a BC break on some sites.
Comment #11
mondrakeSorry, I do not get how this would break BC. Can you point to a case? Right now, $router not implementing the two interfaces would throw a big fat TypeError. To me, we’re likely just removing previous BC layer cruft.
Comment #12
mondrakeComment #13
daffie commentedCould you explain how/when that is happening. I am probably missing the point. The methods added by the RouterInterface have all checks for that $this->router supports the interface and if not then NULL is returned. I just do not see when a "big fat TypeError" is being thrown. Please explain.
Comment #14
longwave#3233031: [Symfony 6] Add "RequestContext" type hint to methods overridding Symfony\Component\Routing\RequestContextAwareInterface::getContext() is now related to this, because we need to add return types, and the instanceof check means that return type cannot always be satisfied - but I think we are using too many interfaces here and we should just accept a RouterInterface only.
\Symfony\Component\Routing\RouterInterface extends \Symfony\Component\Routing\Generator\UrlGeneratorInterface and \Symfony\Component\Routing\Matcher\UrlMatcherInterface.
\Symfony\Component\Routing\Generator\UrlGeneratorInterface and \Symfony\Component\Routing\Matcher\UrlMatcherInterface both extend Symfony\Component\Routing\RequestContextAwareInterface.
This means that RouterInterface actually implements three interfaces: UrlGeneratorInterface, UrlMatcherInterface and RequestContextAwareInterface.
The first argument to AccessAwareRouter::__construct is \\Drupal\Core\Routing\Router which already fully implements RouterInterface.
Therefore, I think AccessAwareRouter::__construct should be changed to accept RouterInterface instead of RequestMatcherInterface, and we can remove all the instanceof checks in this class.
Comment #15
longwaveComment #16
longwave#3233031: [Symfony 6] Add "RequestContext" type hint to methods overridding Symfony\Component\Routing\RequestContextAwareInterface::getContext() was committed to 10.0.x with the change to RouterInterface, unsure it's worth doing in 9.4.x, maybe just close this as duplicate?
Comment #17
mondrakeYes. AFAICS PHPStan will be a D10+ thing.