Reverted autoloader patch

[sreichel]

Description (*)

Related Pull Requests

1. See Moved phpseclib, mcrypt_compat, Cm_RedisSession, Cm_Cache_Backend_Redis and Pelago_Emogrifier to composer #2411
2. Needs Fixed baseline, ref #2785 #2789

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)
• Add yourself to contributors list

[tmotyl]

please add more context here

[sreichel]

@tmotyl These lines are added by composer install and should not be there.

[elidrissidev]

Since a lot of third-party code was removed from the repository, why not add this back? If I'm understanding it right, all the installation methods need composer autoloader now. Even the release builder is bundling the vendor folder which requires the vendor/autoload.php to be required.

[sreichel]

@elidrissidev i think its required when you have a custom vendor path ...

[elidrissidev]

The code already checks the file in root folder and one level above (assuming you installed OM with composer and set the installation path to htdocs for example) which are enough IMO, no?

[sreichel]

No time to test it now ...

... when i set "vendor-dir": "test123" in composer.json the added code looks like ...

/** AUTOLOADER PATCH **/
if (file_exists($autoloaderPath = BP . DS . 'test123/autoload.php') ||
    file_exists($autoloaderPath = BP . DS . '../test123/autoload.php')
) {
    require $autoloaderPath;
}
/** AUTOLOADER PATCH **/

Will review later.

[elidrissidev]

Yeah I know but what are the odds, almost all PHP projects use "vendor" as folder name.

[sreichel]

I know its annoying to not-commit everytime ... but do we realy need to change it?

[fballiano]

I raised this same question before and the problem was with the custom vendor.

it's not that annoying not to commit it, but the problem is that it can get commited by mistake and if it's a big commit it may go unnoticed.

I would prefer to have it in the repo so that I wouldn't need to waste any second to check before commit.

If the composer-plugin would rewrite the patch at every install it would solve our problem.

[Flyingmana]

there are ways to make this change ignored by git, might be useful during development on OpenMage, as long as one doesnt need to change this specific file.
https://stackoverflow.com/questions/10755655/git-ignore-tracked-files

also, as we now require some parts as composer dependencies and the release builds contain composer installed modules anyway, it probably does not make a difference anymore if its added or not.

[fballiano]

@Flyingmana would it be possible for the composer plugin to just rewrite the patch at every install instead of just checkin if the file it's already patched?

[Flyingmana]

@Flyingmana would it be possible for the composer plugin to just rewrite the patch at every install instead of just checkin if the file it's already patched?

should not be hard to do https://github.com/Cotya/magento-composer-installer/blob/main/src/MagentoHackathon/Composer/Magento/Patcher/Bootstrap.php#L127

might do it somewhen during the week and likely extend the config so its configurable which patch behavior is used.

[elidrissidev]

Could we push the patch to app/etc/includes/autoload.php and then ignore that file in git? Files in app/etc/includes/*.php are already getting included in Mage.php, and it'll also make it easier to rewrite the patch if the vendor path changes.

[colinmollenhour]

I've found that with this removed it is a hassle for example if I create a clean install and then use "git stash" now my store is broken and I have to run composer again.. Just having it always present seems a whole lot simpler.

If the only objection is that someone might have a different vendor folder name, does anyone actually know of any Magento users who did that, and was there a legitimate reason for it?

[fballiano]

it happened maaany time that we (mosly me actually hehehe) committed that patch by mistake, it makes our life very difficult.

maybe there was also somebody with a vendor folder outside the document root? I don't remember.

I'll create a PR to introduce it and let's see what happens there?

[fballiano]

@elidrissidev what if we add it in the default place and if somebody really needs they can add their own version to app/etc/includes?

[colinmollenhour]

One possibility would be to introduce an environment variable like "COMPOSER_VENDOR_PATH" so it could be changed that way while the default would be to assume the BP.

/** AUTOLOADER PATCH **/
if (file_exists($autoloaderPath = (getenv('COMPOSER_VENDOR_PATH') ?: BP) . DS . 'vendor/autoload.php')) {
    require_once $autoloaderPath;
}
/** AUTOLOADER PATCH **/

[fballiano]

I think it would have to be

/** AUTOLOADER PATCH **/
if (file_exists($autoloaderPath = (getenv('COMPOSER_VENDOR_PATH') ?: BP . DS .  'vendor') . DS . 'autoload.php')) {
    require_once $autoloaderPath;
}
/** AUTOLOADER PATCH **/

because the vendor word itself could be overridden

[sreichel]

now my store is broken and I have to run composer again.. Just having it always present seems a whole lot simpler.

Isnt composer install a "default" command during deployment process?

[colinmollenhour]

now my store is broken and I have to run composer again.. Just having it always present seems a whole lot simpler.

Isnt composer install a "default" command during deployment process?

Yes, but when doing development on the core code after you clone the git repo and run the installation, now if you want to commit changes or do a git stash or git reset --hard HEAD or various other git operations you have to constantly dance around the changes to app/Mage.php to avoid inadvertently committing them and/or run composer install to keep the environment working. This is the kind of thing .gitignore and config files or environment variables are for.