Problem/Motivation

#3311365: Use PHP attributes for route discovery added Attribute based discovery.

In TFA 1.x we have classes that extend Core classes where signatures may from time to time change, in TFA we thus have to selectively choose which class is loaded based on the version of parent class. We do so in a Route Alter Subscriber.

In 11.4.x it appears that the new processor attempts to load all classes (including 'malformed' classes) and does not properly catch errors that may occur.

Similar occurred for missing traits #3502913: Add a fallback classloader that can handle missing traits for attribute discovery

PHP Fatal error: Declaration of Drupal\tfa\Controller\TfaUserControllerDeprecated::resetPassLogin($uid, $timestamp, $hash) must be compatible with Drupal\user\Controller\UserController::resetPassLogin($uid, $timestamp, $hash, Symfony\Component\HttpFoundation\Request $request) in /var/www/html/web/modules/contrib/tfa/src/Controller/TfaUserControllerDeprecated.php on line 15

10	4.0281	14888440	Drupal\Core\Routing\AttributeRouteDiscovery->collectRoutes( )	.../StaticRouteDiscoveryBase.php:93
11	4.0281	14888440	Drupal\Core\Routing\AttributeRouteDiscovery->createRouteCollection( $className = 'Drupal\\tfa\\Controller\\TfaUserControllerDeprecated' )	.../AttributeRouteDiscovery.php:52
12	10.2775	14899488	class_exists( $class = 'Drupal\\tfa\\Controller\\TfaUserControllerDeprecated' )	.../AttributeRouteDiscovery.php:71
13	10.2776	14899600	Composer\Autoload\ClassLoader->loadClass( $class = 'Drupal\\tfa\\Controller\\TfaUserControllerDeprecated' )	.../AttributeRouteDiscovery.php:71
14	10.2777	14899712	{closure:/var/www/html/vendor/composer/ClassLoader.php:575-577}( $file = '/var/www/html/web/modules/custom/tfa/src/Controller/TfaUserControllerDeprecated.php' )	.../ClassLoader.php:427

Steps to reproduce

Install TFA 8.x-1.12 on Core 11.x4.x, visit core/rebuild.php or drush cr

Proposed resolution

Catch errors during attribute scanning and continue on operations.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3592887

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:

About issue forks

Comments

cmlara created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

What's the actual exception/error message here?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Title: AttributeRouteDiscovery dos not cleanly handle invalid classes » AttributeRouteDiscovery does not cleanly handle invalid classes
Related issues: +#3584793: Use PHP attributes for form route discovery

Also I think #3584793: Use PHP attributes for form route discovery might contain a fix for this, can you check if applying the patch from there solves this please?

cmlara’s picture

cmlara
he/him
commented
Issue summary: View changes

Sorry failed to copy the error message:

PHP Fatal error: Declaration of Drupal\tfa\Controller\TfaUserControllerDeprecated::resetPassLogin($uid, $timestamp, $hash) must be compatible with Drupal\user\Controller\UserController::resetPassLogin($uid, $timestamp, $hash, Symfony\Component\HttpFoundation\Request $request) in /var/www/html/web/modules/contrib/tfa/src/Controller/TfaUserControllerDeprecated.php on line 15

Which is expected if one tries to use the deprecated class, two exist and the one chosen is based upon signature:
https://git.drupalcode.org/project/tfa/-/blob/ffccb9eb012695cdef8d0a236e...

Pre 11.4.x core would not attempt to load the incompatible class into memory.

cmlara’s picture

cmlara
he/him
commented

#3584793: Use PHP attributes for form route discovery just changes the lines around class_exists() still calls the auto loader(and apparently its not caught by \Error) 

11	3.9622	23981800	Drupal\Core\Routing\AttributeRouteDiscovery->getReflectionClass( $className = 'Drupal\\tfa\\Controller\\TfaUserControllerDeprecated' )	.../AttributeRouteDiscovery.php:59
12	3.9622	23981800	class_exists( $class = 'Drupal\\tfa\\Controller\\TfaUserControllerDeprecated' )	.../AttributeRouteDiscovery.php:149
13	3.9622	23981912	Composer\Autoload\ClassLoader->loadClass( $class = 'Drupal\\tfa\\Controller\\TfaUserControllerDeprecated' )	.../AttributeRouteDiscovery.php:149
14	3.9622	23982024	{closure:/var/www/html/vendor/composer/ClassLoader.php:575-577}( $file = '/var/www/html/web/modules/custom/tfa/src/Controller/TfaUserControllerDeprecated.php' )	.../ClassLoader.php:427
godotislate’s picture

