Skip to content
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

Allow composer autoloading (to coexist) #9407

Merged
merged 27 commits into from
Apr 10, 2024

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Apr 7, 2024

extracted from #9241

This PR is 100% BC and does not imply any functional change.

This PR allows composer autoloading to address the goals from #9241, but keeps the Roundcube autoloading intact.

Later, the transition to composer autoloading can be discussed in the original #9241 with minimal LoC needed.

@mvorisek mvorisek force-pushed the allow_composer_autoloading branch from aeadc74 to eaf9adf Compare April 7, 2024 09:52
@mvorisek mvorisek changed the title Allow composer autoloading Allow composer autoloading (to coexist) Apr 7, 2024
@mvorisek mvorisek marked this pull request as ready for review April 7, 2024 10:11
@mvorisek
Copy link
Contributor Author

mvorisek commented Apr 7, 2024

PR is done and does not intact the current autoloading in any way. However, Roundcube/plugins can be newly easily used using composer as deps 🎉🎉🎉

@alecpl
Copy link
Member

alecpl commented Apr 7, 2024

Looks good. I wonder whether we could do something that does not look like a hack regarding that change in rcmail_oauth. How about skipping rcmail_oauth instantiation in rcmail::startup() if PHP_SAPI == 'cli'?

@mvorisek
Copy link
Contributor Author

mvorisek commented Apr 7, 2024

Looks good. I wonder whether we could do something that does not look like a hack regarding that change in rcmail_oauth. How about skipping rcmail_oauth instantiation in rcmail::startup() if PHP_SAPI == 'cli'?

I do not support this. The reason is I belive it should be initialized for every environment (web & CLI).

Here we solve the first composer install/update only. The reason is composer classmap autoloader has not discovered the classes yet (when the roundcube plugin installer composer plugin is called). For consequent run, the class is autoloadable/present, thus disabling this for CLI in general is not needed and IMO not wanted.

@alecpl
Copy link
Member

alecpl commented Apr 7, 2024

It would also break oauth tests. So, maybe the ExtensionInstaller should include iniset.php instead of clisetup.php. I.e. not initialize rcmail at all. I'm not sure whether this would just work or some more changes would be needed.

@mvorisek mvorisek force-pushed the allow_composer_autoloading branch from 27b349b to f6be053 Compare April 10, 2024 10:07
@mvorisek mvorisek marked this pull request as ready for review April 10, 2024 10:09
@mvorisek
Copy link
Contributor Author

@alecpl now this PR is complete and fully BC. Please merge it and submit this repo to Packagist. I will then take care of the remaining changes.

@alecpl alecpl merged commit 52d8172 into roundcube:master Apr 10, 2024
16 checks passed
@mvorisek mvorisek deleted the allow_composer_autoloading branch April 10, 2024 10:30
@alecpl
Copy link
Member

alecpl commented Apr 12, 2024

ps. roundcube/roundcubemail has been registered on Packagist.

@mvorisek
Copy link
Contributor Author

Thank you very much ❤

I did a quick verification using:

{
    "repositories": [
        {
            "type": "path",
            "url": "vendor/roundcube/roundcubemail/plugins/acl"
        }
    ],
    "require": {
        "roundcube/roundcubemail": "dev-master",
        "roundcube/acl": "^1.9"
    },
    "minimum-stability": "dev",
    "prefer-stable": true,
    "config": {
        "allow-plugins": {
            "roundcube/plugin-installer": true
        }
    }
}

and all works as expected incl. plugins 🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants