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

Get the autoloader class from current registered autoload functions #163

Merged
merged 1 commit into from
May 2, 2018

Conversation

silvester
Copy link
Contributor

If the default folder for vendor is changed the AutloaderUtil class is no more able to find the ClassLoader. Testing the failure can be easily done by changing the vendor-dir in composer.json. After adjusting the autoload require in console file, all makers that use AutoloadUtil will fail.

Motivation behind this is development with docker. The application is a lot faster on windows and mac if the vendor folder is not a shared folder.

@weaverryan
Copy link
Member

Tests are failing. But, yea, I think this approach makes even more sense!

@silvester
Copy link
Contributor Author

Just fixed the closely related test. I will look into others.

@stof
Copy link
Member

stof commented Apr 25, 2018

I also suggest using an instance variable rather than a static variable for the cache

if ($autoloader[0] instanceof ClassLoader) {
self::$classLoader = $autoloader[0];
break;
} elseif (class_exists('Symfony\Component\Debug\DebugClassLoader')
Copy link
Member

Choose a reason for hiding this comment

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

class_exists is useless. $a instanceof NonExistentClass is perfectly valid (it won't ever return true for non-existent classes) and this will avoid triggering the autoloader when it does not exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know. Changed it.

foreach ($this->getPathForFutureClassTests() as $className => $expectedPath) {
$this->assertSame(
// the paths will start in vendor/composer and be relative
str_replace('\\', '/', self::$currentRootDir.'/vendor/composer/../../'.$expectedPath),
// the paths will start in vendor/composer and be relative
Copy link
Member

Choose a reason for hiding this comment

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

you should revert this indentation change

$this->classLoader = $autoloader[0];
break;
} elseif ($autoloader[0] instanceof DebugClassLoader
&& $autoloader[0]->getClassLoader()[0] instanceof ClassLoader) {
Copy link
Member

Choose a reason for hiding this comment

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

what if the wrapped loader is not an array callable ?

if ($autoloader[0] instanceof ClassLoader) {
$this->classLoader = $autoloader[0];
break;
} elseif ($autoloader[0] instanceof DebugClassLoader
Copy link
Member

Choose a reason for hiding this comment

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

this can be if rather than elseif as there is a break above


$autoloaderUtil = new AutoloaderUtil(self::$currentRootDir);
foreach ($this->getPathForFutureClassTests() as $className => $expectedPath) {
$this->assertSame(
// the paths will start in vendor/composer and be relative
Copy link
Member

Choose a reason for hiding this comment

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

this comment is now wrong

}
}
}
$autoloader->setUseIncludePath(true);
Copy link
Member

Choose a reason for hiding this comment

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

mutating the actual autoloader of PHP is a bad idea, as it will impact the test process (until the end of the testsuite).

a better idea would be to create a Composer\ClassLoader instance with your fake configuration, and put it in the $autoloaderUtil private property (as if the class loader had already been located previously).
And then, you could have a separate test triggering the actual class loader retrieval code, but without altering the instance itself.

@silvester
Copy link
Contributor Author

Any other changes needed?

@weaverryan
Copy link
Member

No other changes needed - this is great work! Thank you Silvester!

@weaverryan weaverryan merged commit bc04e9a into symfony:master May 2, 2018
weaverryan added a commit that referenced this pull request May 2, 2018
…d functions (silvester)

This PR was squashed before being merged into the 1.0-dev branch (closes #163).

Discussion
----------

Get the autoloader class from current registered autoload functions

If the default folder for vendor is changed the AutloaderUtil class is no more able to find the ClassLoader. Testing the failure can be easily done by changing the **vendor-dir** in composer.json. After adjusting the autoload require in console file, all makers that use AutoloadUtil will fail.

Motivation behind this is development with docker. The application is a lot faster on windows and mac if the vendor folder is not a shared folder.

Commits
-------

bc04e9a Get the autoloader class from current registered autoload functions
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.

3 participants