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

Add allow-plugins config. #6

Closed
wants to merge 1 commit into from
Closed

Conversation

paul121
Copy link
Member

@paul121 paul121 commented Dec 27, 2021

Composer 2.2.0 has a new allow-plugins config option that we should define in our composer project: https://getcomposer.org/doc/06-config.md#allow-plugins

I noticed this warning when running composer install locally, you can also see it in the github action tests: https://github.com/paul121/farmOS/runs/4635317246?check_suite_focus=true#step:6:1034

For additional security you should declare the allow-plugins config with a list of packages names that are allowed to run code. See https://getcomposer.org/allow-plugins
You have until July 2022 to add the setting. Composer will then switch the default behavior to disallow all plugins.

I've committed the config that is added to the composer.json after running composer install and allowing all the plugins that are prompted.

@mstenta
Copy link
Member

mstenta commented Dec 28, 2021

Thanks @paul121! Didn't know this was coming...

Do we want to consider adding this to https://github.com/farmOS/farmOS/blob/2.x/composer.project.json instead of this repository? Or some here and some there (depending on which packages are explicitly used in each)? That assumes that composer-merge-plugin can merge the config section, I suppose... I'm not sure it can.

Did you find that you needed to declare both oomphinc/composer-installers-extender and composer/installers explicitly, even though we don't use composer/installers directly? The former depends on the latter, so does that mean allow-plugins needs to declare plugins that are sub-dependencies too? Just curious... seems like that could cause things to break if a dependency adds a plugin in the future... but that's a broader question for allow-plugins upstream more generally I guess.

@paul121
Copy link
Member Author

paul121 commented Dec 28, 2021

That assumes that composer-merge-plugin can merge the config section, I suppose... I'm not sure it can.

Ah yeah, not sure either.. I imagine it would need to be updated to support that. I could see it either way...

Did you find that you needed to declare both oomphinc/composer-installers-extender and composer/installers explicitly, even though we don't use composer/installers directly?

I didn't try that, but it did prompt for each of these plugins, so I imagine we do need to declare it. The GitHub actions log doesn't show this but if you run composer install locally on the current composer.json it will prompt you yes/no for each of the plugins.

We could also allow all plugins by setting allow-plugins to true.. but ideally we can avoid that.

Maybe there is more information on best practices somewhere outside the documentation? I didn't have much time to look deeper into this.

@paul121
Copy link
Member Author

paul121 commented Dec 29, 2021

I don't think it answers any of our questions, but here is the 2.2.0 release announcement: https://blog.packagist.com/composer-2-2/

@mstenta
Copy link
Member

mstenta commented Dec 30, 2021

Thanks @paul121!

I found these two related upstream issues/patches in the Drupal queue, one for the Drupal core composer.json (already merged into 9.3.x, 9.4.x, and 10.0.x), and the other for the "recommended" project templates (equivalent to our farmOS/composer-project template):

The first one (Drupal core composer.json) makes me wonder if we need to do the same in our farmOS composer.json? https://github.com/farmOS/farmOS/blob/2.x/composer.json - Although it's not clear from that issue if that is actually necessary, or if it would have also been covered by the proposed change to the recommended project template... Wouldn't hurt to add to our composer.json either way I suppose.

As for the project template itself, I wonder if it would actually be better to omit this and let it be a conscious choice by the end-user to allow?

There are two scenarios (I think):

  1. We use composer create-project for our own build process, and we obviously want that to continue to work without interruption. It actually sounds like there are two ways approach this:
    1. We add allow-plugins to our project template and explicitly declare (and maintain) each plugin as a line in that file.
    2. We use the --no-interaction argument when we run composer create-project, which will automatically accept the plugin (it sounds like - I have not tested this).
  2. The other scenario is an end-user running composer create-project themselves. It sounds like (though I haven't tested), that they will be prompted to allow each plugin, and when they say "yes" it automatically adds the allow-plugins section to their composer.json (which they are responsible for managing henceforth anyway).

I kind of like the idea of leaving this as an end-user build step. The only people who will be running composer create-project are going to be the advanced users who want to manage their installations via Composer anyway - so they know what they're doing - and this is an acceptable step IMO. AND it allows us to leave these lines out of our project template, which keeps things simpler (and less to maintain if we add/remove plugins in the future).

What do you think? Either way we'll want to test some of my assumptions here before committing to a path...

I think we can wait until after farmOS 2.0.0-beta1 for this, since the default for allow-plugins will not change from null to {} until July 2022, which gives us some time. I assume we'll be making another beta release before then. And in the meantime we can also see what approach Drupal core decides to take.

@paul121
Copy link
Member Author

paul121 commented Dec 30, 2021

The first one (Drupal core composer.json) makes me wonder if we need to do the same in our farmOS composer.json?

Good digging @mstenta! Well the config key is "root-only" so that change shouldn't effect composer projects. https://getcomposer.org/doc/04-schema.md#config

But maybe that composer.json is meant to be used for development as well? The steps to reproduce in that issue were "get core from git and use composer ... install" which seems like a development workflow. But like you say, we actually use composer create-project for our build process - so maybe we don't need to update our farmOS composer.json?

As for the project template itself, I wonder if it would actually be better to omit this and let it be a conscious choice by the end-user to allow?

+1 yes I tend to agree with this!

We use the --no-interaction argument when we run composer create-project, which will automatically accept the plugin (it sounds like - I have not tested this).

I think our decision will ultimately depend on how this works... and unfortunately it seems this will only work (even with --no-interaction) until July 2022 - good info in this PR: composer/composer#10314 (comment)

This has already been decided for the 2.2 LTS release, but maybe we can revisit this when 2.3 comes out (presumably it will require allow-plugins to be defined, and be out before July 2022): composer/composer#10314 (comment)

If we don't want to define the allow-plugins in our composer project we might need to manually do this as a part of our build process:

  1. Either set composer config --global allow-plugins true (not great)
  2. Or manually set each allowed plugin (more maintenance, but likely the proper solution)

@mstenta
Copy link
Member

mstenta commented Jan 2, 2022

unfortunately it seems this will only work (even with --no-interaction) until July 2022

Ah I see. Well then this would just be kicking the can...

manually set each allowed plugin (more maintenance, but likely the proper solution)

Yea seems like we're coming full circle on this (good to think it all through)!

Shall we just merge this PR then? :-)

@paul121
Copy link
Member Author

paul121 commented Jan 3, 2022

manually set each allowed plugin (more maintenance, but likely the proper solution)

Shall we just merge this PR then? :-)

Well, maybe not. I like what you said about downstream users of our composer project needing to make these decisions themselves. Instead of merging this here we could add some steps to our Dockerfile that will add this allow-plugins config after running composer create-project. This config should persist in the docker images we provide but we will need to maintain the plugin list going forward.

@mstenta
Copy link
Member

mstenta commented Jan 3, 2022

Sounds like a plan!

@paul121
Copy link
Member Author

paul121 commented Jan 3, 2022

I've opened farmOS/farmOS#467

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.

None yet

2 participants