Skip to content
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.

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

[WIP] first draft to guess dependencies from composer's autoloader #91

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

maglnet
Copy link
Owner

@maglnet maglnet commented Oct 11, 2018

This adresses #52

ClassLoader::findFile($class) does not even include the file, it just checks if the Loader finds a file within the configured autoloader config.

@maglnet maglnet force-pushed the feature/guess-dependencies-from-composer branch from 8f6addc to f4e79b1 Compare October 11, 2018 22:18
@maglnet maglnet force-pushed the feature/guess-dependencies-from-composer branch from f4e79b1 to 326308f Compare October 11, 2018 22:19
@maglnet maglnet force-pushed the feature/guess-dependencies-from-composer branch from 8386810 to 266391f Compare October 11, 2018 22:40
{
$this->guessers[] = new GuessFromLoadedExtensions();
foreach ($guessers as $guesser) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$this->guessers = array_merge([new GuessFromLoadedExtensions(), $guessers])

*/
private $composerAutoloader;

private $configVendorDir;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type declaration


public function __construct(string $composerJsonPath)
{
$composerJson = json_decode(file_get_contents($composerJsonPath), true);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably better to require the composer DSL to already be in array shape, instead of adding I/O and nullability in here

{
$composerJson = json_decode(file_get_contents($composerJsonPath), true);
$this->configVendorDir = $this->normalizePath(dirname($composerJsonPath) . '/' . ($composerJson['config']['vendor-dir'] ?? 'vendor'));
$this->composerAutoloader = include $this->configVendorDir . '/autoload.php';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very risky, and it can lead to pre-loading analysed code. So far, the library limited itself to using the AST.

If you need to add composer-based file location, maybe check Roave/BackwardCompatibilityCheck#102, where I implemented an abstraction for PSR-4, PSR-0, classmap and files loading via BetterReflection. Still extremely slow, but since we need such an abstraction in BetterReflection itself, putting efforts there would be better.

Copy link
Collaborator

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requires rebase rather than merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants