Problem/Motivation
The Change Record for PHP attribute-based route discovery documents support for invokable controllers where #[Route] is placed only on the class, with __invoke() having no attribute:
namespace Drupal\router_test\Controller;
use Drupal\Core\Controller\ControllerBase;
use Symfony\Component\Routing\Attribute\Route;
/**
* Test controller.
*/
#[Route(
'/test_class_attribute',
'test_class_attribute',
requirements: ['_access' => 'TRUE']
)]
class TestClassAttribute extends ControllerBase {
/**
* Provides test content.
*/
public function __invoke() {
return ['#markup' => 'Testing __invoke() with a Route attribute on the class'];
}
}
This pattern does NOT work. The route is never registered.
AttributeRouteDiscovery::getRoutes() contains the following condition (line 122):
if ($collection->count() && $class->hasMethod('__invoke') === 0) {
ReflectionClass::hasMethod() returns bool. TRUE === 0 and FALSE === 0 are both FALSE under strict comparison. The condition is always FALSE, making the entire block dead code — the invokable controller fallback never executes.
Additionally, the first operand likely has the wrong polarity: the block should activate when NO routes were collected from method attributes !$collection->count(), not when some were.
Steps to reproduce
- Create a controller with #[Route] only on the class, __invoke() with no attribute.
- Clear caches.
- Visit the route path — get a 404.
Proposed resolution
The correct condition should be:
if (!$collection->count() && $class->hasMethod('__invoke')) {
Note that this was already fixed incidentally in #3584793: Use PHP attributes for form route discovery, but that was committed only to main and 11.x (11.5, essentially), and without test coverage.
11.4.x needs the fix and test coverage.
main and 11.x need the test coverage only.
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Issue fork drupal-3593939
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:
Comments
Comment #3
niklanComment #4
longwaveFWIW this was also discovered in #3584793-3: Use PHP attributes for form route discovery and has been fixed over there, so if that lands first this issue can be closed.
Comment #5
longwaveAlthough if we didn't add an explicit test over there that would be helpful to keep from here.
Comment #6
niklanComment #7
smustgrave commentedThe other issue is RTBC and probably good to get that one in first. Postponing this one so we can add expanded test coverage (unless we should move the tests over).
Moved credit for niklan over regardless.
Comment #8
avpadernoProbably that line was supposed to be
if ($collection->count() === 0 && $class->hasMethod('__invoke')) {, to whichif (!$collection->count() && $class->hasMethod('__invoke')) {is preferable.Comment #9
godotislate#3584793: Use PHP attributes for form route discovery, but for 11.5/12 only is in, so I think it makes sense to do the specific fix here for 11.4 and add explicit test coverage.
Comment #10
godotislateComment #13
niklanLooks like I messed up on https://git.drupalcode.org/project/drupal/-/merge_requests/16002, so I've created a new MR against the 11.4.x branch: https://git.drupalcode.org/project/drupal/-/merge_requests/16197. It contains the same fixes but also takes into account the recommendation from comment #8.
Comment #14
godotislateCan you open another MR against
mainwith only the test changes as well?Comment #15
smustgrave commentedper #14
Comment #17
niklanDone: https://git.drupalcode.org/issue/drupal-3593939/-/tree/3593939-main-test...
Comment #18
smustgrave commentedCan the summary be updated to mention this is only for 11.4.x and why not needed in 11.5.x and main please
Comment #19
godotislateUpdated IS.
Comment #20
smustgrave commentedThanks sorry to be nitpicky just thought good practice. Changes look good for the additional test coverage. Know there’s some regressions for 11.4 coming so maybe we can get this bundled with.
Comment #25
godotislateTest only for main passes as expected https://git.drupalcode.org/issue/drupal-3593939/-/jobs/10720291
Test only for 11.4 fails as expected: https://git.drupalcode.org/project/drupal/-/jobs/10852013
Committed and pushed b599308 to main and 6e2fb88 to 11.x.
Going to wait a bit for things to settle after the 11.4.2 release to push the 11.4.x commit.
Comment #27
godotislateAnd pushed to 11.4.x