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

  1. Create a controller with #[Route] only on the class, __invoke() with no attribute.
  2. Clear caches.
  3. 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

Command icon 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

niklan created an issue. See original summary.

niklan’s picture

Status: Active » Needs review
longwave’s picture

FWIW 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.

longwave’s picture

Although if we didn't add an explicit test over there that would be helpful to keep from here.

niklan’s picture

smustgrave’s picture

Status: Needs review » Postponed
Issue tags: +Needs issue summary update
Related issues: +#3584793: Use PHP attributes for form route discovery

The 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.

avpaderno’s picture

Probably that line was supposed to be if ($collection->count() === 0 && $class->hasMethod('__invoke')) {, to which if (!$collection->count() && $class->hasMethod('__invoke')) { is preferable.

godotislate’s picture

Status: Postponed » Needs work

#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.

godotislate’s picture

Version: main » 11.4.x-dev

niklan’s picture

Status: Needs work » Needs review

Looks 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.

godotislate’s picture

Can you open another MR against main with only the test changes as well?

smustgrave’s picture

Status: Needs review » Needs work

per #14

niklan’s picture

smustgrave’s picture

Status: Needs review » Needs work

Can the summary be updated to mention this is only for 11.4.x and why not needed in 11.5.x and main please

godotislate’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update

Updated IS.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Thanks 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.

  • godotislate committed b599308d on main
    fix: #3593939 AttributeRouteDiscovery: invokable controllers with class-...

  • godotislate committed 6e2fb887 on 11.x
    fix: #3593939 AttributeRouteDiscovery: invokable controllers with class-...
godotislate’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Test 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.

  • godotislate committed 94acbe95 on 11.4.x
    fix: #3593939 AttributeRouteDiscovery: invokable controllers with class-...
godotislate’s picture

Status: Patch (to be ported) » Fixed

And pushed to 11.4.x

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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