More robust autoloader detection #379

GwendolenLynch · 2019-04-16T08:02:18Z

When the project, or dependent package, adds a custom autoloader, e.g. PhpStan, the added autoloader will be returned instead of the one containing the project's autoloader when ComposerAutoloaderFinder iterates the autoloader list leading to the mysterious error:

Could not determine where to locate the new class "App\Controller\DeliciousPuppyController", maybe try with a full namespace like "\My\Full\Namespace\DeliciousPuppyController"

Fixes #196
Fixes #231
Fixes #313

Related #378

weaverryan

I think this is close. I'd be very happy to eliminate this confusing error!

src/Util/ComposerAutoloaderFinder.php

weaverryan · 2019-04-19T17:49:21Z

src/Util/ComposerAutoloaderFinder.php

@@ -46,13 +56,15 @@ private function findComposerClassLoader()

        foreach ($autoloadFunctions as $autoloader) {
            if (\is_array($autoloader) && isset($autoloader[0]) && \is_object($autoloader[0])) {
-                if ($autoloader[0] instanceof ClassLoader) {
+                if ($autoloader[0] instanceof ClassLoader


Maybe we should improve the Composer ClassLoader not found message above?

Could not find a Composer autoloader that autoloads from "{$this->rootNamespace}"

What about PSR-0?

And also, there is still an edge case: I have App\\ configured as my default, but I actually always generate things using a full class name for some reason - e.g. MyCompany\Entity\FooEntity. And, I don't have App configured in my autoloader. In that case, I would get the error EVEN if MyCompany is mapped in a valid way.

Perhaps, to at least help reduce this, we should only apply this extra check that you've added IF we find multiple class loaders? If we only find 1, it's safe to use that, even if it doesn't make the root namespace.

…nt's autoloader

GwendolenLynch · 2019-04-20T10:19:19Z

OK, so while I am not happy with my choice of method naming nor elegance — ideas/suggestions very welcome! — I think I've addressed your concerns.

I started with a baseline of a fresh composer create-project symfony/website-skeleton, which already has two autoloaders. So I figured the best approach now is to "remember" the autoloader that contains the component, and should the match still not be found, using that as the fallback.

Also, I've slightly expanded the tests to cover PSR-0, PSR-4 & fallback.

weaverryan

Sorry for the delay :). I left a few minor comments... but I realized they all summarize down to two points:

What's the edge case that we're trying to solve by falling back to the autoloader that contains this bundle's namespace? I'd prefer to not do this, but I might be missing some legitimate use-case.
If we don't need that, we can return from all the methods, instead of setting the property directly.

Cheers!

src/Util/ComposerAutoloaderFinder.php

GwendolenLynch · 2019-06-02T08:41:39Z

What's the edge case that we're trying to solve by falling back to the autoloader that contains this bundle's namespace? I'd prefer to not do this, but I might be missing some legitimate use-case.

Specifically for the fallback option, and assuming symfony/maker-bundle is installed via Composer:

It is a namespace we know will be in the Composer autoloader
The end-project root namespace is a config setting, so could be wrong, or missing
The end-project composer.json is not guaranteed to have its own psr-4 or psr-0 defined and loading, or not 'dumped' by Composer yet

If we don't need that, we can return from all the methods, instead of setting the property directly.

Done 😀

weaverryan · 2019-06-14T13:29:19Z

This is very nice work @GawainLynch! Thanks for the final changes - the flow was easy for me to understand :). Cheers!

This PR was squashed before being merged into the 1.0-dev branch (closes #379). Discussion ---------- More robust autoloader detection When the project, or dependent package, adds a custom autoloader, e.g. PhpStan, the added autoloader will be returned instead of the one containing the project's autoloader when `ComposerAutoloaderFinder` iterates the autoloader list leading to the mysterious error: ``` Could not determine where to locate the new class "App\Controller\DeliciousPuppyController", maybe try with a full namespace like "\My\Full\Namespace\DeliciousPuppyController" ``` Fixes #196 Fixes #231 Fixes #313 Related #378 Commits ------- 719979e Return objects from methods & switch fallback logic 22f5a3b Search for matching PSR-0 or PSR-4 namespaces, and default to component's autoloader 07830f9 Only return the autoloader when it contains the root namespace

GwendolenLynch · 2019-06-14T15:09:15Z

A pleasure mate, thank you too.

Kocal · 2019-06-16T14:08:16Z

Thanks for this patch! I had the phpstan-shim issue and now it's working properly! :)

symfony/maker-bundle#379

Only return the autoloader when it contains the root namespace

07830f9

GwendolenLynch force-pushed the autoloader-finder branch from 28f1e18 to 4aff8ba Compare April 16, 2019 09:49

weaverryan reviewed Apr 19, 2019

View reviewed changes

Search for matching PSR-0 or PSR-4 namespaces, and default to compone…

22f5a3b

…nt's autoloader

GwendolenLynch force-pushed the autoloader-finder branch 2 times, most recently from 8027f0c to 23d731c Compare April 20, 2019 12:35

GwendolenLynch changed the title ~~Only return the autoloader when it contains the root namespace~~ More robust autoloader detection Apr 20, 2019

weaverryan reviewed Jun 1, 2019

View reviewed changes

src/Util/ComposerAutoloaderFinder.php Outdated Show resolved Hide resolved

src/Util/ComposerAutoloaderFinder.php Show resolved Hide resolved

src/Util/ComposerAutoloaderFinder.php Outdated Show resolved Hide resolved

src/Util/ComposerAutoloaderFinder.php Show resolved Hide resolved

Return objects from methods & switch fallback logic

719979e

GwendolenLynch force-pushed the autoloader-finder branch 2 times, most recently from c015539 to 719979e Compare June 2, 2019 09:27

weaverryan merged commit 719979e into symfony:master Jun 14, 2019

GwendolenLynch deleted the autoloader-finder branch June 14, 2019 15:08

Kocal added a commit to Kocal/symfony-app-template that referenced this pull request Jun 24, 2019

Update maker-bundle to dev-master

eb5a7ba

symfony/maker-bundle#379

Kocal added a commit to Kocal/symfony-app-template that referenced this pull request Jun 24, 2019

Update maker-bundle to dev-master

d2deaf9

symfony/maker-bundle#379

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More robust autoloader detection #379

More robust autoloader detection #379

GwendolenLynch commented Apr 16, 2019

weaverryan left a comment

weaverryan Apr 19, 2019

GwendolenLynch commented Apr 20, 2019 •

edited

Loading

weaverryan left a comment

GwendolenLynch commented Jun 2, 2019 •

edited

Loading

weaverryan commented Jun 14, 2019

GwendolenLynch commented Jun 14, 2019

Kocal commented Jun 16, 2019 •

edited

Loading

More robust autoloader detection #379

More robust autoloader detection #379

Conversation

GwendolenLynch commented Apr 16, 2019

weaverryan left a comment

Choose a reason for hiding this comment

weaverryan Apr 19, 2019

Choose a reason for hiding this comment

GwendolenLynch commented Apr 20, 2019 • edited Loading

weaverryan left a comment

Choose a reason for hiding this comment

GwendolenLynch commented Jun 2, 2019 • edited Loading

weaverryan commented Jun 14, 2019

GwendolenLynch commented Jun 14, 2019

Kocal commented Jun 16, 2019 • edited Loading

GwendolenLynch commented Apr 20, 2019 •

edited

Loading

GwendolenLynch commented Jun 2, 2019 •

edited

Loading

Kocal commented Jun 16, 2019 •

edited

Loading