Extend service provider search

[CyberiaResurrection]

My attempt to implement a suggestion implied in #273 (comment) , namely getting a list of directories to check for service providers, directly from Composer data.

@szepeviktor , I wasn't sure exactly how to test the load-from-extra-search-directories without polluting production use with test code (such as a dummy service provider). In lieu of a way to do that cleanly, I added a test that verified my changes didn't break the status quo on Larastan itself.

[szepeviktor]

Thank you for you contribute.

Could you use Composer's mechanisms instead of getcwd().... ?
Maybe some sophisticated version of https://stackoverflow.com/a/43543482

[CyberiaResurrection]

@szepeviktor , I'll certainly try - I used getcwd() because the existing code did so.

[szepeviktor]

$reflection = new \ReflectionClass(\Composer\Autoload\ClassLoader::class);
$vendorDir = dirname($reflection->getFileName(), 2);

[CyberiaResurrection]

Thanks - let me try that.

[CyberiaResurrection]

@szepeviktor , thanks for your suggestion - it seems to have worked well.

[szepeviktor]

getFileName() gives you vendor/composer/ClassLoader.php so one dirname() is enough

[szepeviktor]

require_once may not serve us well as this file could be already loaded, please consider require, see in vendor/composer/autoload_real.php

[szepeviktor]

if it is possible please try to nuke Finder::create()->files()->name('*.php') and require_once $file;

See #284

[CyberiaResurrection]

@szepeviktor , that should be all of your comments addressed except Finder::create()->files()->name('*.php') and the inherited require_once , both in ApplicationResolver::getProjectClasses.

I'm not sure if they're in this pull req's scope - maybe I should try to clean them up in another pull request after you're otherwise happy with this one?

[c-harris]

@szepeviktor I just waited to add my two cents on the removal of the find and require_once.

I have recently tackled a similar issue and had to build a custom class finder. If you want I could either
A) Publish a package to find classes in directory
Or
B) Advise @CyberiaResurrection on how to reproduce similar functionality

What's your preference?

[szepeviktor]

@c-harris We welcome your advise!

Please note that the logic is dangerous not the implementation!! See #284

[c-harris]

@szepeviktor i think the danger could be heavily mitigated removing the need to include get declared classes and the require (And execution) of the files.

If composer/composer was added as a dep then the directory information gained from the new getProjectSearchDirs method added by @CyberiaResurrection could then be piped into
Composer\Autoload\ClassMapGenerator::createMap

That would yield a set of classes (as key) and the files (as value). They would then invoked by the autoloader in the class_parents call.

Thoughts?

[CyberiaResurrection]

@szepeviktor , I was reluctant to expand the dependency list on my own because I've had pull requests rejected for that reason before.

However, adding composer/composer and proceeding as @c-harris suggests seems to be the easiest way forward to defuse your concerns. I'll welcome a better idea, from anyone.

[szepeviktor]

I would agree any solution that avoids iteration through files.

[szepeviktor]

@nunomaduro Please tell us your opinion.

[CyberiaResurrection]

@szepeviktor , @nunomaduro , I've followed @c-harris' suggestion and used the composer ClassMapGenerator's createMap method. Is this what you were thinking of, @szepeviktor ?

[szepeviktor]

I am just a janitor here!

[CyberiaResurrection]

I am just a janitor here!

Almighty janitor you may be (and an extremely helpful one, to boot), @szepeviktor , is my commit where I introduced ClassMapGenerator::createMap what you had in mind when you were asking me to nuke Finder and require_file?

[szepeviktor]

:)

Yes, it is.

@nunomaduro Please take another look at it.

[CyberiaResurrection]

@nunomaduro , I've just rebased this, for ease of merging, on top of the other pull reqs that have landed since this one did. Is there anything else you want me to clean up, or can this PR be merged?

[canvural]

This looks good 👍

[CyberiaResurrection]

Ok, looks like I've missed something here and I hope you can help me, @canvural - do I need to request specific reviewers for this PR, or can anyone with write access jump in and mark it approved?

[canvural]

@nunomaduro No, you didn't miss anything. @nunomaduro is the only one with the write access. And at the end its his decision for merging. I just gave my opinion.

[nunomaduro]

This may be an important change - I will test it out tonight.

[CyberiaResurrection]

@canvural , thanks for your patience and your explanation.

@nunomaduro , thank you for your patience, too - sorry for being a little eager.

[c-harris]

@nunomaduro as changes go this one has helped me solve a problem where I had to separate my project into two but required the same service provider (Or specifically fascard) on both.

Just wanted to add my two cents here, I'm keen yo see it incorporated.

[nunomaduro]

@canvural Leaving this one to you. We just need to make sure that Larastan works on:

• Laravel packages
• Lumen apps
• Laravel apps

[canvural]

Sure. I can test it. Then let you know. (I guess I don't have rights to merge)

[nunomaduro]

@canvural You are admin - so you should have rights 👍

[canvural]

@nunomaduro This is good to merge! But I can't merge because it says Required status check "continuous-integration/styleci/push" is expected. 🤷‍♂️

[szepeviktor]

• Laravel packages

• Lumen apps

• Laravel apps

A package is tested: Larastan itself
Lumen is tested
The default Laravel app is tested.

[CyberiaResurrection]

@canvural , should I send up a dummy commit to trip the CI infrastructure again?

[canvural]

@CyberiaResurrection I'm not sure how the StyleCI push system works. I've never seen it work.

Or we can wait @nunomaduro to help 😄

[CyberiaResurrection]

@canvural , if the StyleCI push bitz aren't working, are you able to merge this PR anyway?

[canvural]

No. It doesn't let me.