Composer autoloader patch was added to app/Mage.php

[fballiano]

There were multiple discussions about this but they're kinda lost all over the repository and kinda, what about we gather them here?

This PR is born from the last comments to #2791 and it is based on the last @colinmollenhour suggestions and allows for a configurable vendor folder, which should solve everybody's problem?

[colinmollenhour]

Tested:

• Autoloader works (no fatal errors)
• Running composer install no longer modifies Mage.php ("./app/Mage.php was already patched")

@sreichel @Flyingmana Are you aware of any production setups that use the "vendor-dir" option and what the reasons are for it?

It'd be great if this was a MINOR update as although it is not a huge issue, it is pretty annoying. But if there are people using "vendor-dir" maybe it should be MAJOR to be safe. In such cases setting the COMPOSER_VENDOR_PATH should be an easy fix.

[Flyingmana]

only in very few cases, and then I think to have it outside the root directory.

I dont think its a notable number, and they could use the patch functionality for it

[colinmollenhour]

Is that the purpose of the check for '../vendor' to cover users who set the vendor-dir to the parent?

It seems that is a minor security concern for if a user was using the parent path but a hacker could create vendor/autoload.php then they could effectively override the true autoload.php file with their own. Basically by checking for that one first it would circumvent the whole purpose of moving it to the parent path in the first place, right? The version in this PR removes the possibility to accidentally load the wrong autoload.php file.

[fballiano]

@Flyingmana they should be able to set the COMPOSER_VENDOR_PATH to the absolute path to the actual vendor folder (or ../custom-vendor too for example), it should cover all the cases if I'm not missing something (could very well be ehhehe)

[sreichel]

@sreichel @Flyingmana Are you aware of any production setups that use the "vendor-dir" option and what the reasons are for it?

No, im not. I still we coukd keep the option to set custom vendor dir in composer.json.

[fballiano]

you can customise it in the composer.json, you'll just have to add the COMPOSER_VENDOR_PATH env variable or repatch Mage.php

[sreichel]

But if there are people using "vendor-dir"

If vendor-dir is used in a companies/team workflow, everyone has to set the ENV variable now?

imho ... this fixes something, that is not broken :/

[fballiano]

@colinmollenhour the idea was yours (I just turned it into a PR), I support it and it has 2 other positive reviews, what are we doing?

[colinmollenhour]

If vendor-dir is used in a companies/team workflow, everyone has to set the ENV variable now?

A user could also skip the env variable and create vendor as a symlink to link to their vendor path at the default location. I just tried this and it appears to work just fine.

I think environment variables are the most broadly supported and widely used way of handling this type of thing.

• In Apache: SetEnv COMPOSER_VENDOR_PATH /path/to/vendor
• In a Docker container: with ENV or with docker-compose.yaml
• Non-Docker environments with CLI: /etc/environment or /etc/profile or many other ways

This is the best way I can come up with that doesn't add any security risk or require any modifications to the magento-core-composer-installer package.

I'm sure everyone is happy to consider alternative propsoals, but it seems many also agree that having a default way to include the composer autoloader without modifying core files now that composer is a requirement of the project just makes sense.

Please let us know if you still have any objections and/or proposal for an alternative.

[fballiano]

I merged it, everything can be changed anyway, in case a better solution comes ahead.

[colinmollenhour]

So I had never tried the composer way to install OpenMage, I always just used git clone..

Now I see that even our default install procedure puts the vendor path above the BP so this PR was a bit ill-conceived.. I propose having the patch still included in the core code, but going back to having it check the parent directory before the current directory:

/** AUTOLOADER PATCH **/
$autoloaderPath = getenv('COMPOSER_VENDOR_PATH');
if (!$autoloaderPath) {
    $autoloaderPath = dirname(BP) . DS .  'vendor');
    if (!is_dir($autoloaderPath)) {
        $autoloaderPath = BP . DS . 'vendor';
    }
}
require $autoloaderPath;
/** AUTOLOADER PATCH **/

I'll open a new PR..

[Flyingmana]

You did Check with the Implementation to check If the Patch is already applied? https://github.com/Cotya/magento-composer-installer/blob/ece5cc5bb2a0150a743619901543036597f9eb5b/src/MagentoHackathon/Composer/Magento/Patcher/Bootstrap.php#L97

[colinmollenhour]

You did Check with the Implementation to check If the Patch is already applied?

Yes, that part was tested and works fine. One goal was to avoid requiring changes to that library to keep it simple.

What was not tested was installing with a core installer like aydin-hassan/magento-core-composer-installer - I think that should also just work out of the box.