contao / manager-plugin

Contao Manager Plugin
GNU Lesser General Public License v3.0
4 stars 9 forks source link

Do not replace class but use a separate generated file instead #42

Closed Toflar closed 2 years ago

Toflar commented 2 years ago

Currently, on every composer install or composer update, we actually dynamically replace the PluginLoader class with the defined plugins. While this might be fine for production use, it has two major drawbacks during development:

Generally speaking, I also think it's bad practice to dynamically replace a PHP class that is then used. This PR changes the logic so that the plugins are stored inside vendor/contao/manager-plugin/generated/plugins.php and included in the PluginLoader. It's still an immutable array so it can be cached by the OPCode cache. The /.generated folder inside the directory is gitignored so git status --porcelain --untracked-files=no won't complain about changed files in there.

leofeyer commented 2 years ago

Generally speaking, I also think it's bad practice to dynamically replace a PHP class that is then used.

This is how composer/package-versions-deprecated works, too. I donβ€˜t think it is bad practice per se.

But you should not commit the dynamic class into your Git repo, of course. Neither the PackageVersion\Versions class nor our Contao\ManagerPlugin\PluginLoader class.

Toflar commented 2 years ago

Which is what we're doing at the moment though, that's why I've extracted it :D

aschempp commented 2 years ago

Maybe we should follow composer/package-versions-deprecated and generate the class to vendor/composer?

Toflar commented 2 years ago

Is there any official recommendation as to where a generated PHP file (in order to improve performance) should be stored by any Composer plugin? In its own directory (vendor/<vendor-name>/<package-name>) or in vendor/composer/<vendor-name>/<package-name>? @Seldaek maybe 😊

Seldaek commented 2 years ago

composer/package-versions-deprecated generaets stuff in its own directory (much like ocramius/package-versions did. vendor/composer/InstalledVersions.php is generated by composer (2) itself not that plugin.

vendor/composer is as far as I am concerned reserved for our usage and anyone doing stuff in there better not complain if things break :p

Seldaek commented 2 years ago

After a quick glance at the diff, the PR seems sensible to me as it is πŸ‘πŸ»

Toflar commented 2 years ago

This PR also fixes compat with Composer 2.3 :)

leofeyer commented 2 years ago

Thank you @Toflar.