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

1.15.0 breaks CI pipelines #213

Closed
mwernaert opened this issue Feb 9, 2023 · 50 comments · Fixed by #217 or #219
Closed

1.15.0 breaks CI pipelines #213

mwernaert opened this issue Feb 9, 2023 · 50 comments · Fixed by #217 or #219

Comments

@mwernaert
Copy link

PHP version: 8.1.15
Composer version: 2.5.2
Description

Composer asks if I want to trust "php-http/discovery" when 1.15.0 gets required. This breaks CI pipelines when php-http/discovery is an indirect dependency.

How to reproduce
composer require php-http/discovery

Possible Solution
Use a new major version (2.0.0) for breaking changes.

Additional context
$ composer require php-http/discovery
./composer.json has been created
Running composer update php-http/discovery
Loading composer repositories with package information
Updating dependencies
Lock file operations: 1 install, 0 updates, 0 removals

  • Locking php-http/discovery (1.15.0)
    Writing lock file
    Installing dependencies from lock file (including require-dev)
    Package operations: 1 install, 0 updates, 0 removals
    php-http/discovery contains a Composer plugin which is currently not in your allow-plugins config. See https://getcomposer.org/allow-plugins
    Do you trust "php-http/discovery" to execute code and wish to enable it now? (writes "allow-plugins" to composer.json) [y,n,d,?]
@Cluster2a
Copy link

Cluster2a commented Feb 9, 2023

You might want to add this to your composer.json and set it to false - so this plugin is being skipped.
image

@ambroisemaupate
Copy link

ambroisemaupate commented Feb 9, 2023

Same issue here.

