Skip to content

Commit

Permalink
Merge remote-tracking branch 'jeskew/feature/require_files_but_once'
Browse files Browse the repository at this point in the history
  • Loading branch information
@Seldaek
Seldaek committed Nov 21, 2015
2 parents c0b49d0 + 42b0257 commit 9710b26
Show file tree
Hide file tree
Showing 10 changed files with 84 additions and 34 deletions.
32 changes: 25 additions & 7 deletions src/Composer/Autoload/AutoloadGenerator.php
View file
Expand Up @@ -475,8 +475,9 @@ protected function getIncludePathsFile(array $packageMap, Filesystem $filesystem
protected function getIncludeFilesFile(array $files, Filesystem $filesystem, $basePath, $vendorPath, $vendorPathCode, $appBaseDirCode)
{
$filesCode = '';
foreach ($files as $functionFile) {
$filesCode .= ' '.$this->getPathCode($filesystem, $basePath, $vendorPath, $functionFile).",\n";
foreach ($files as $fileIdentifier => $functionFile) {
$filesCode .= ' ' . $fileIdentifier . ' => '
. $this->getPathCode($filesystem, $basePath, $vendorPath, $functionFile) . ",\n";
}

if (!$filesCode) {
Expand Down Expand Up @@ -649,8 +650,8 @@ public static function getLoader()
if ($useIncludeFiles) {
$file .= <<<INCLUDE_FILES
\$includeFiles = require __DIR__ . '/autoload_files.php';
foreach (\$includeFiles as \$file) {
composerRequire$suffix(\$file);
foreach (\$includeFiles as \$fileIdentifier => \$file) {
composerRequire$suffix(\$fileIdentifier, \$file);
}
Expand All @@ -668,9 +669,17 @@ public static function getLoader()
return $file . <<<FOOTER
}
function composerRequire$suffix(\$file)
function composerRequire$suffix(\$fileIdentifier, \$file)
{
require \$file;
if (empty(\$GLOBALS['composerRequiredFiles'])) {
\$GLOBALS['composerRequiredFiles'] = array();
}
if (empty(\$GLOBALS['composerRequiredFiles'][\$fileIdentifier])) {
require \$file;
\$GLOBALS['composerRequiredFiles'][\$fileIdentifier] = true;
}
}
FOOTER;
Expand Down Expand Up @@ -742,7 +751,11 @@ function ($matches) use (&$updir) {

$relativePath = empty($installPath) ? (empty($path) ? '.' : $path) : $installPath.'/'.$path;

if ($type === 'files' || $type === 'classmap') {
if ($type === 'files') {
$autoloads[var_export($this->getFileIdentifier($relativePath), true)]

This comment has been minimized.

Sign in to view

Copy link
@stof

stof Nov 26, 2015

Contributor

IMO, the var_export should be done when writing the code, not here

This comment has been minimized.

Sign in to view

Copy link
@Seldaek

Seldaek Nov 26, 2015

Author Member

I have fixed that already..
= $relativePath;
continue;
} elseif ($type === 'classmap') {
$autoloads[] = $relativePath;
continue;
}
Expand All @@ -755,6 +768,11 @@ function ($matches) use (&$updir) {
return $autoloads;
}

protected function getFileIdentifier($path)
{
return md5_file($path);
}

/**
* Sorts packages by dependency weight
*
Expand Down
6 changes: 3 additions & 3 deletions tests/Composer/Test/Autoload/AutoloadGeneratorTest.php
View file
Expand Up @@ -1069,8 +1069,8 @@ public function testVendorDirExcludedFromWorkingDir()
$this->assertEquals($expectedNamespace, file_get_contents($vendorDir.'/composer/autoload_namespaces.php'));
$this->assertEquals($expectedPsr4, file_get_contents($vendorDir.'/composer/autoload_psr4.php'));
$this->assertEquals($expectedClassmap, file_get_contents($vendorDir.'/composer/autoload_classmap.php'));
$this->assertContains("\n \$vendorDir . '/b/b/bootstrap.php',\n", file_get_contents($vendorDir.'/composer/autoload_files.php'));
$this->assertContains("\n \$baseDir . '/test.php',\n", file_get_contents($vendorDir.'/composer/autoload_files.php'));
$this->assertContains("\$vendorDir . '/b/b/bootstrap.php',\n", file_get_contents($vendorDir.'/composer/autoload_files.php'));
$this->assertContains("\$baseDir . '/test.php',\n", file_get_contents($vendorDir.'/composer/autoload_files.php'));
}

public function testUpLevelRelativePaths()
Expand Down Expand Up @@ -1147,7 +1147,7 @@ public function testUpLevelRelativePaths()
$this->assertEquals($expectedNamespace, file_get_contents($this->vendorDir.'/composer/autoload_namespaces.php'));
$this->assertEquals($expectedPsr4, file_get_contents($this->vendorDir.'/composer/autoload_psr4.php'));
$this->assertEquals($expectedClassmap, file_get_contents($this->vendorDir.'/composer/autoload_classmap.php'));
$this->assertContains("\n \$baseDir . '/../test.php',\n", file_get_contents($this->vendorDir.'/composer/autoload_files.php'));
$this->assertContains("\$baseDir . '/../test.php',\n", file_get_contents($this->vendorDir.'/composer/autoload_files.php'));
}

public function testEmptyPaths()
Expand Down
4 changes: 2 additions & 2 deletions tests/Composer/Test/Autoload/Fixtures/autoload_files.php
View file
Expand Up @@ -6,6 +6,6 @@
$baseDir = dirname($vendorDir);

return array(
$baseDir . '/foo.php',
$baseDir . '/bar.php',
'25b429360a61ca2629fdf9a4f484981c' => $baseDir . '/foo.php',
'524f65941cc9a0fa65ff0ec097ccde8a' => $baseDir . '/bar.php',
);
2 changes: 1 addition & 1 deletion tests/Composer/Test/Autoload/Fixtures/autoload_files2.php
View file
Expand Up @@ -6,5 +6,5 @@
$baseDir = dirname($vendorDir);

return array(
$baseDir . '/devfiles/foo.php',
'fc1e99cd93a6aa58e3b1565e688bcd87' => $baseDir . '/devfiles/foo.php',
);
10 changes: 5 additions & 5 deletions tests/Composer/Test/Autoload/Fixtures/autoload_files_functions.php
View file
Expand Up @@ -6,9 +6,9 @@
$baseDir = dirname($vendorDir);

return array(
$vendorDir . '/a/a/test.php',
$vendorDir . '/b/b/test2.php',
$vendorDir . '/c/c/foo/bar/test3.php',
$vendorDir . '/c/c/foo/bar/test4.php',
$baseDir . '/root.php',
'b18a3c4a24b1dc69fdd96098e5c28e01' => $vendorDir . '/a/a/test.php',
'93797e35718fdafb825b1fe25c08a92c' => $vendorDir . '/b/b/test2.php',
'a57008eef928ced7c299bfea724d8441' => $vendorDir . '/c/c/foo/bar/test3.php',
'32e13a06a803ac6553a93454bcd3d2da' => $vendorDir . '/c/c/foo/bar/test4.php',
'504bb142db8acef729eeeb06b0aedec5' => $baseDir . '/root.php',
);
4 changes: 2 additions & 2 deletions tests/Composer/Test/Autoload/Fixtures/autoload_files_target_dir.php
View file
Expand Up @@ -6,6 +6,6 @@
$baseDir = dirname($vendorDir);

return array(
$baseDir . '/foo.php',
$baseDir . '/bar.php',
'25b429360a61ca2629fdf9a4f484981c' => $baseDir . '/foo.php',
'524f65941cc9a0fa65ff0ec097ccde8a' => $baseDir . '/bar.php',
);
16 changes: 12 additions & 4 deletions tests/Composer/Test/Autoload/Fixtures/autoload_real_files_by_dependency.php
View file
Expand Up @@ -41,15 +41,23 @@ public static function getLoader()
$loader->register(true);

$includeFiles = require __DIR__ . '/autoload_files.php';
foreach ($includeFiles as $file) {
composerRequireFilesAutoloadOrder($file);
foreach ($includeFiles as $fileIdentifier => $file) {
composerRequireFilesAutoloadOrder($fileIdentifier, $file);
}

return $loader;
}
}

function composerRequireFilesAutoloadOrder($file)
function composerRequireFilesAutoloadOrder($fileIdentifier, $file)
{
require $file;
if (empty($GLOBALS['composerRequiredFiles'])) {
$GLOBALS['composerRequiredFiles'] = array();
}

if (empty($GLOBALS['composerRequiredFiles'][$fileIdentifier])) {
require $file;

$GLOBALS['composerRequiredFiles'][$fileIdentifier] = true;
}
}
16 changes: 12 additions & 4 deletions tests/Composer/Test/Autoload/Fixtures/autoload_real_functions.php
View file
Expand Up @@ -41,15 +41,23 @@ public static function getLoader()
$loader->register(true);

$includeFiles = require __DIR__ . '/autoload_files.php';
foreach ($includeFiles as $file) {
composerRequireFilesAutoload($file);
foreach ($includeFiles as $fileIdentifier => $file) {
composerRequireFilesAutoload($fileIdentifier, $file);
}

return $loader;
}
}

function composerRequireFilesAutoload($file)
function composerRequireFilesAutoload($fileIdentifier, $file)
{
require $file;
if (empty($GLOBALS['composerRequiredFiles'])) {
$GLOBALS['composerRequiredFiles'] = array();
}

if (empty($GLOBALS['composerRequiredFiles'][$fileIdentifier])) {
require $file;

$GLOBALS['composerRequiredFiles'][$fileIdentifier] = true;
}
}
12 changes: 10 additions & 2 deletions tests/Composer/Test/Autoload/Fixtures/autoload_real_include_path.php
View file
Expand Up @@ -65,7 +65,15 @@ public static function autoload($class)
}
}

function composerRequireIncludePath($file)
function composerRequireIncludePath($fileIdentifier, $file)
{
require $file;
if (empty($GLOBALS['composerRequiredFiles'])) {
$GLOBALS['composerRequiredFiles'] = array();
}

if (empty($GLOBALS['composerRequiredFiles'][$fileIdentifier])) {
require $file;

$GLOBALS['composerRequiredFiles'][$fileIdentifier] = true;
}
}
16 changes: 12 additions & 4 deletions tests/Composer/Test/Autoload/Fixtures/autoload_real_target_dir.php
View file
Expand Up @@ -43,8 +43,8 @@ public static function getLoader()
$loader->register(true);

$includeFiles = require __DIR__ . '/autoload_files.php';
foreach ($includeFiles as $file) {
composerRequireTargetDir($file);
foreach ($includeFiles as $fileIdentifier => $file) {
composerRequireTargetDir($fileIdentifier, $file);
}

return $loader;
Expand All @@ -69,7 +69,15 @@ public static function autoload($class)
}
}

function composerRequireTargetDir($file)
function composerRequireTargetDir($fileIdentifier, $file)
{
require $file;
if (empty($GLOBALS['composerRequiredFiles'])) {
$GLOBALS['composerRequiredFiles'] = array();
}

if (empty($GLOBALS['composerRequiredFiles'][$fileIdentifier])) {
require $file;

$GLOBALS['composerRequiredFiles'][$fileIdentifier] = true;
}
}

5 comments on commit 9710b26

@davidalger
Copy link

Choose a reason for hiding this comment

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

@jeskew / @Seldaek This seems like a really roundabout way to require a file only once, especially since PHP has a require_once construct which can be used to do the exact same thing this appears to be implementing. Is there a reason it was done this way instead of simply changing require to require_once?

This code change breaks the magento-composer-installer, resulting in the issue outlined on magento/magento2-community-edition#15.

@xabbuh
Copy link
Contributor

@xabbuh xabbuh commented on 9710b26 Nov 25, 2015

Choose a reason for hiding this comment

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

@davidalger You may want to take a look at the discussion in #4389.

@jeskew
Copy link
Contributor

@jeskew jeskew commented on 9710b26 Nov 25, 2015

Choose a reason for hiding this comment

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

@davidalger This solves a different problem than require_once. Packages installed globally are sometimes configured to load a project-local autoloader, which can cause conflicts among the autoload_files (as described here).

@Seldaek
Copy link
Member Author

Choose a reason for hiding this comment

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

@davidalger the problem it solves is not that of require_once, it's solving having multiple times the same file added in a global dependency and a local one.

It seems the problem in magento is that the file does not yet exist when the autoloader is dumped. I changed the algo in 8072448 to use only the package name + path for the hash. That should solve it.

@davidalger
Copy link

Choose a reason for hiding this comment

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

@Seldaek Thanks for the added context! It makes a lot more sense now…

Yes, in this case, these files are copied into place via the magento/magento-composer-installer plugin which I guess does not run until after the auto-loader is dumped, as you noted. I've updated my composer and re-run the test which reproduced the issue, and your fix resolved it. Thanks for the quick response! :) Think I like the new solution better too because it won't have to load a bunch of files from disk, meaning quicker autoloader generation.

Please sign in to comment.