Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
These routes have been wrong since they were added (in #2362517: Improve default 403/404 exception HTML subscriber: don't duplicate the page render pipeline, use the routing system — add system.403 and system.404 routes and #2472371: Exception shown on 401 Unauthorized.
The problem lies in \Drupal\Core\Controller\ControllerResolver::createController() It claims to want either service:method or class::method, note 1 vs 2 colons. From #2.
Proposed resolution
Fix them
Remaining tasks
Find out why HEAD is passing, and write tests
Add exception messages.
A title change
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#27 | interdiff_25-27.txt | 2.12 KB | ravi.shankar |
#27 | 2507885-27.patch | 4.79 KB | ravi.shankar |
#25 | interdiff_23-25.txt | 1.06 KB | pooja saraah |
#25 | 2507885-25.patch | 4.92 KB | pooja saraah |
#23 | 2507885-23.patch | 4.86 KB | ravi.shankar |
Comments
Comment #1
tim.plunkettComment #2
tim.plunkettThe problem lies in \Drupal\Core\Controller\ControllerResolver::createController()
It claims to want either
service:method
orclass::method
, note 1 vs 2 colons.When originally added in #1875504: Add the possibility to use services as controllers, it followed that expectation.
But when #2165475: Provide a generic class resolver went in, it now will accept a class or a service, followed by either : or ::, and a method name.
I only noticed because PHPStorm was smart enough to flag it.
Should we rewrite ControllerResolver::createController() to be more strict again?
If not, we cannot write tests for this.
Comment #3
dawehnerI think we should make it strict and throw a damn stupid exception
Note: https://github.com/symfony/symfony/blob/2aeaa22e3171b5e1f10f2cc2a7ff7f66... also does not support that directly ...
Comment #4
Crell CreditAttribution: Crell at Palantir.net commentedYes, let's be strict about it. Consistent expectations. But don't throw a damned stupid exception. Throw a smart, helpful exception that tells developers what they screwed up. :-)
Comment #5
dawehnerOh yeah with stupid I meant a in your face exception, something which fails early, as long you still have the context available.
Comment #6
tim.plunkettNow ControllerResolver *and* ClassResolver are ContainerAware, and both have $this->container->has() and class_exists() checks.
I did not spend time picking an exception class or writing a message
Comment #8
tim.plunkettwhoops :)
Comment #9
mgiffordComment #21
larowlanIs this still relevant?
Comment #22
quietone CreditAttribution: quietone at PreviousNext commentedYes, this is still valid, system.routing.yml has Controllers with a single ':' instead of '::'.
Comment #23
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded reroll of patch #8 on Drupal 9.4.x.
Comment #24
andypostinstead of NOPE it would be helpful to show "$class_or_service"
still needs re-roll as this annotations are deprecated
Comment #25
pooja saraah CreditAttribution: pooja saraah at Srijan | A Material+ Company for Drupal India Association commentedFixed failed commands on #23
Attached interdiff
Comment #26
andypostExpected exception should be in code, not in doc-blocks
Comment #27
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedMade changes as per comment #24, please review.
Comment #29
andypostIn related #2531564: Fix leaky and brittle container serialization solution there could be a special method to detect public services in container