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

[Scoper] Scoping autoloaded files result in an autoload conflict #298

Closed
Smitsel opened this issue Jan 16, 2019 · 62 comments
Closed

[Scoper] Scoping autoloaded files result in an autoload conflict #298

Smitsel opened this issue Jan 16, 2019 · 62 comments

Comments

@Smitsel
Copy link

Smitsel commented Jan 16, 2019

Edit to avoid to summarise the state of this issue.

When scoping a package which has autoloaded files, the scoped files will keep the same hash. As a result if your current application/library is using the scoped package and has files with the same hash, only one file will be loaded which results in undesirable behaviours.

It appears the hash strategy is not something we can change composer/composer#7942 and changing the package name of the scoped packages has undesirable side effects so it is not a solution either.

@jarrodbell #298 (comment) and @ondrejmirtes #298 (comment) made it work by changing it after the composer dump process as part of the scoping build process.

The discussion is carrying from that point to figure out if there is a way PHP-Scoper could fix it by itself without further actions from the user.


Bug report

Question Answer
Box version PHP Scoper version 0.11.4 2018-11-13 08:49:16 UTC
PHP version PHP 7.1.22
Platform with version Ubuntu 16.04.5 LTS
Github Repo https://github.com/mollie/mollie-api-php

We are using php-scoper in our client to generate a release for integrators to be used in for example Wordpress. When it is used there, there are often more packages with Guzzle via composer.

When this happens they will get the exception:
Uncaught Error: Call to undefined function GuzzleHttp\choose_handler()

This is because the corresponding guzzlehttp/guzzle/src/functions_include.php is not loaded for the second implementation due to the composer file require hash being the same.

Below there is an example of our package being ran alongside another guzzle package.
This is a var_dump inside the composer_real.php where the functions_include is being required if the file hash is not already loaded.

array(3) {
  ["c964ee0ededf28c96ebd9db5099ef910"]=>
  string(99) "{path_to_site}/vendor/composer/../guzzlehttp/promises/src/functions_include.php"
  ["a0edc8309cc5e1d60e3047b5df6b7052"]=>
  string(95) "{path_to_site}/vendor/composer/../guzzlehttp/psr7/src/functions_include.php"
  ["37a3dc5111fe8f707ab4c132ef1dbc62"]=>
  string(97) "{path_to_site}/vendor/composer/../guzzlehttp/guzzle/src/functions_include.php"
}
array(3) {
  ["c964ee0ededf28c96ebd9db5099ef910"]=>
  string(106) "{path_to_site}/guzzle/vendor/composer/../guzzlehttp/promises/src/functions_include.php"
  ["a0edc8309cc5e1d60e3047b5df6b7052"]=>
  string(102) "{path_to_site}/guzzle/vendor/composer/../guzzlehttp/psr7/src/functions_include.php"
  ["37a3dc5111fe8f707ab4c132ef1dbc62"]=>
  string(104) "{path_to_site}k/guzzle/vendor/composer/../guzzlehttp/guzzle/src/functions_include.php"
}

So this causes only the first implementation it's choose_handler function to be available.

I'm not really sure if this bug should be reported here, but i'm curious to your expert opinion about this. Might it be worth it to also scope the files that outta be required by composer? To avoid some of them not being loaded? As manipulating the hash in above example would fix the problem.

@theofidry
Copy link
Member

theofidry commented Jan 16, 2019

Hi! Thanks for the report. It's nice to see it being tried on Wordpress :)

I admit I'm a bit confused.

Uncaught Error: Call to undefined function GuzzleHttp\choose_handler()

Since it is not a function from the global namespace, unless explicitly configured otherwise, I would expect this function to be scoped. If it is the case, then it means somewhere a piece of code has not been scoped properly and is still using the old reference (which can be an expected case. Unfortunately due to PHP nature PHP-Scoper won't be able to be perfect and will sometimes need some help via the patchers).

This is because the corresponding guzzlehttp/guzzle/src/functions_include.php is not loaded for the second implementation due to the composer file require hash being the same.

What I would find weird/confusing there is why would it be a problem for the scoped version and not the unscoped one? Also if the hash file is the same it means the content is the same, so indeed there shouldn't be a need to include it twice should it?

@Smitsel
Copy link
Author

Smitsel commented Jan 16, 2019

Yes, it's not a function from the global namespace, but since it is defined in the functions_include.php it will not be available for the second guzzle package.

The hash is the same even though the contents are not.

Scoped functions_include.php:

<?php

namespace _PhpScoper5c3efd0df2edb;

// Don't redefine the functions if included multiple times.
if (!\function_exists('_PhpScoper5c3efd0df2edb\\GuzzleHttp\\uri_template')) {
    require __DIR__ . '/functions.php';
}

Unscoped functions_include.php:

<?php

// Don't redefine the functions if included multiple times.
if (!function_exists('GuzzleHttp\uri_template')) {
    require __DIR__ . '/functions.php';
}

So whatever functions_include.php gets loaded first is the one that will work.
As the second one won't be included.

@theofidry
Copy link
Member

The hash is the same even though the contents are not.

Hm this is very weird then. Has the autoloader been dumped again after the scoping?

@Smitsel
Copy link
Author

Smitsel commented Jan 16, 2019

Yes, i've done a lot of testing with it.

I've tried adding even more code to the functions_include.

Does not change a thing, unfortunately.

@theofidry
Copy link
Member

theofidry commented Jan 16, 2019

I think it would be best to try to narrow down the issue in a reproducible repo to ease the debugging. It may be a bug in Composer or it means Composer is relying on the hash from the installed.json files which would be troublesome and means we would need to do something about it for PHP-Scoper to work properly in this scenario

@Smitsel
Copy link
Author

Smitsel commented Jan 16, 2019

I'll create a repo for you 👍

@Smitsel
Copy link
Author

Smitsel commented Jan 16, 2019

The repository:
https://github.com/Smitsel/guzzle-scoper-reproduction

If you run into any problems or need any more information just let me know :)

@theofidry
Copy link
Member

👍 I'll try to take a look ASAP but it's likely gonna take a couple of weeks, this month is quite busy for me :/

@Smitsel
Copy link
Author

Smitsel commented Jan 16, 2019

Not a problem. We can hack in a solution if it becomes really pressing. Happy that you are willing to take a look!

@willemstuursma
Copy link

@Smitsel
Copy link
Author

Smitsel commented Jan 16, 2019

Yep, manipulating the package name fixes it indeed.

Very weird choice to do it just based on that. They should also compare content-size or something.

@sandervanhooft
Copy link

sandervanhooft commented Jan 31, 2019

I'd hash the whole package directory file (incl contents), but that may be a bit resource expensive 😄

@theofidry
Copy link
Member

it's for the dumping the autoloader so I don't think it would be a big deal

@sandervanhooft
Copy link

Would using getDistSha1Checksum() be a solution? It's on the PackageInterface.

@theofidry
Copy link
Member

I'm not sure. But I think it's a Composer issue so might be better to create an issue and look for a solution there.

If not achievable, maybe we can look for a workaround in PHP-Scoper but I would like to avoid that if possible.

@willemstuursma
Copy link

@sandervanhooft no its needs to hash the file contents so it can deal with out situation.

@theofidry Could you open an issue at Composer? I think it will carry more weight if you do it as the owner / maintainer of the humbug.

@theofidry
Copy link
Member

theofidry commented Jan 31, 2019

So as per the discussion in composer/composer#7942, I changed my mind and I think it makes sense for PHP-Scoper to change the names of the scoped packages.

So I guess a solution would be to also change the packages names in composer.json and installed.json to solve the issue, maybe by just appending the prefix to the name.

The concerned code is this one I think: https://github.com/humbug/php-scoper/tree/ddcf428/src/Scoper/Composer

@theofidry theofidry added the bug label Jan 31, 2019
@Smitsel
Copy link
Author

Smitsel commented Feb 1, 2019

I agree with that approach, makes more sense since this will only happen when file contents of required files get scoped or altered. It's not a composer bug. Sounds like a good solution!

@patrickjahns
Copy link

I've experimented with this a bit and couldn't find a simple way to solve this within scoper.
Once the package-name is altered in installed.json - a autoload dump ( which is recommended after scoping right? ) won't work - as the package name does no longer reflect the path on the file system for the vendor libraries.

For the time being I've went to parsing autoload_static.php and autoload_files.php and prefix the file hashes in there to trick autoloader into loading it again

But this is then a 3 step process:

  1. scoper
  2. dumper autoloader
  3. fix autoloader

Any ideas how to best move forward here?

@theofidry
Copy link
Member

I think fixing the autoloader itself is a must avoid: we are jumping in an incredibly more complex solution if we do so.

I'm quite busy at the moment so I can't take a proper look but maybe @naderman has an idea?

@naderman to avoid the whole thread, the issue here is that:

  • Guzzle relies on autoloaded files for declaring some functions
  • The scoped Guzzle works as expected, except the autoloaded files loading strategy relies on the file name + package name, which might be the same as the non-scoped Guzzle

The result is that one file is loaded (e.g. the non scoped Guzzle one) and the scoped one won't be loaded since has the same hash as the non-scoped one.

Related Composer issue: composer/composer#7942

@patrickjahns
Copy link

Understood - however I am not sure how to catch this in scoper - when one is supposed to dump the autoloader after using scoper.

What solved it for me was this simple script"

<?php
/**
 * This helper is needed to "trick" composer autoloader to load the prefixed files
 * Otherwise if owncloud/core contains the same libraries ( i.e. guzzle ) it won't
 * load the files, as the file hash is the same and thus composer would think this was already loaded
 *
 * More information also found here: https://github.com/humbug/php-scoper/issues/298
 */
$scoper_path = './build/scoper/vendor/composer';
$static_loader_path = $scoper_path.'/autoload_static.php';
echo "Fixing $static_loader_path \n";
$static_loader = file_get_contents($static_loader_path);
$static_loader = \preg_replace('/\'([A-Za-z0-9]*?)\' => __DIR__ \. (.*?),/', '\'a$1\' => __DIR__ . $2,', $static_loader);
file_put_contents($static_loader_path, $static_loader);
$files_loader_path = $scoper_path.'/autoload_files.php';
echo "Fixing $files_loader_path \n";
$files_loader = file_get_contents($files_loader_path);
$files_loader = \preg_replace('/\'(.*?)\' => (.*?),/', '\'a$1\' => $2,', $files_loader);
file_put_contents($files_loader_path, $files_loader);

@theofidry
Copy link
Member

Maybe a way would be to collect the included scoped files and append them to the dumped autoloader?

@polevaultweb
Copy link

polevaultweb commented Oct 16, 2019

I'm here with the same problem, I have a WordPress plugin bundling the scoped AWS SDK, but it loads a functions file before another plugin with the SDK.

@theofidry have you had any further thoughts on this?

@patrickjahns is #298 (comment) still your working solution? Where does that code live? Found it https://github.com/owncloud/files_primary_s3/pull/237/files#diff-89e7fc97925a92c2521656947a720895

Update: Thanks @patrickjahns - works a treat!

@theofidry
Copy link
Member

No I couldn't really check. One would need to check if, when the scoped file is appended to the scoper-autoload.php like it is already done for class & function aliases, works. If that's the case then it could be possible to collect those kind of files to append them to the scoper autoloader

@theofidry
Copy link
Member

theofidry commented Jan 22, 2020

Thinking about this again, a possible solution would be to have a FileCollector class and:

  • when coming across a installed.json file:
    • collect all the files listed in autoload.files (which can only be files, not directories, if I understood correctly) in FileCollector
    • remove all those files from the installed.json
  • when coming across a composer.json:
    • collect all the files listed in autoload.files in FileCollector
    • remove all those files from composer.json
  • ignore composer.lock files

And then when dumping the PHP-Scoper autoloader we can append all those files there and they should not be in the regular Composer vendor/composer/autoload_files.php (even after dumping the autoloader again).

I can help out if one desires to implement this, but I'm unlikely to work on this in a foreseeable future

@althaus
Copy link

althaus commented Sep 7, 2021

Any momentum left here? The issue prevents us from using psalm.phar on any code using symfony/polyfill-80.

@mclaurent
Copy link

Hi

Have come across this issue aswell and awaiting a fix.

In the meantime, I've added a custom patcher that bypasses this issue.

//...
return [
    'patchers' => [
        static function (string $filePath, string $prefix, string $content): string {
            $path = dirname( __FILE__ ) . DIRECTORY_SEPARATOR . implode( DIRECTORY_SEPARATOR, [ 'vendor','composer','autoload_real.php' ] );
            if ( 0 === strcasecmp( $filePath, $path ) ) {
                $content = str_replace( '\'__composer_autoload_files\'', '\'__composer_autoload_files_myapplication\'', $content );
            }
            return $content;
        },
    ],
];

@theofidry
Copy link
Member

Saving this discussion with Ondřej as Slack will likely erase it soon:

  • PHPStan solves it here; It unsets all hashes except for the non-prefixed ones.
  • this test checks to know when the autoloaded files changed

@theofidry theofidry modified the milestones: 1.0.0, 0.18.0 Nov 13, 2022
@theofidry
Copy link
Member

Note for myself: the best lead is to update the installed.json file to change the name of the libraries. Alternatively it would be changing the path of the files but that looks a bit harder to me.

@theofidry
Copy link
Member

Closed by #774

@tkfv
Copy link

tkfv commented Aug 28, 2023

hello, in a WordPress context, my plugin is scoped, the other one is not, we both depend on guzzle. With PHP scoper 0.18.3 it seems that the autoloaded guzzle file of the other plugin is still not loaded. Is this to be expected? I run composer dump-autoload after scoping.

@theofidry theofidry reopened this Sep 25, 2023
@theofidry
Copy link
Member

@tkfv would you have a reproducer by any chance?

@theofidry
Copy link
Member

I'll be closing this issue for now. If you experience any problem please open a new issue with a reproducer so that I can investigate 🙏

theofidry added a commit that referenced this issue Nov 4, 2023
In [0.18.0-rc.0](https://github.com/humbug/php-scoper/releases/tag/0.18.0-rc.0), more specifically in #298, the composer autoload global variable is completely reset. This is actually a problem for when there is excluded files, as they now count as not loaded and if included again in another project, will be loaded, which can cause problems.
@janboddez
Copy link

janboddez commented May 3, 2024

I seem to be running into this (well, something similar!) again. My build/vendor/autoload_files.php (in a WordPress plugin) looks like this:

<?php

// autoload_files.php @generated by Composer

$vendorDir = dirname(__DIR__);
$baseDir = dirname($vendorDir);

return array(
    '757772e28a0943a9afe83def8db95bdf' => $vendorDir . '/mf2/mf2/Mf2/Parser.php',
    'a01125dfebcda7ec3333dcd2d57ad8f2' => $baseDir . '/../includes/functions.php',
);

I used PHP-Scoper v0.18.7.

My composer.json is this:

{
    "require": {
        "mf2/mf2": "^0.5"
    },
    "autoload": {
        "classmap": ["includes/"],
        "files": ["includes/functions.php"]
    }
}

Now, the thing is, I have another plugin of which the autoload_files.php file looks exactly the same as the one above. (Because it also uses the same Parser library, and a file called includes/functions.php.)

When I first encountered this issue (which was fixed by moving to PHP-Scoper v0.18.x), it was the first line (mf2/mf2/Mf2/Parser.php) that caused problems. As if the (scoped) class was included only once, and a differently scoped (copy of that same) class would never load. Anyway, that was fixed, which is great!

This time, however, it seems to be the second line (/../includes/functions.php).

I'm guessing this is because I used the same filename/relative path in different plugins. (The contents of both files, and their namespaces, are very different, though.) In fact, if I rename the first file (to includes/helpers.php or something), and then run composer update and again run PHP-Scoper, the conflict is gone.

But now I'm worried that any other WordPress plugin using such a filename/path (and Composer or PHP-Scoper) could cause a (future) conflict.

Or is this just how Composer works, nothing to do with PHP-Scoper?

In that case, I should probably just include the file directly (I'm going to need it anyway), and I guess I learned something today. But then why does it work for the Parser.php file?

@janboddez
Copy link

When I first encountered this issue (which was fixed by moving to PHP-Scoper v0.18.x), it was the first line (mf2/mf2/Mf2/Parser.php) that caused problems.

Never mind, this thing is now also (again) failing: Call to undefined function IndieBlocks\Mf2\parse()

@janboddez
Copy link

And the issue seems gone now that I moved from the Activitypub\Addon to a AddonForActivityPub namespace.

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

No branches or pull requests