-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use native composer autoloader #9241
base: master
Are you sure you want to change the base?
Conversation
What's the reason for replacing composer.json-dist with composer.json. On upgrade we can't just sync that file. It will remove all plugins/skins/packages added by the user. |
Yes, upgrade script is the last place with |
8103e9d
to
534ae0b
Compare
I have reworked/simplified this PR to update the autoloading only, eg. to replace the custom autoloaders in favor of native composer autoloader. The advantages are summarized in the PR description. |
Ok, less contentious, I guess, but still
|
d46255f
to
b8f30e3
Compare
b8f30e3
to
03f8676
Compare
FYI, I know two plugins that depend on |
176b64f
to
b4ca4e9
Compare
1c50e1b
to
61f1019
Compare
61f1019
to
f0325f6
Compare
PR is done, all autoloaders are migrated to native composer. External plugins needs to be updated if they do not utilize native composer already. |
Not everyone uses composer to install plugins, not everyone can, not everyone has command line access to their hosting or is confident enough to use it. This PR prevents those people from being able to use internal or external plugins which use the autoloader. Please consider retaining the Roundcube autoloader or do something else to maintain the functionality of plugins used in none composer environments. If Composer is to become a requirement then please update the documentation e.g. https://github.com/roundcube/roundcubemail/wiki/Plugin-API#installing-and-activating-plugins to reflect this and also what simply adding the plugin name to your config file is no longer enough to enable internal plugins. |
Internal plugins should be in "require" not "require-dev", I suppose. Anyway, I'm not sure I like such a big composer.json file. I'd prefer to keep it simpler. More complicated it becomes, more error-prone updates become. I understand it allows to remove some code in tests, but other than that it's not a big win. That part regarding plugins, I mean. As for the general autoloader, I like that part. That being said, I would accept this only for Roundcube 2.0. We might decide to go with 2.0 instead of 1.7 if we merge other breaking changes, specifically #9294. |
The plugins are not hosted on Packagist, ie. Other reason to keep them in "require-dev" is to allow plugins customization using composer in child packages. Becauose of this I belive it is wanted to the bundled plugins listed in "require-dev". For distribution/downloads, the
I support this. I will rework this PR to keep the old autoloading working. I am fairly confident it can coexist until v2.0 at least. |
I have reviewed the removed/replaced original autoloader methods. Let's separate the autoloading to two domains. ROUNDCUBE INTERNAL AUTOLOADING - all autoloading code loading files withing this repo. With this PR, such loading is replaced with composer native autoloading mechanism. There any many advantages, it is faster, more secure, static analysers can rely on "descriptive autoloading", and it also allows to use this package as a dependency easily. The "disadvantage"/BC break is composer is needed. I do not think this is a problem as long as the "composer update" is run before the files are packged and distributed. This is already done for "complete distribution bundle" - https://roundcube.net/download/.
With this said, composer autoloader is fine to be used for v1.7/master. Composer files are distributed and no composer is needed for the install (on hosting). CUSTOM PLUGIN AUTOLOADING - autoloading of files outside this repo. The preferred autoloading is using composer for the same reason as for internal files. However, I agree with #9241 (comment) above, currently the Roundcube does not require composer to be installed/rerun when a plugin is added and it should stay so. This has been fixed in fd0d669. AFAK the plugin main file (file named like the plugin directory) is kept loaded by Roundcube as before and if the plugin relies on autoloading using custom With this said, I belive this PR can be landed. Feel free to test this PR and let me know if there are any concerns. |
fd0d669
to
b814691
Compare
a2bc8e5
to
cf99023
Compare
cf99023
to
d15b9fc
Compare
d15b9fc
to
6538a28
Compare
In #9407 and #9412 composer autoloading was introduced to coexist with the legaly autoloader.
This PR replaces the legacy autoloader so composer is newly needed.