PHPCSStandards / composer-installer

Composer installer for PHP_CodeSniffer coding standards
https://packagist.org/packages/dealerdirect/phpcodesniffer-composer-installer
MIT License
552 stars 36 forks source link

Composer 2.x compatibility #108

Closed danepowell closed 4 years ago

danepowell commented 4 years ago

The release of Composer 2.0 is imminent, and it requires updates for composer plugins such as this package. Until this package is explicitly made compatible with Composer 2, it cannot be installed with Composer 2 (even as a dependency of another package).

This package needs to be updated not only to ensure compatibility with Composer 2, but to unblock downstream projects that rely on it and want to support composer 2.

Hopefully you received an email from the Composer maintainer about this, but if not let me know and I can forward it to you or put you in contact. Cheers.

jrfnl commented 4 years ago

@danepowell While I agree it would be good to support Composer 2.0 once it comes out, I can't find any useful information to confirm what you are saying, let alone inform Composer plugin developers on what is needed to support Composer 2.

If you have links to relevant sources, please add them to this issue as, as things stand, it is impossible to action this for now.

Seldaek commented 4 years ago

I posted the info at https://github.com/composer/composer/issues/8726 now - there still isn't a lot of what you mentioned, but it's a start :)

jrfnl commented 4 years ago

Thanks @Seldaek for chipping in and posting that information. I'll try and have a look at it in more detail soon.

Unfortunately this project doesn't have really have any integration tests at this moment, so testing this will not be easy for us as there are a lot of scenarios to consider.

Seldaek commented 4 years ago

Had a quick look through the source and tbh I don't think you'll have to change anything beyond the composer.json requirement. I might have missed something obviously but it doesn't seem to touch anything we broke.

jrfnl commented 4 years ago

@Seldaek You're a star! Thank you so much for that.

I presume we should add (empty) deactivate() and uninstall() methods though ? Not that anything should happen in that case for this plugin. Removing a set installed_paths may break more than it fixes.

I've just pushed up a WIP branch which can be used for testing: https://github.com/Dealerdirect/phpcodesniffer-composer-installer/tree/WIP/feature/support-composer-2.0 https://github.com/Dealerdirect/phpcodesniffer-composer-installer/tree/feature/support-composer-2.0

Seldaek commented 4 years ago

Yup that looks good as a first step. I'd say if some basic testing doesn't surface any issues it's probably fine..

danepowell commented 4 years ago

Thanks, I've tested your WIP branch (using a local path repo) and confirmed that it works great with Composer 2. Let me know if you need anything else to get this into a new release.

jrfnl commented 4 years ago

@danepowell Thanks so much for testing this!

I kind of want to have a better read through the upgrade guide @Seldaek has posted since before opening a PR, but I hope to get to that some time this week.

danepowell commented 4 years ago

Hey @jrfnl I don't want to rush you, but any chance you could provide a new timeline for this? Composer 2 is expected to be released in the next few weeks, and downstream projects that depend on phpcodesniffer-composer-installer can't start their own upgrade testing until phpcodesniffer-composer-installer has the integration. Thanks!

jrfnl commented 4 years ago

@danepowell I'd be happy to promote the WIP branch to a PR, but would still really like to see some more people testing it and confirming all is ok for lack of integration tests.

Two people testing for just their scenario for a plugin with millions of installs seems a bit meager....

Seldaek commented 4 years ago

@jrfnl a PR would increase visibility and likeliness that people test too IMO, plus it makes it easier to give feedback.

jrfnl commented 4 years ago

@Seldaek I just ran some tests myself before opening the PR and am running into problems. Will open an issue upstream.

jrfnl commented 4 years ago

@Seldaek Grr... one of those... I can't seem to create a scenario to consistently reproduce the issue. Might have been a fluke. Would you still like me to report the issue with the stack trace or leave it ?

This was the error I got:

  [Composer\DependencyResolver\SolverBugException]
  Reached invalid decision id 0 while looking through (-230) for a literal present in the analyzed rule (-75|457|458|459|1139).
  This exception was most likely caused by a bug in Composer.
Seldaek commented 4 years ago

Unfortunately for this kind of errors if you don't have a sure way to reproduce it this info is kinda useless :/ it would be great to get a report with repro case though if you figure it out. .

jrfnl commented 4 years ago

@Seldaek I figured as much and haven't found a consistently reproducible scenario. If I do, I'll be sure to report it properly.

mxr576 commented 4 years ago

Any update on this?

jrfnl commented 4 years ago

@mxr576 There is a PR open for this: https://github.com/Dealerdirect/phpcodesniffer-composer-installer/pull/111 It would be great if you could test it and leave your feedback in the PR thread.