Skip to content

[Routing] Psr4DirectoryLoader issue with abstract classes #48168

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
MatTheCat opened this issue Nov 9, 2022 · 7 comments · Fixed by #48170
Closed

[Routing] Psr4DirectoryLoader issue with abstract classes #48168

MatTheCat opened this issue Nov 9, 2022 · 7 comments · Fixed by #48170
Labels
Bug Routing Status: Needs Review

Comments

@MatTheCat
Copy link
Contributor

MatTheCat commented Nov 9, 2022 •
edited
Loading

Symfony version(s) affected

6.2-beta

Description

The new Psr4DirectoryLoader only supports attribute or annotation type so it will use the AnnotationClassLoader which don’t want to deal with abstract classes:

symfony/src/Symfony/Component/Routing/Loader/AnnotationClassLoader.php

Lines 114 to 116 in b02a689

if ($class->isAbstract()) {
throw new \InvalidArgumentException(sprintf('Annotations from class "%s" cannot be read as it is abstract.', $class->getName()));
}

(See 946ccb6 for rationale.)

If AnnotationDirectoryLoader skips them, Psr4DirectoryLoader does not meaning it will crash on abstract classes.

How to reproduce

Configure your routes to use the PSR-4 route loader, eg.

<?php

use Symfony\Component\Routing\Loader\Configurator\RoutingConfigurator;

return static function (RoutingConfigurator $routes)
{
    $routes->import(['path' => '../src/Controller', 'namespace' => 'App\Controller'], 'annotation');
};

and make one of your controllers extends an abstract one (it does not need to define any method).

Refresh the cache to see the error.

Possible Solution

Psr4DirectoryLoader should skip abstract classes. As it means it will need to create an instance of ReflectionClass it could be useful to make AnnotationClassLoader::load accept these.

Additional Context

Spotted from #48161
@MatTheCat MatTheCat added the Bug label Nov 9, 2022
@MatTheCat MatTheCat mentioned this issue Nov 9, 2022
Closed
@derrabus derrabus mentioned this issue Nov 9, 2022
Merged
@derrabus
Copy link
Member

derrabus commented Nov 9, 2022

This is a very good catch. I indeed missed that case in my test suite. Thank you for working on a fix in #48169! However, I believe there's a simpler way to address the issue, see #48170.

@MatTheCat
Copy link
Contributor Author

As every loader using AnnotationClassLoader will need to instantiate a ReflectionClass that means two are created for each controller; my PR addressed this point and the potentially wrong $type passed in

symfony/src/Symfony/Component/Routing/Loader/Psr4DirectoryLoader.php

Line 90 in b02a689

$collection->addCollection($this->import($className, 'attribute'));

Don’t really mind though, do what you think is best!

@derrabus
Copy link
Member

derrabus commented Nov 9, 2022

As every loader using AnnotationClassLoader will need to instantiate a ReflectionClass that means two are created for each controller;

I believe we can live with that.

my PR addressed this point and the potentially wrong $type passed in

Why do you think a wrong $type is passed and what does it have to do with the issue of not being able to load from directories with abstract classes?

@MatTheCat
Copy link
Contributor Author

Why do you think a wrong $type is passed

Because attribute is hardcoded in

symfony/src/Symfony/Component/Routing/Loader/Psr4DirectoryLoader.php

Line 90 in b02a689

$collection->addCollection($this->import($className, 'attribute'));
whereas $type could be annotation. When working on my PR this led to wrong error messages.

what does it have to do with the issue of not being able to load from directories with abstract classes?

Nothing, just spotted while working on it. It is such a small change I included it in this issue and the associated PR.

@derrabus
Copy link
Member

derrabus commented Nov 9, 2022

Because attribute is hardcoded in […] whereas $type could be annotation.

Functionality-wise this does not make much of a difference. Both types are equivalent.

When working on my PR this led to wrong error messages.

Okay, for DX it's probably a good idea to pass the actual type then. Can you open a new PR just for that change? And please include a test that reproduces the issue with the wrong error message.

@MatTheCat
Copy link
Contributor Author

Hm I cannot produce a failing test case so it must be linked to the changes I was making.
Sorry for the mixed issues 😅

@derrabus
Copy link
Member

derrabus commented Nov 9, 2022

All right. Thanks for having a look and thank you very much for this bug report!

@MatTheCat MatTheCat changed the title [Routing] Psr4DirectoryLoader issue with abstract classes and annotations [Routing] Psr4DirectoryLoader issue with abstract classes Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Routing Status: Needs Review
Projects
None yet
Milestone
No milestone
3 participants
@derrabus @MatTheCat @carsonbot