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

Explicitly check if a file has already been required before requiring it #4186

Merged
merged 4 commits into from Nov 21, 2015

Conversation

Projects
None yet
4 participants
@jeskew
Contributor

jeskew commented Jun 27, 2015

This would address #3003 by assigning an identifier to each included file during AutoloadGenerator::dump. Currently, this identifier is the value returned by md5_file (to address the concern raised in guzzle/guzzle#676 (comment)), but it could also be a logical file address such as $packageName::$relativePath, e.g. guzzlehttp/guzzle::src/functions.php.

This is preferable to simple changing require to require_once because it prevents two copies of the same file from being loaded, as would happen if a globally installed package loads project's autoloader and both rely on the same package. A known example of this is using a globally installed copy of CodeCeption on a project requiring Guzzle.

Would also address:
guzzle/guzzle#1146
guzzle/psr7#22

@alcohol

This comment has been minimized.

Show comment
Hide comment
@alcohol

alcohol Jun 28, 2015

Member

Wouldn't it be easier instead if perhaps the Composer autoloader defined a constant (e.g. COMPOSER_AUTOLOADER_LOADED), and people could use that to check if the autoloader has been loaded already or not?

Basically approaching the situation from a different angle. Just trying to add more ideas. :-)

Member

alcohol commented Jun 28, 2015

Wouldn't it be easier instead if perhaps the Composer autoloader defined a constant (e.g. COMPOSER_AUTOLOADER_LOADED), and people could use that to check if the autoloader has been loaded already or not?

Basically approaching the situation from a different angle. Just trying to add more ideas. :-)

@jeskew

This comment has been minimized.

Show comment
Hide comment
@jeskew

jeskew Jun 28, 2015

Contributor

@alcohol The two autoloaders might load different files (e.g., if you install PHPUnit globally and Guzzle locally), so I'm not sure a simple flag would be useful.

Contributor

jeskew commented Jun 28, 2015

@alcohol The two autoloaders might load different files (e.g., if you install PHPUnit globally and Guzzle locally), so I'm not sure a simple flag would be useful.

@alcohol

This comment has been minimized.

Show comment
Hide comment
@alcohol

alcohol Jun 28, 2015

Member

What if the value of it was the resolved path to the autoloader? For
comparison values.
On Jun 28, 2015 10:45 PM, "Jonathan Eskew" notifications@github.com wrote:

@alcohol https://github.com/alcohol The two autoloaders might load
different files (e.g., if you install PHPUnit globally and Guzzle locally),
so I'm not sure a simple flag would be useful.


Reply to this email directly or view it on GitHub
#4186 (comment).

Member

alcohol commented Jun 28, 2015

What if the value of it was the resolved path to the autoloader? For
comparison values.
On Jun 28, 2015 10:45 PM, "Jonathan Eskew" notifications@github.com wrote:

@alcohol https://github.com/alcohol The two autoloaders might load
different files (e.g., if you install PHPUnit globally and Guzzle locally),
so I'm not sure a simple flag would be useful.


Reply to this email directly or view it on GitHub
#4186 (comment).

@jeskew

This comment has been minimized.

Show comment
Hide comment
@jeskew

jeskew Jun 28, 2015

Contributor

I don't believe that would solve the issue reported in #3003, which was that the same file, existing in two locations on disk, could be included by two separate autoloaders.

Contributor

jeskew commented Jun 28, 2015

I don't believe that would solve the issue reported in #3003, which was that the same file, existing in two locations on disk, could be included by two separate autoloaders.

@alcohol

This comment has been minimized.

Show comment
Hide comment
@alcohol

alcohol Jun 29, 2015

Member

as would happen if a package is installed globally and in a project

I don't understand how this is relevant? Globally installed doesn't mean it is always loaded. You would still have to explicitly include ~/.composer/vendor/autoloader.php first if I am not mistaken?

Member

alcohol commented Jun 29, 2015

as would happen if a package is installed globally and in a project

I don't understand how this is relevant? Globally installed doesn't mean it is always loaded. You would still have to explicitly include ~/.composer/vendor/autoloader.php first if I am not mistaken?

@jeskew

This comment has been minimized.

Show comment
Hide comment
@jeskew

jeskew Jun 29, 2015

Contributor

That's correct, and that is the issue that was reported. #3003 (comment)

Contributor

jeskew commented Jun 29, 2015

That's correct, and that is the issue that was reported. #3003 (comment)

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Jun 29, 2015

Contributor

@alcohol the problem occurs when a tool installed globally (PHPUnit for instance) also loads the autoloading of your project after loading the global one

Contributor

stof commented Jun 29, 2015

@alcohol the problem occurs when a tool installed globally (PHPUnit for instance) also loads the autoloading of your project after loading the global one

@jeskew

This comment has been minimized.

Show comment
Hide comment
@jeskew

jeskew Jul 1, 2015

Contributor

@stof Do you think this is a good way to solve the problem? I used file checksums instead of logical identifiers so that accidentally loading two versions of the same package would continue to cause a failure, but using something like $packageName::$pathRelativeToVendorDir => $absolutePath would also be viable.

Contributor

jeskew commented Jul 1, 2015

@stof Do you think this is a good way to solve the problem? I used file checksums instead of logical identifiers so that accidentally loading two versions of the same package would continue to cause a failure, but using something like $packageName::$pathRelativeToVendorDir => $absolutePath would also be viable.

@Seldaek Seldaek merged commit 42b0257 into composer:master Nov 21, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Seldaek added a commit that referenced this pull request Nov 21, 2015

Simplify files autoload include function, and make sure files are inc…
…luded once per package even if exactly the same, refs #4186
@Seldaek

This comment has been minimized.

Show comment
Hide comment
@Seldaek

Seldaek Nov 21, 2015

Member

Thanks!

Member

Seldaek commented Nov 21, 2015

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment