inpsyde / vip-composer-plugin

A Composer plugin to ease deployment to wordpress.com VIP servers alongside Composer-based local development.
MIT License
12 stars 0 forks source link

[Bug]: Autoloading of files fails with Composer >= 2.3 #7

Closed Chrico closed 2 years ago

Chrico commented 2 years ago

Description of the bug

Due some change on Composer v2.3 and dropped support of PHP < 7.2 the path for the generated autoload file is not correct anymore.

The change on Composer was following:

v2.3 https://github.com/composer/composer/blob/2.3.0/src/Composer/Autoload/AutoloadGenerator.php#L983

self::\$loader = \$loader = new \\Composer\\Autoload\\ClassLoader(\\dirname(__DIR__));

v.2.2 https://github.com/composer/composer/blob/2.2/src/Composer/Autoload/AutoloadGenerator.php#L985

self::\$loader = \$loader = new \\Composer\\Autoload\\ClassLoader(\\dirname(\\dirname(__FILE__)));

We see now following fatal error:

PHP message: Compile error: require(): Failed opening required /var/www/wp-content/client-mu-plugins/vendor/vip-autoload/../../../../vip/plugins/{snipped}/inc/helpers.php

It seems, the paths is incorrect. 🤔

Reproduction instructions

Deploy to VIP with Composer v2.3 and autoload.files in one of the packages.

Expected behavior

Path replacement should work with Composer v2.3.

Environment info

Relevant log output

No response

Additional context

No response

Code of Conduct

gmazzap commented 2 years ago

dirname(dirname(__FILE__)) is identical to dirname(__DIR__), that's not the issue.

The problem is that Composer VIP plugin replaces the default path in autoload_classmap.php with paths that works on VIP.

In fact, when deployed, the devault vendor/composer/autoload folder is not used, but a vendor/composer/vip-autoload is used instead.

In that replacementfolder, we use VIP constants like WPCOM_VIP_CLIENT_MU_PLUGIN_DIR, WP_CONTENT_DIR, and ABSPATH to make sure correct paths are used.

But Composer v2.3 does not use anymore autoload_classmap.php, the class map is now in a separate class in a separate file.

autoload_classmap.php is kept for backward compatility, but unused. So Composer VIP plugin replaces something unused, and the object used is not replaced because Composer VIP knew nothing about it.

The problem is not seen locally, but only online, because the files in vendor/composer/vip-autoload are only used online, locally the plugin keep the standard Composer autoload.

Seldaek commented 2 years ago

I do not understand how your code worked before, because in the old code:

https://github.com/composer/composer/blob/2.2/src/Composer/Autoload/AutoloadGenerator.php#L1002-L1007

On PHP 5.6+ it would always only load the autoload_static.php already, and ignore everything else. So now the else block is gone, but I don't know how you ever had autoload_classmap.php changes "active".

Chrico commented 2 years ago

It looks like some condition isn't fullfilled. Just checked a deployment from yesterday and it produced with Composer v2.2 following:

 $useStaticLoader = false;
        if ($useStaticLoader) {
            require __DIR__ . '/autoload_static.php';

            call_user_func(\Composer\Autoload\ComposerStaticInitbeb4a7d289ba7d7c73b1ea3566f80968::getInitializer($loader));
        } else {
            $classMap = require __DIR__ . '/autoload_classmap.php';
            if ($classMap) {
                $loader->addClassMap($classMap);
            }
        }

So it was actually not using the autoload_static.php (see comment below from Giuseppe).

gmazzap commented 2 years ago

I do not understand how your code worked before, because in the old code:

@Seldaek I forced $useStaticLoader = false (see https://github.com/inpsyde/vip-composer-plugin/blob/master/src/Task/GenerateProductionAutoload.php#L134)

And that because static autoloader was harder to replace considering the paths in there where not predictable.

I don't think the culprit is Composer, we are doing things that Composer is not supposed to foresee or take into account, I would have only appreciated a mention somewhere that autoload changed, and that had rang a bell for me.

Seldaek commented 2 years ago

Yeah ok.. you voided the warranty a few times there sorry but not sorry it broke :)

Isn't there a way you can register your own autoloader with your own classmaps that takes precedence over the composer one, instead of doing such hackery in files you don't own? That would seem much wiser to me.

gmazzap commented 2 years ago

Isn't there a way you can register your own autoloader

The problem is that we need to replace the paths for all the packages, so we would need to parse all installed packages autoload config (PSR-4, files, classmap, exclude-from-classmap) and then generate the autoloader.

So basically we would need to do the same thing Composer do, using the same classes and methods Composer uses, with a slight difference on the path used as "base".

And that would happen twice, everytime the autoload is generated.

Seldaek commented 2 years ago

You can reuse the autoload_classmap.php & other files from Composer as base instead of regenerating them from scratch (that's why I left them there for BC, despite them being unused by Composer itself now), all I mean is that you can then register your own ClassLoader which loads your modified files, and register/include that instead of including vendor/autoload.php in your prod env?

gmazzap commented 2 years ago

Yep, that's the idea we were talking internally. Do ll the replacements we do now, and then instead of muddering with autoload_real.php, register a separate autoload that use the modified files.

That would ensure compat with both 2.3 and lower versions.

gmazzap commented 2 years ago

And if you'll ever delete those autoload_*.php files, please mention that in release notes :)

Seldaek commented 2 years ago

Yes for sure that I'd consider a BC break because there are tons of things relying on that for inspecting classes/files/..

gmazzap commented 2 years ago

Not sure why this is still open.

The bug is fixed since 1.5.0.