deployphp / deployer

The PHP deployment tool with support for popular frameworks out of the box
https://deployer.org
MIT License
10.65k stars 1.49k forks source link

Use `--classmap-authoritative` on `composer install` #2898

Closed Schrank closed 2 months ago

Schrank commented 2 years ago

We had a recent discussion on Twitter, read the complete thread, it is interesting for non-Magento folks as well: https://twitter.com/PeterJaap/status/1481166381332807680?s=20

https://getcomposer.org/doc/articles/autoloader-optimization.md#optimization-level-2-a-authoritative-class-maps

This option says that if something is not found in the classmap, then it does not exist and the autoloader should not attempt to look on the filesystem according to PSR-4 rules.

This option makes the autoloader always return very quickly. On the flipside it also means that in case a class is generated at runtime for some reason, it will not be allowed to be autoloaded. If your project or any of your dependencies does that then you might experience "class not found" issues in production. Enable this with care.

I think "generating classes on runtime" is an edge case, were (as @peterjaap wrote on Twitter) "typos or wrong casing in class names vs file names." is not an edge case.

So what I think we should implement at least is an option to use optimization level 2/A or 2/B as per composer documentation and/or even set it as default.

Upvote & Fund

Fund with Polar

antonmedv commented 2 years ago

Make sense. Let’s add it before v7 release.

sprankhub commented 2 years ago

Wanted to leave my vote for making option 2/A (authoritative class maps) the default :hand:

sprankhub commented 2 years ago

When this is implemented, the respective option should also be added to the dump-autoload calls in the Magento 2 recipe:

https://github.com/deployphp/deployer/blob/fb6f2154ef36f03e4e4b0644206b06722ee09fd8/recipe/magento2.php#L75-L77

antonmedv commented 2 years ago

Any volunteers to implement?

Schrank commented 2 years ago

@peterjaap @sprankhub If you have time, feel free. I'm currently drowning and don't see light.

Schrank commented 2 years ago

When this is implemented, the respective option should also be added to the --dump-autoload calls in the Magento 2 recipe:

https://github.com/deployphp/deployer/blob/fb6f2154ef36f03e4e4b0644206b06722ee09fd8/recipe/magento2.php#L75-L77

Checked the rest of the codebase, Magento 2 is the only recipe we need to edit!

sprankhub commented 2 years ago

Checked the rest of the codebase, Magento 2 is the only recipe we need to edit!

You only checked for composer dump-autoload calls, right? However, this would also need to be added to composer install calls (and theoretically to composer update commands, but they should not be present here ^^).

Any volunteers to implement?

I could have a look when I have some time, but I will need some help with testing. Plus, it should be clear how exactly this should be implemented before we start: Will we make --classmap-authoritative the new default? Should it be possible to disable it? Or should it be the other way round, so that you need to enable it if you want to use it?

antonmedv commented 2 years ago

Users already can override composer_options. Let’s just add it as new default.

Schrank commented 2 years ago

You only checked for composer dump-autoload calls, right? However, this would also need to be added to composer install calls (and theoretically to composer update commands, but they should not be present here ^^).

Nope, I searched for {{bin/composer}} ^^

https://twitter.com/IvanChepurnyi/status/1481279298057162755?s=20

Can we check wether apcu is installed? If it is, we better use it 🙈 And if not we use classmaps?

sprankhub commented 2 years ago

Yeah the question is how willing @antonmedv is to potentially break other peoples' systems :stuck_out_tongue: --classmap-authoritative can indeed break systems. The issue with this is that it cannot be detected during the deployment, but only on the (potentially live) system after the deployment. Hence, maybe the solution suggested by @Schrank makes sense? If APCu is enabled, use it and if it is not, we use --classmap-authoritative (or nothing). What do you think, @antonmedv?

By the way, many in the Magento community used a deployer-based recipe by @jalogut before and there, --apcu is used:

https://github.com/jalogut/magento2-deployer-plus/blob/a8c4432a68a4ac442541f250577e6049abaed52b/recipe/magento_2_1/files.php#L17

trsteel88 commented 2 years ago

+1 for making --classmap-authoritative the default.

github-actions[bot] commented 2 months ago

This issue has been automatically closed. Please, open a discussion for bug reports and feature requests.

Read more: [https://github.com/deployphp/deployer/discussions/3888]