godotislate
he/him
commented

Discussed with @longwave and @catch on Slack: solution for route discovery for now will likely be to do a regex match for the FQCN of the Route attribute in the class file contents before checking class_exists() and doing reflection. Not sure how extensible this will be for, say, plugins, or hooks, etc., since those have multiple attributes targeted, but we can look into that. (And there could eventually be subclasses of the attributes outside core.)

We have #3586328: [11.x] Handle attribute discovery fatal errors on missing traits for routing and other subsystems looking at centralizing how to handle exceptions and fatal errors in auto discovery, at least for PHP < 8.5. But since it's not just missing traits we have to worry about, we maybe have to widen scope there or create a meta.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Related issues: -#3584793: Use PHP attributes for form route discovery +#3586328: [11.x] Handle attribute discovery fatal errors on missing traits for routing and other subsystems
godotislate’s picture

godotislate
he/him
commented
Related issues: +#3490322: Use nickic/php-parser for attribute-based plugin discovery to avoid errors

Explored using nikic/php-parser to parse attributes way back in #3490322: Use nickic/php-parser for attribute-based plugin discovery to avoid errors. Don't know how much would be relevant now, but linking as related as a possibility to explore using that or some other kind of static parsing.

godotislate’s picture

godotislate
he/him
commented

Another option is to make attribute route discovery opt in per module for now. Either with a property in .info.yml or service parameters like {module}.allow_attribute_routes.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

I think we could likely just use the PHP tokenizer directly instead of nikic/php-parser, but the most performant solution is just str_contains() to look for the attribute or its class name; this won't cover all edge cases but it's probably good enough. I have this locally but right now the GitLab fork creation doesn't seem to be working...

longwave opened merge request !15945

longwave’s picture

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

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

I think the str_contains() is a good option, we should try to centralise all of this somewhere which would then help if we end up moving to use the tokenizer too. Not sure if there's already an issue.

cmlara’s picture

cmlara
he/him
commented

Quick test of MR against TFA's code base appears to confirm issue resolved.

Visual review: I forsee several false negatives on the detection, the most likely to be a comment similar to We do not use the [#Route attribute due to foo bar. Possible other false negatives if another class is named Route and its attribute is utilized.

Tokenzier of some sort sounds a lot more stable long term. I imagine it has higher performance costs however my understanding is almost all of these areas have been cached container so that they are infrequent.

Ideally none of those false negatives cause issues however worst at least acknowledging that the performant method is also the riskier method and core is doing so knowing these limitations exist.

There is a much larger discussion around how much core wants to designed around "everything works even when bad data is present" vs "throw errors early and often" both have a place in design.

Ultimately for me, as long as the changes don't break code that worked in the previous minor I could really care less which path is chosen as Majors are permitted to significantly break releases.

Subnote: I did a quick test of the earlier linked version and \Throwable did (on PHP8.4) catch the class load failure (though looking through the linked issues sounds like that may not be true on 8.3). That goes back to the above question of how much work with bad data present, or error early.

godotislate closed merge request !15945

  • godotislate committed 5f815c3b on main 
    fix: #3592887 AttributeRouteDiscovery does not cleanly handle invalid...
godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 5f815c3 and pushed to main. Thanks!

Merge conflicts cherry-picking to 11.x, so this will need a backport MR.

longwave opened merge request !15958

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: main » 11.x-dev
Status: Patch (to be ported) » Needs review
godotislate’s picture

godotislate
he/him
commented
Status: Needs review » Reviewed & tested by the community

Backport diff looks essentially the same as main.

  • longwave committed 14166896 on 11.4.x 
    fix: #3592887 AttributeRouteDiscovery does not cleanly handle invalid...

  • longwave committed b0d5f3a7 on 11.x 
    fix: #3592887 AttributeRouteDiscovery does not cleanly handle invalid...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 11.x-dev » 11.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed b0d5f3a7aab to 11.x and 14166896510 to 11.4.x. Thanks!

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.

longwave closed merge request !15958

Status: Fixed » Closed (fixed)

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