-
-
Notifications
You must be signed in to change notification settings - Fork 37
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
Drop support for Composer v1.x #230
Drop support for Composer v1.x #230
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @fredden Thanks for preparing this PR!
I've gone through the codebase with a fine toothcomb and found some more changes which I believe should be made for this PR:
CONTRIBUTING.md
line 102 still contains a reference to acomposer1.phar
file.BaseLineTest::testBaseLineLocal()
: I believe the test skip condition can be removed.RemovePluginTest
: I think thegetUninstallStdOutExpectation()
method can be removed and calls to it replaced with''
.TestCase::writeComposerJsonFile()
: The special casing to disable TLS on Windows with Composer 1.x can be removed.
I look forward to the next iteration of the PR.
@Potherca I wonder if we shouldn't just drop support for Composer < 2.2 (released Dec 2021 vs 2.0 released Oct 2020).
Composer 2.2 is a LTS release with support for PHP < 7.2 and still sees releases, so should definitely stay supported for now.
Also see: https://packagist.org/packages/composer/composer/stats#major/2
What do you think ?
README.md
Outdated
@@ -48,7 +48,7 @@ That's it. | |||
This plugin is compatible with: | |||
|
|||
- PHP **5.4+**, **7.x**, and **8.x** (Support for PHP v8 is available since [`v0.7.0`][v0.7]) | |||
- [Composer][composer] **1.x** and **2.x** (Support for Composer v2 is available since [`v0.7.0`][v0.7]) | |||
- [Composer][composer] **2.x** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to remove the historic context remark ?
Might it be more informative to keep it and add info about when support for Composer 1.x was dropped ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After I removed the "1.x and", the words in parenthesis no longer made sense. I considered rewriting this section into a table; would this be preferable?
Example table: https://github.com/fredden/magento2-module-javascript-error-reporting?tab=readme-ov-file#compatibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a table like this:
Plugin version | PHP | Composer | PHP_CodeSniffer |
---|---|---|---|
^0.1.0 |
* |
^1.0 |
* |
^0.2.0 |
* |
^1.0 |
* |
^0.3.0 |
* |
^1.0 |
* |
^0.4.0 |
* |
^1.0 |
* |
^0.5.0 |
^5.3 || ^7.0 |
^1.0 |
^2.0 || ^3.0 |
^0.6.0 |
^5.3 || ^7.0 |
^1.0 |
^2.0 || ^3.0 |
^0.7.0 |
>=5.3 |
^1.0 || ^2.0 |
^2.0 || ^3.0 |
^1.0 |
>=5.4 |
^1.0 || ^2.0 |
^2.0 || ^3.1 || ^4.0 |
^1.1 |
>=5.4 |
^2.0 |
^2.0 || ^3.1 || ^4.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think a table is needed. That's for Composer to sort out on install. Just some generic info should be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some generic info should be enough.
So, should we leave this as-is, or restore to the original, or do you want to supply some different text?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about something like:
(Support for Composer v2 is available since [
v0.7.0][v0.7]; Support for Composer < 2.0. was dropped in [
v1.1.0][v1.1])
@Potherca What do you think ?
I think that we can drop support for 2.0 and 2.1 as both (combined) have dropped under 1.5K daily downloads out of a +/-65K total, and there aren't many (any?) compelling reasons for people to not update to 2.2 (or higher). I think <2% is an acceptable cut-off point when perfectly good alternatives exists for the same PHP version(s). |
Well, the stats on Packagist are the package downloads. There's also the Phars, Phive and other installation methods, which I suspect in this case to be more prevalent and more relevant, but for which we don't have stats available. Having said that, I definitely support dropping support for Composer < 2.2. |
I did see this and chose to keep it. Would you prefer that this is removed?
I have applied these changes. Initially I chose to not make any PHP changes, but it makes sense to put the obvious ones in this pull request. |
5c1436d
to
0df962b
Compare
Why yes (or updating to a 2.x variant). as the sentence is about running the tests locally and that won't work with a
Curious why you initially chose not to make those PHP changes. Care to explain ? |
0df962b
to
7247a64
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these updates @fredden !
The PR now also includes the version drop for Composer 2.0 < 2.2, which means some additional changes are needed. Most have been made, but I've left some inline comments related to this + I think the InstallUpdateEventsTest::testPluginRunsOnInstallWithNoScripts()
test can get some clean up for the Composer 2.0 < 2.2 version drop.
@@ -28,13 +28,13 @@ | |||
}, | |||
"require": { | |||
"php": ">=5.4", | |||
"composer-plugin-api": "^1.0 || ^2.0", | |||
"composer-plugin-api": "^2.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record - as I had to double-check this myself:
The plugin API does follow the same major version as Composer has, but does NOT necessarily align to the same minors.
The current version of the composer-plugin-api
is 2.6.0
while the Composer package itself is at 2.8.4
. See:
https://github.com/composer/composer/blob/f5e7a8390d700d09c87ae2ed79d5aea1e4153cd3/src/Composer/Plugin/PluginInterface.php#L35
However, in this specific case, the versions happen to line up for the 2.2.0
version of Composer (so all good ✔️ ):
https://github.com/composer/composer/blob/e174a4c4324f50a6f2de472aa1055c24a2fe2b2a/src/Composer/Plugin/PluginInterface.php#L35
Composer themselves will no longer support version 1.x beyond 1st August 2025. Composer version 2.2 was released in December 2021, which is more than three years ago. Anyone wanting to use this plug-in with Composer <2.2 can use any existing version. https://blog.packagist.com/composer-2-0-is-now-available/ https://blog.packagist.com/deprecating-composer-1-support/ https://blog.packagist.com/shutting-down-packagist-org-support-for-composer-1-x/
7247a64
to
645f3fc
Compare
@@ -177,7 +177,7 @@ public function testPluginRunsOnReinstall() | |||
} | |||
|
|||
/** | |||
* Test that the plugin runs (or doesn't run) when Composer is invoked with the --no-scripts argument. | |||
* Test that the plugin runs when Composer is invoked with the --no-scripts argument. | |||
* | |||
* Note: the behaviour of Composer changed in 2.1.2. Prior to that, `--no-scripts` would |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this "Note" can now be removed as well (as it's no longer relevant).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept this because it explains why the test exists. Should we instead drop the whole test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood and no, let's keep the test as it has value either way.
FYI: I've updated the branch protection to be in line with this change. @Potherca Do we want to tag a release for this already ? Or should we include the "drop support for PHPCS 2.x" task into the same minor ? |
I'm in favor of keeping this as part of 1.1 |
@Potherca Sorry, there appears to be confusion. Let me rephrase the question a little: Do we want to tag the |
I don't really have a rationale but my feeling/instinct is to split the releases, 1.1 now and 1.2 with the Composer <2.2 changes. |
@Potherca Same intuition I had, which is why I asked the question ;-) Though to be clear, this would mean:
Agreed ? How do you want to do the release ? Quick videocall with the two of us some time next week ? |
Proposed Changes
Composer themselves will no longer support version 1.x beyond 1st August 2025. Composer version 2.0 was released in October 2020, which is more than four years ago. Anyone wanting to use this plug-in with Composer v1.x can use any existing version.
https://blog.packagist.com/composer-2-0-is-now-available/
https://blog.packagist.com/deprecating-composer-1-support/
https://blog.packagist.com/shutting-down-packagist-org-support-for-composer-1-x/
Related Issues