If I allow php-http/discovery plugins, composer update is calling itself in loop until I kill it (#211).

@clxmstaab
Copy link

clxmstaab commented Feb 9, 2023

came here to report this very same problem. it seems adding a composer plugin is kind of a breaking change and should use a new major version.

we depend on php-http/discovery only as a transitive dependency and still the latest release breaks CI which is unfortunate.

@vitalij-vladimirov

This comment was marked as outdated.

@thomasnordahl-dk
Copy link

Same issue here. First impression is that is seems a bit "much" to install dependencies like that. Isn't the point of PSR libraries to offer dependency inversions for common use cases / components?

@zlodes

This comment was marked as outdated.

@frsmoray
Copy link

frsmoray commented Feb 9, 2023

Same issue here

@nicolas-grekas
Copy link
Collaborator

nicolas-grekas commented Feb 9, 2023

Adding conflict or locking are the worst solutions.
The right solution is to disable the plugin, you likely don't need it anyway in your CI:

"allow-plugins": {
    "php-http/discovery": false
 }

This can be done on the command line with
composer config --no-plugins allow-plugins.php-http/discovery false

@nicolas-grekas
Copy link
Collaborator

My analysis: no BC break here. Breaking CI is what they are for: detecting something you might need to adapt to. This is especially true for CI that run composer update (as opposed to composer install with locked dependencies), which is the case if your CI broke after v1.15.0.

@mwernaert
Copy link
Author

Enforcing a manual change in composer.json is no problem for local dev environments, but you are asking people to update hundreds of projects due to a change in a minor version of an indirect dependency. That's breaking enough in my opinion.

@FiHoEco
Copy link

FiHoEco commented Feb 9, 2023

It's also a dependency that we don't personally have. Sentry includes this package so it's not as easy as just setting it to false ourselves.

@nicolas-grekas
Copy link
Collaborator

Here is the command-line to do it if needed:
composer config --no-plugins allow-plugins.php-http/discovery [true|false]

The rationale behind the current behavior of composer is described in composer/composer#10912
It'd be great to allow a plugin to declare that it's fine to skip it in non interactive mode.

@AndreasA
Copy link

AndreasA commented Feb 9, 2023

This is definitely a breaking change, as e.g. also create-projects can now fail - where one cannot change the allow-plugins before initial setup - at least not without using --no-install.

e.g. an issue for
composer create-project sulu/skeleton:^2.5.6
fails. it works with --no-plugins but it is a bit annoying and in any case it is definitely a breaking change.

It would have been better to create a separate package for the plugin, that one can install or not.

@nicolas-grekas
Copy link
Collaborator

nicolas-grekas commented Feb 9, 2023

Backward compatibility is not about ensuring stability of installation commands. It's about the API of PHP code.

Thanks for the hint for Sulu, here is the fix: sulu/skeleton#212

@thomasnordahl-dk
Copy link

It might not count as a BC break in the sense that the APIs and so on remain unchanged. But this change is definitely highly disruptive, and achieves a feature, I don't really understand why you would want.

As I understand the point of having the generic interfaces from PSR, is that we should be able to depend solely on the interface and not worry about concrete implementations. So forcing specific implementations seems counterproductive to me.

Also for highly modular setups, it's not about just adjusting one composer.json, but going through a lot of individual packages, to allow or disallow something I didn't ask for in the first place. And I actually don't even want it. I actually just wanted a multipart stream builder, but now I have to deal with this due to php-http/multipart-stream-builder having a dependency on this package.

Since Composer 2.2, my colleagues and I have actually made efforts to avoid packages that uses composer plugins. It shouldn't generally be necessary to hook into the package manager like that, and it is just unwanted overhead to have to consider if a script that you didn't ask for is allowed to run or not. (In most cases we get these plugins as a dependency of a dependency).

@nicolas-grekas
Copy link
Collaborator

nicolas-grekas commented Feb 9, 2023

@thomasnordahl-dk that another discussion. You might want to check https://github.com/FriendsOfPHP/well-known-implementations for some explanations about why this fills a hole in the ecosystem. But it's fine to disable the plugin if you want explicit manual control over your deps. The good news with the new approach is that you won't be forced to install fallback packages anymore, as many third party libs currently require since there is no other way to provide a nice out-of-the-box experience.

@Seldaek
Copy link

Seldaek commented Feb 9, 2023

One big takeaway here is that CI should not run interactive, make sure you have -n (or --no-interaction) in your Composer commands. If you do that Composer will automatically reject any new plugin (it'll output a warning but will not block). Sorry my bad, this will fail hard but at least it won't block forever. Solution is definitely to explicitly allow or reject the plugin via the allow-plugins config.

You can also set COMPOSER_NO_INTERACTION=1 in your environment variables in CI if that is easier.

@Tofandel
Copy link

Tofandel commented Feb 9, 2023

It might be disruptive but is indeed not a direct breaking change, because if your CI fails because of an added plugin, it was not implemented correctly in the first place because it means you are not running composer in non interactive mode in your CI pipeline

Make sure you use the -n option in your CI script

composer upgrade -n

@Seldaek beat me to it

@AndreasA
Copy link

AndreasA commented Feb 9, 2023

Using create-project with non-interactive still fails if the referenced project does not have the corresponding allow-plugin section.

@PhilETaylor
Copy link

My 2cents... I manually ran composer update - said yes to this plugin - and now composer update never ever completes, it just keeps running and running, over and over and over again... surely that cannot be the expected feature? (on my Mac, interactively)

@Tofandel
Copy link

Tofandel commented Feb 9, 2023

Yes this is a bug in the plugin, being discussed in the PR

@ekes
Copy link

ekes commented Feb 9, 2023

Guess this becomes my problem not the packages. But in a CI that I can't edit top level composer.json and that is non interactive https://www.drupal.org/pift-ci-job/2589961 it stops.

@mwernaert
Copy link
Author

I have composer running with --no-interaction. What I should have mentioned is that it fails later on due to "php-http/discovery contains a Composer plugin which is blocked by your allow-plugins config."

@PhilETaylor
Copy link

Yes this is a bug in the plugin, being discussed in #208

Awesome - thanks. So there are two conversations, 1 is the bug and 1 is for the non-interactive nature.

@nicolas-grekas
Copy link
Collaborator

I'm checking the bug now.

@ekes check the last line of #213 (comment)

@nicolas-grekas
Copy link
Collaborator

The infinite loop will be fixed by #214

@nicolas-grekas
Copy link
Collaborator

nicolas-grekas commented Feb 9, 2023

Releasing a new major version will not help in any way: as soon as a library requires discovery ^1|^2, the failure will come back again.
I do understand that it's annoying, but there is not other way to push this forward unfortunately. Even creating a new dedicated package only for the plugin won't help.
It's a bit like when composer introduced the check: projects had to adapt (but no prod broke.)
The improvement to composer we discussed is another solution, but it would need time to propagate to the community so in the end, listing the plugin in failing projects is likely going to take the same time. (it took only a few weeks for the community to adapt to the new behavior of composer.)

@AndreasA
Copy link

AndreasA commented Feb 9, 2023

In a way I also think that the main issue is that composer does not seem to have an option where plugins that are not defined in the allowed-plugin list are assumed to be disallowed, which could be used in combination with no-interaction in e.g. CI/CD enviornments or other automation scripts.

Sure, the user would not use the new plugin then but that was basically the same as prior to the dependency update. I created a corresponding composer issue - as the only one I found that fit the bill was closed already with some info text being added to the CLI output (composer/composer#11314).

@jefhar
Copy link

jefhar commented Feb 9, 2023

Would it expect too much to have this added to the docs somewhere?

Adding conflict or locking are the worst solutions. The right solution is to disable the plugin, you likely don't need it anyway in your CI:

"allow-plugins": { 
    "php-http/discovery": false
 }

This can be done on the command line with composer config --no-plugins allow-plugins.php-http/discovery false

@AndreasA
Copy link

AndreasA commented Feb 9, 2023

one solution I found to avoid tihngs like this in the future is to run:

composer config --no-plugins allow-plugins.* false # Disable all non allowed plugins.

and if a new plugin is supposed to be allowed:

composer config --unset --no-plugins allow-plugins.* # Needed to ensure correct order if this is already set.
composer config --no-plugins allow-plugins.<plugin> true # Allow new plugin necessary plugins.
composer config --no-plugins allow-plugins.* false # Disable all other plugins again.

the disadvantage is that one is never notified about potentially new plugins. and that config is always active - independent of CI/CD.

Though it could probably be set in CI/CD temporarily. In which case it would always be at the end, so

composer config --no-plugins allow-plugins.* false

would be enough.

Though I am not sure if the order of allow-plugins is always used as it is defined in the file.

So in case of create-project it would be:

composer create-project sulu:skeleton:^2.0 --no-install
composer config --no-plugins allow-plugins.* false
composer install

@nicolas-grekas
Copy link
Collaborator

the main issue is that composer does not seem to have an option where plugins that are not defined in the allowed-plugin list are assumed to be disallowed

💯 this

Here is a PR to improve composer in this regards: composer/composer#11315

This would close this issue.

@AndreasA
Copy link

AndreasA commented Feb 10, 2023

Here is a PR to improve composer in this regards: composer/composer#11315

This would close this issue.

It would still fail if a new plugin is added that is optional but the maintainer did not use the corresponding flag. So the user has no option to just "ignore" this in the CI/CD.

Hoewver, in most cases it should be enough, as plugin developers hopefully take care of this - and if not it would probably be a breaking change anyway.

@nicolas-grekas
Copy link
Collaborator

Thank you all for the discussion. As far as this package is concerned, I think we did everything we could.
Next step is composer/composer#11315

@mwernaert
Copy link
Author

mwernaert commented Feb 10, 2023

Thanks for improving Composer and taking some responsibility. I still think it would have been wiser to revert the breaking change in a 1.15.1 release and push the new plugin in a 2.0 version. While I agree that sooner or later people will have to deal with allowing/disallowing the plugin, the push could have been done more gracefully. Giving package maintainers (like Sentry) a choice and chance to stick to a specific version for now, bump a major as well later on (and communicate why) or replace the dependency. I don't want to nag too much about this because it is easily resolved (and already done and communicated within my company), but the 'the damage is done, deal with it' attitude is a bit too careless for my taste. Lot's of people and package maintainers were impacted by this.

The comparison with a Composer change that required a manual change sounds fair, but people could easily compare this mistake with the left-pad NPM debacle as well.

@bensebborn
Copy link

+1 for lots of disruption caused, especially as this is just a dependancy of another package - not something we're using directly ourselves.

Consideration of the developer ecosystem needs to happen, not just brute forcing something through because it doesn't warrant a breaking chance according to standards.

@nicolas-grekas
Copy link
Collaborator

Composer v2.2.20 and v2.5.3 have now been released in case you missed them. Thanks @naderman and @Seldaek for the quick help!

leohemsted added a commit to alphagov/notifications-php-client that referenced this issue Feb 10, 2023
as per php-http/discovery#213 (comment)

> The right solution is to disable the plugin, you likely don't need it anyway in your CI
@amenk
Copy link

amenk commented Feb 10, 2023

I still see endless loops on composer install (no .lock present) in some situations - posted on StackOverflow because I am not sure if it is related to this module, composer or our own module.

@nicolas-grekas
Copy link
Collaborator

Please open a dedicated issue with the details.

@nicolas-grekas
Copy link
Collaborator

@amenk fixed as of 1.15.2 and #219

I'm locking this issue to encourage creating new dedicated issues if needed.

@php-http php-http locked as resolved and limited conversation to collaborators Feb 11, 2023
brandon-reed1 added a commit to brandon-reed1/Discovery that referenced this issue Jun 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.