Problem/Motivation
Composer 2 is on its way, and may provide benefits to Drupal sites. It should be possible to use Composer 2 to run commands such as require, update and install on Drupal sites.
https://github.com/composer/composer/blob/master/CHANGELOG.md#200--2020-
Proposed resolution
- Declare that our plugins work with the ^2 Composer API
- Implement the new methods of the Composer 2.x plugin interface
- Upgrade to composer/installers ^1.9 (earlier versions are not compatible with the Composer 2 plugin API)
- Remove wikimedia/composer-merge plugin
- Call
OperationInterface:: getOperationType()
instead ofOperationInterface::getJobType()
(renamed in Compsoer 2) - Skip tests that will not work with Composer 2 until there is a stable release of our Composer plugins that are compatible with Composer 2.
Note that Drupal also has Composer dependencies internally, including composer/composer (require-dev only) and composer/semver (required in drupal/core). Updating these components cannot be done until there is a stable release available; therefore, updating these components is out of scope for this issue. See below.
Remaining tasks
Change logCreate follow-on issue to enable the scaffold upgrade test for Composer 2 once there is a stable (tagged) release of drupal/core-composer-scaffold that can be installed with Composer 2.#3128641: Enable scaffold upgrade test for Composer 2Create follow-on issue to upgrade internal Composer components used by Drupal, composer/composer and composer/semver, to ^2 once stable versions are available. Also re-enable the scaffold upgrade test for Composer 2 once this is done.#3128631: Update dependencies composer/composer ^2 and composer/semver to ^3Follow-on issue for drupa/coder issue#2851279: Update version requirements for drupal/coder to best practicesDetermine how to detect newly-installed packages aftercomposer require
in Composer 2.Roll back the upgrade of composer/composer and composer/semver (and additional upgrades from --with-dependencies), and keep them what they were prior to this patch. Retain upgrade of composer/installers, though.Release notes snippet
User interface changes
None
API changes
Drupal must react to some Composer API changes, as mentioned above; however, no APIs defined by Drupal change.
Data model changes
None
Release notes snippet
The Composer plugins provided by Drupal -- the scaffold plugin, the vendor hardening plugin, and the project message plugin -- are now compatible with Composer 2, and Drupal core now allows using either Composer 1 or 2.
Comment | File | Size | Author |
---|---|---|---|
#103 | 3126566-8.9.x-103.patch | 8.6 KB | greg.1.anderson |
#103 | 101-to-103-interdiff.txt | 2.74 KB | greg.1.anderson |
#101 | 3126566-8.9.x-101.patch | 8.97 KB | greg.1.anderson |
#101 | 96-to-101-interdiff.txt | 581 bytes | greg.1.anderson |
#96 | 3126566-96.patch | 9.54 KB | greg.1.anderson |
Comments
Comment #2
andypostIn related it was reported that psr-4 checks are more strict
Comment #3
heddnNot directly related, but linking in anyway: https://github.com/composer/composer/pull/8655
Comment #4
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedJust bumping our Composer plugin compatibility to allow for either ^1 or ^2 seems to mostly do the trick for making our plugins work with Composer 2. It is also necessary to implement the two new methods of the plugin interface. With these changes, our plugins work under ad-hoc testing using the latest snapshot of Composer 2. Note that the test bot runs the tests currently run with Composer 1.x. When I install Composer 2.x on my local machine and run the scaffold plugin tests, then two of the scaffold tests fail. I haven't isolated why yet, but it looks like it has something to do with the techniques used to set up the test fixtures rather than actual bugs in the scaffold plugin.
Note also that the drupal/drupal project requires composer/composer ^1 for testing. I haven't yet run the tests with Composer 2.x in the lock file, nor have I ran the full test suite. We should discuss how we would like to do testing for Composer 1 and Composer 2, e.g. should we always use Composer 1 with Drupal 8.9.x, and always use Composer 2 for Drupal 9.0.x, or should we run tests using both Composer 1 and Composer 2 with Drupal 9.0.x, etc.
As mentioned in #2, additional required changes might be discovered once we increase the breadth of the Composer 2 testing.
Comment #5
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedWhen attempting to install the Core Scaffold plugin test fixtures with Composer 2, the following errors are reported:
This is likely a bug in Composer 2 path repositories, although I have not isolated the cause.
Comment #6
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere's a patch with drupal/drupal's internal composer/composer dependency bumped to dev-master. Also upped composer/semver to 2.x, because that is required by Composer 2.x. This is just to see the failures caused by the path repository thing; maybe this will expose some of the issues hinted at in #2.
Built via
composer update composer/composer --with-dependencies
, so there are a couple of updates that might not be strictly necessary. composer.lock omitted from interdiff.Comment #8
catch#2951375: Allow for build targets is open for core min/max dependency testing.
Drupal 8.9 where it's depending on composer/composer should continue to specify Composer 1, so if 9.x switches to composer 2 then that is probably good enough for min/max until we have something proper sorted out?
Comment #9
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThe wikimedia/composer-merge-plugin is not working with Composer 2 yet. Since we deprecated this plugin in Drupal 8.8.0, we should just remove it in Drupal 9. It is only present in drupal/drupal (core development); it is not used, but its presence causes warnings.
I create #3127013: Remove wikimedia/composer-merge-plugin to track.
Comment #10
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedLet's try to monkey-patch Composer 2 into the test bot.
Comment #12
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented#10 revealed that we also need to upgrade composer/installers in order to use Composer 2. Support for Composer 2 is already available in composer/installers 1.9.0.
Also jumping in and removing wikimedia/composer-merge-plugin in this patch, for the same reason. It would still be best to get #3127013: Remove wikimedia/composer-merge-plugin committed in advance of this issue.
Comment #13
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedSetting status to run the tests.
Comment #15
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThis patch might test green, although I disabled one feature and two tests in order to get here. See "remaining tasks", above.
Comment #16
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThe main purpose of #15 is to run the tests on the interim patch. :P Setting status so that happens.
Comment #17
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI requested a stable composer/semver 2.x release, because that would be somewhat convenient to have for Composer 2 testing.
Comment #18
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere's a patch that restores the Core Composer Scaffold feature removed in #15, updated to work with either Composer 1 or Composer 2.
Comment #19
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedComment #20
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented#18 proves that our changes to our Composer plugins are sufficient to make them compatible with Composer 2, but we obviously do not want to start using Composer 2.0@dev or composer/semver 2.0@dev yet.
Here's a patch that keeps all of the Composer compatibility changes needed from #18, including the version bump of composer/installers, but otherwise does not increase the version of any internal component. Composer 1.x is back in the lock file, and most tests are run with Composer 1.x. At the end, I bump Composer up to the 2.x dev release, and then run the Composer plugin tests again (n.b Composer 1.x is still in the lock file for these tests, but this works; the only code mixing between Composer 2 and Drupal AFAIK is in the functional tests when the Composer tool is used to run the Drupal Composer plugins, and these avoid using APIs other than Composer's).
I made the dubious decision to revert to Composer 1 testing AND switch to the 9.0.x branch in the same patch, so I have no interdiff from #18. I'll go back and make an 8.9.x version of this later so that I can rectify that.
Comment #21
jungleShould use
!== FALSE
, not!== false
.Comment #22
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedMy custom CI commands were not right, and apparently were causing the test to abort and re-run over and over again. Trying a different way. Also fixed style issues from #21.
Comment #23
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedComment #24
catchDoing the compatibility changes without bumping the version sounds good here - we can have a tracking issue for the actual update (with a patch to run tests against too). What do you mean by 'most tests' though?
Comment #25
jungleGot some deprecation notices Composer v2.0 relevant, 3116414 is an issue of coder, which has similar warnings too.
Comment #26
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented#24: Good idea, I'll do that.
#25: Nothing we can do about that until we update those components. I'll add that to the follow-on issue. Since the follow-on issue already entails updating composer/composer and composer/semver, it makes sense to update the other dependencies with deprecations at the same time.
Comment #27
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedSorry, I was editing my comment as I was working on my strategy for the patch. That sentence was a fragment that didn't get fixed up.
In #22, all tests are run on Composer 1.x. At the end of the test run, #22 runs the Composer plugin tests again using the Composer 2 cli. This means that the functional tests run with Composer 2.x; however, most of the other tests in this part of the run (the unit tests) still run against the Composer 1 APIs from our vendor directory, because I didn't try to do an on-the-fly upgrade of composer/composer and composer/semver in the tests. Doing that seemed dubious and fragile, so I thought it best to skip it at this stage, and just rely on the all-Composer-2-tests in #18 until the follow-on issue.
Comment #28
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThis patch does a little more than just plugin compatibility, so my last title update isn't very accurate. Trying again.
Comment #29
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere is a cleaned-up version of the 8.9.x patch. The patch from #22 is also re-uploaded here without change simply to avoid any confusion over which patches are most recent.
An interdiff with #18 is also provided; this shows the roll-back from using Composer 2 dependencies internally to this patch, which uses Composer 1 dependencies internally + changes to be compatible with Composer 2.
Finally, I discovered one more situation that arises with Composer 2 and Drupal 8/9:
It seems that what is happening here is that Composer 1 is happy to dismiss incompatible constraints from higher-priority repository and instead satisfy the request from a lower-priority repository, but Composer 2 considers this an error. I'm not sure if we do or do not have a good workaround if we have to adapt to this behavior.
Comment #30
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedGood news: the drupal/coder problem reported in #29 only occurs in the drupal/drupal repository (core development).
I created a fork of drupal/recommended-project and its immediate dependencies, and applied the patch from #29. To reproduce my ad-hoc test:
I won't reproduce all of the output, but in the test run above, drupal/coder installed without incident.
Comment #31
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedCreated follow-on issue #3128631: Update dependencies composer/composer ^2 and composer/semver to ^3.
Comment #32
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedFixing bad edit in #31.
Comment #33
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedComment #34
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI should also mention that now that #3127013: Remove wikimedia/composer-merge-plugin has been merged to the 8.9.x branch, the 8.9.x patch on #29 needs a re-role. The 9.0.x patch on #29 was done after wikimedia composer merge was removed from the 9.0.x branch, though, and therefore is still fine.
I'll re-roll the 8.9.x patch after the 9.0.x is committed.
Comment #35
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedWe can fix the drupal/coder problem described in #29 if we switch to installing it from Drupal Packagist instead of Packagist. To do this, just switch to the version scheme in Drupal Packagist:
COMPOSER_ROOT_VERSION=8.9.x-dev composer require --dev drupal/coder:^3.7
I'm not clear on the difference between the drupal/coder in Packagist vs the drupal/coder on drupal.org. The version on drupal.org appears to be newer (has a 3.8.0 release that isn't on Packagist yet), so perhaps this solution is preferable, even?
If folks think this is a good solution, I'll update the patches to use the other repository to fetch drupal/coder.
UPDATE As pointed out in the issues linked below, installing drupal/coder from Drupal Packagist does not work, because doing so causes it to be installed as a module, to modules/contrib. We install from Packagist so that it will be installed to the vendor directory instead.
Comment #36
drumm#2851279: Update version requirements for drupal/coder to best practices is the issue for installing drupal/coder from Drupal Packagist.
Comment #37
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedAlso:
Code in Packagist endpoint responsible for mapping projects to types.
Comment #38
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI think that #2851279: Update version requirements for drupal/coder to best practices is a good follow-on issue to track the drupal/coder problem. I commented on that issue to note that it is blocking Composer 2 use for Drupal core development. We won't be able to do that until there's a stable release of composer/semver (and maybe composer/composer as well), so we needn't let that issue block us here.
Comment #39
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedComment #40
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedSemver 2.0.0 stable is out! This opens the door for us to bump up to `composer/semver: ^1|^2` in #3128631: Update dependencies composer/composer ^2 and composer/semver to ^3. We might want to decouple the composer/semver update from the composer/composer update.
Comment #41
Dane Powell CreditAttribution: Dane Powell at Acquia commented#29 seems to work well with both Composer 1 and 2, and the code looks good based on what I know about the Composer 1/2 upgrade process.
My interest is to get this into core ASAP so that downstream projects depending on core-composer-scaffold can also start work on the upgrade process: https://github.com/acquia/blt/pull/4088
Comment #42
alexpottThe 9.0.x patch doesn't apply anymore.
Comment #43
alexpottThis is super smart. I wish we could control the test matrix in the project though and not run this on postgres etc...
Comment #44
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented#43: Maybe we should take out Composer 2 testing for now, and tackle testing as a follow-on issues.
Comment #45
xjmComment #46
alexpott@greg.1.anderson I think it's good to leave it in. That way we can be sure we don't break it. My comment was to compliment the idea and to remark on functionality that would be useful but not in-scope here.
Comment #47
tedbowOk here is reroll for 9.0.x
Comment #48
tedbowComment #49
tedbowcouple nit fixes and a question
Should use
===
Both need comments
We should type hint OperationInterface here.
Won't the $operation always have 1 or the other of the methods? Just depends on the version of composer? Shouldn't we throw exception instead of returning ''?
Comment #50
jungleReplaced
strstr()
withstrpos()
Comment #51
jungleBTW. filed a new issue to replace strstr() with strpos()
Comment #52
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented+1 on 1. through 4. in #49.
Comment #53
tedbowChatted with @greg.1.anderson. This does #49.4
I did the other things in the patch in #49
Comment #54
xjmThanks @tedbow and @greg.1.anderson. Having Composer 2 compatibility is very important for Drupal's future.
A couple questions:
AllowedPackages
affect anything aside from enabling Composer 2 packages to be installed?Comment #55
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented1. The change in
AllowedPackages
allows Drupal core to work when composer/composer:^2 is required for Core development, while also allowing composer/composer:^1 to continue working.2. The behavior of AllowedPackages should remain completely unchanged regardless of the version of Composer being used; there shouldn't be any edge cases or side effects from that code.
I did quite a bit of ad-hoc testing and tried to make the tests cover the important behaviors, but it's always possible that something unexpected could come up. I'm pretty confident with what we have here, though.
I am +1 on #53, but can't RTBC. I'll try to find time to write up the change record later this afternoon.
Comment #56
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI created a change record for this issue, and updated the release notes snippet to match the wording used in the CR.
Comment #57
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedComment #58
kim.pepperUbernit: 'implemented' should be 'implement'.
Comment #59
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedFIx for #58.
Comment #60
alexpottIt would nice to comment which one is composer 1 and which is composer 2. It looks like we're doing Composer 1 call first. Which probably makes sense now but won't once everyone has Composer 2.
Also I think triggering a specific exception here is unnecessary - PHP will thrown an exception if the method doesn't exist and it's not something we're going to catch and do something with. So something like:
Other than that this is looking good to go to me for 9.x to me.
I also think we should consider rolling this patch for 8.9.x so we have a Drupal 8 compatible with COmposer 2 too.
Comment #61
jungleAddressing #60, rerolling a patch for 8.9.x next.
Comment #62
jungleA patch for 8.9.x,
Followed similar steps here (#3122112) making sure the package got updated against config.platform.php: 7.0.8.
Comment #64
jungleA patch for 8.9.x again, updating the composer-plugin-api version to see if the testing passes.
For 9.0.x testing failed in #61, probably, it's a random failure.
Comment #65
Kristen PolAssigning to myself to review.
Comment #66
Kristen PolComment #67
Kristen PolThanks for the patches.
1) Compared D8 vs D9 patches and not sure why these are included in D8 patch. They are not used anywhere in the codebase.
https://github.com/mantisbt/mantisbt
https://github.com/Sylius/Sylius
2) Patches applied cleanly
3) Reviewed the changes requested in #60 are in the patches and the changes are identical.
4) I see that @greg.1.anderson has some commands that were manually tested but I'm not sure the best way to test this myself.
Comment #68
Kristen PolI noticed another thing when checking the code for
composer/installers
,composer/composer
, andcomposer-plugin-api
after applying the patches:drupal-8.9.x-dev
composer/installers
composer/composer
composer-plugin-api
drupal-9.0.x-dev
anddrupal-9.1.x-dev
composer/installers
composer/composer
composer-plugin-api
Do any of the dissimilar versions need to be changed?
Comment #69
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThanks for the review, @Kristen Pol.
#67.1: Not sure why these are here. I'll look into it later.
composer why
will shed some insight.#67.4: The tests should cover functionality pretty well, but if others want to do more ad-hoc testing, that certainly wouldn't hurt.
#68.1: composer/installers: Should probably be ^1.9 everywhere, for the avoidance of doubt.
#68.2: composer-plugin-api: There's no need to bump the minimum version of the plugin api to 1.1 where it was previously 1.0. Doing so wouldn't hurt, though.
#68.3: Not sure why PinnedDevDependencies didn't get regenerated. Maybe
composer update --lock
was omitted for this patch? I wouldn't think so. I'll need to investigate more late.#68.4: It would be consistent to make the Scaffold plugin require composer/composer ^1.9 instead of ^1.8@stable, but this is not necessary.
I appreciate all the cross-checking you did.
Comment #70
jungle@Kristen Pol, many thanks for your detailed review, always!
#67.1, nothing to do to me. sylius and MantisBT exist in composer.lock of both two patches.
See vendor/composer/installers/src/Composer/Installers/Installer.php
BTW.
Composer version 1.9.0 2019-08-02 20:55:32
used to reroll the patch for 8.9.x#68, to me, it's acceptable to have different versions in different branches. The min PHP version required in 8.x is 7.0.8, while it's 7.3.0 in 9.1.x. So I am setting back to NR.
Comment #71
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented#68.3, the versions in PinnedDevDependencies, are cause to put this back to
needs work
if they are in fact inconsistent with what is in composer.lock.Not putting this back to needs work myself just yet because I don't have a minute to verify that adjustment is needed. Running
COMPOSER_ROOT_VERSION=8.9.x-dev composer update --lock
is probably sufficient to fix this problem, if it exists.Comment #72
hussainwebIn the D9 patch in #61, I noticed the composer.lock was not updated for projects in the path repositories. I updated it but my composer version added a bunch of additional values.
These were the changes I was looking for:
Particularly, lines 711 and 741 were not updated in composer.lock. They were in sections for `drupal/core-project-message` and `drupal/core-vendor-hardening`. They are now updated in the new patch.
Comment #73
hussainwebFrom #68, @Kristen Pol, I think that none of those versions contradicts each other and that's fine. (If I'm understanding your comment correctly.)
I don't think that reflects what's in composer.lock. It's still worth making the versions consistent but maybe in a separate issue?
Comment #74
hussainwebI was doing more digging related to #68 and I thought I should clarify that I am only commenting on 9.0.x branch at the moment. Also, one of the versions for composer/installers is ^1.2, but that's in the README.md file. It makes sense to update it in this issue itself.
Comment #75
hussainwebOh, one more thing. Re #71, @greg.1.anderson, you're right that the version mismatch in PinnedDevDependencies would be cause to set to "needs work", but PinnedDevDependencies is not in the patch (for 9.0.x at least). It's already set to 1.10.5 in HEAD of the branch.
Sorry for the repeat posts. I think this is all for now. :)
Comment #76
Kristen PolThanks. I see a lot of changes in the lock file so I'm going to hold off reviewing at least until @greg.1.anderson weighs in more on this.
Comment #78
hussainwebThis is the failure.
I am not sure what to think of this because the actual hash in composer.lock is `bf3f5b1c0d2f35255e03825589a1ee4f`. Also, running `composer validate` locally reports no errors.
Comment #79
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented#78: @hussainweb: Downgrade your version of Composer to 1.9.x before running composer update on Drupal core. I think there's an issue for this somewhere.
Comment #80
hussainwebThanks @greg.1.anderson. I'll try that. Although, composer validate on 1.9.0 works fine.
Comment #81
jungleThanks @hussainweb!
Re #78
Funding info were added, which introduced in composer 1.10. So you need to fallback to composer 1.9 to avoid adding funding info.
See #3127918: Add funding info in composer.lock for more
Comment #82
jungleIn 8.9.x patch, I updated the composer-plugin-api versions to allow coexistence with composer 2 manually, which should be automatically generated probably.
Comment #83
hussainwebThanks @jungle, I ran this now on composer 1.9.0 and the hash does look different. Here it goes.
Comment #84
hussainweb#82, @jungle: I am not sure which comment you're responding to but if you're talking about the first patch I posted here, that was for 9.0.x. I haven't seen the 8.9.x patch yet. Am I missing anything here?
Comment #85
jungleRe @hussainweb, #82 is just FYI :)
Comment #86
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedOK, I looked at this a bit more closely.
The dev dependencies copies whatever is in Drupal's composer.json file, recording the current constraint for each dependency. The version constraint for composer/composer in the 9.0.x branch is currently ^1.9.1, and this patch makes no changes to this value, so this is correct.
The pinned dev dependencies records the exact version of each dependency locked in Drupal's composer.lock file. Note that it is expected and common for this version to be later than the version constraint in the composer.lock file. Drupal has Composer 1.10.5 in the composer.lock file, and this patch does not change anything in this area, so this is also correct.
I also tried running
COMPOSER_ROOT_VERSION=9.0.x-dev composer update --lock
on #61, and found that no changes were made to the composer.lock file, so #72 and #83 are not necessary.As previously mentioned, #67.1 is part of the update to composer/installers done in this patch. Composer copies part of every dependency's composer.json file into the composer.lock file of any application that uses it. Sylus and MantisDB are mentioned in the "keywords" of the new version of composer/installers.
The optional version consistency changes could be made as noted in #69, if desired, or we can just continue with #83 / #61 (these should be the same, I think) and #64 (for 8.9.x).
Comment #87
hussainwebThanks @greg.1.anderson, I am not sure if I understand. However, I still think that the changes in #72 (and hence #83) are necessary. IIRC, I got the changes just by running `composer install`. I'm trying to check my history to see if I ran something else but after that, the lock file was updated with the correct constraints for composer-plugin-api under `drupal/core-project-message` and `drupal/core-vendor-hardening`.
Comment #88
Kristen Pol@hussainweb It would be easier to understand the changes in #83 if the interdiff was against #61. It's hard to read with all the
funding
stuff.Comment #89
hussainweb@Kristen Pol, I reran the command I used earlier but now on composer 1.9 and got this diff. As you can see, it updates the locked version references for composer-plugin-api.
This is the command:
docker run --rm -v {$PWD}:/app -w /app composer:1.9.0 require composer/installers:^1.9 --ignore-platform-reqs
It's functionally same as
composer require composer/installers:^1.9
if you have composer 1.9.0 installed locally.Comment #90
Kristen PolThanks @hussainweb. That's much easier to understand.
Comment #91
alexpottHere's the diff between #61 and #82
That change looks acceptable to me. Note that reviewing the composer.lock is pretty fraught. In an ideal world we wouldn't ship it but that would require us to min/max dependency test and involve all sorts of complexities around releases so that's unlikely to happen in any sort of speed.
I've taken the latest patch #83 and run
composer update --lock
with composer 1.9.2 and no changes are made so the lock file is great. More importantly all the code changes are good and we are testing core's composer plugins and meta packages using composer 2 so that's great.The Drupal 9 patch is therefore rtbc.
Doing the same test on the Drupal 8 patch results in changes to the lock file so #64 is not rtbc but we can fix that once this has landed in Drupal 9.
Comment #93
catchBit worried this might lead to testbot instability prior to composer 2 becoming stable. However we can address that if it happens (i.e. a new issue to remove this hunk and put it into a tracking patch or similar). OK with adding it for now.
Committed/pushed to 9.1.x and cherry-picked to 9.0.x.
Moving back to 8.9.x for backport. I think we want to update our composer plugins so they're composer 2 compatible, but keep 8.9's composer dependencies as they are.
Comment #95
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThanks, all. #89 showed that the composer.lock changes described in #87 were necessary.
This seems reasonable. If we do this, then when using Drupal 8.9.x, it would be possible to use Composer 2 with drupal/recommended-project, but not with drupal/pinned-dev-dependencies, and not with Drupal core development. Using Composer 2 with Drupal core is dependent upon #3128631: Update dependencies composer/composer ^2 and composer/semver to ^3 anyway, though, so this is fine.
Comment #96
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere's a backport to 8.9.x. This is essentially #64 with the composer/installers update omitted, and other composer.lock changes applied.
Comment #97
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedYeah, it is not great to have the testbot running with every dev commit of Composer 2 that gets pushed. I originally included this just to demonstrate that it works, but it is also good to have some assurance that future changes to our plugins do not break under Composer 2.
I'll make a follow-on issue to at least pin to a specific version of Composer 2.
Comment #98
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedCreated follow-on issue #3133423: Test with a specific dev release of Composer 2
Comment #99
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedShould we remove the Composer 2 re-test from #96? Anything committed to 8.9.x should at this point have already been tested on the 9.0.x branch, which includes Composer 2 testing. Perhaps maintaining this test on the Composer dev release is not necessary here. It would be less to track in #3133423: Test with a specific dev release of Composer 2 to omit it here.
Comment #100
alexpott#99 sounds okay. We might choose to add composer 2 testing to 8.9.x once composer 2 is stable but I agree that having less to tracking is nice.
Comment #101
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedUpdating #96 to eliminate Composer 2 testing per #100
Comment #102
longwave#82 has this:
#101 has this:
I think we should be consistent, I guess by using
^2
in the 8.9.x patch.Comment #103
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedOK,
^2
and^2.0.0
are semantically identical, but in the interest of minimizing diffs between the 8.9.x and 9.0.x branches, here's an update to the patch.Comment #104
longwaveDiff between the 8.9.x and 9.0.x patches is as small as can be (including the change from #99), so this is good to go.
Comment #106
catchCommitted 53e445b and pushed to 8.9.x. Thanks!
Comment #108
catchReverting this due to #3134648: [backport, needs scheduling] Don't pin the composer/installers version in drupal/core-recommended. From discussion on that issue it looks like we could recommit this with a pinned dependency on composer-installers 1.7 to workaround the composer update bug.
Comment #109
hussainwebI might be missing something here. The patch for 8.9 didn't change the version at all. It happened in #3122112: Update dependencies for Drupal 8.9. Is there a need to revert this patch as well?
Comment #110
alexpottOne option here is to recommit #103 as it does not upgrade composer/installers and then handle the composer/installers update in #3134648: [backport, needs scheduling] Don't pin the composer/installers version in drupal/core-recommended
I've kicked off a test run.
Comment #111
alexpottDoing #110 would mean that drupal/core-recommended is not composer2 compatible for now. But as @greg.1.anderson has pointed out you can
Comment #112
catch@hussainweb yeah I got a bit confused this morning and was over-zealous. I think we can probably land #3122112: Update dependencies for Drupal 8.9 again, then re-commit #103 as-is.
Comment #113
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedYou're right, it looks like we decided not to upgrade dependencies for the 8.9.x version of this patch. Yay.
+1 on re-committing #103.
Comment #114
xjmThis probably needs to be postponed on that, because otherwise we're back in the same situation?
Comment #115
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedWe believe that this patch will have no effect on our erstwhile Composer bug; however, it's hard to prove until we commit to 8.9.x-dev, at which point we've already inconvenienced a bunch of people if we get it wrong.
Postponing this on #3134648: [backport, needs scheduling] Don't pin the composer/installers version in drupal/core-recommended is therefore safer, and couldn't hurt.
Comment #116
alexpott@greg.1.anderson I'm not sure that you're right about #3134648: [backport, needs scheduling] Don't pin the composer/installers version in drupal/core-recommended being safer. This doesn't touch the drupal/core-recommended meta-package so I think it is totally safe to commit. The other one is much more tricky. In fact given the timings I think are necessary I think we need to commit this to 8.9.x sooner rather than later.
Also for what it is worth we know this didn't cause the break for composer update on 8.8 because when catch revert this it was not fixed. It took the revert of #3122112: Update dependencies for Drupal 8.9 to fix the problem because that's the commit that made the 8.9.x drupal/core-recommended meta-package compatible with composer/installers 1.9.0 - and thus reverting it stopped composer update on 8.8 oddly selecting to update to 8.9.x-dev.
Comment #117
catchSo for clarity, this issue was fine and not responsible for breaking HEAD, it just went in at the same time as #3122112: Update dependencies for Drupal 8.9, which did break HEAD. I reverted it out of caution. Have now undone the revert since the general dependency update is fixed.
Comment #119
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedConfirmed that Composer is not upgrading prefer-stable sites to 8.9.x-dev this morning.
Comment #120
xjmComment #121
xjmComment #123
rgpublicUmm, is this really fixed? I'm using Drupal 9.0.2. When I run "composer update" with Composer 2 I get a fatal error in Composer::vendorTestCodeCleanup:
Fatal error: Uncaught Error: Call to undefined method Composer\DependencyResolver\Operation\UpdateOperation::getJobType() in /var/www/projects/aufsichtsrat-handbuch.de/htdocs/core/lib/Drupal/Core/Composer/Composer.php:189
I open that file and see in line 189:
if ($op->getJobType() == 'update')
But in the description of this issue it says:
Call OperationInterface:: getOperationType() instead of OperationInterface::getJobType() (renamed in Compsoer 2)
Now, I'm confused. Is this fixed? Am I doing sth. wrong?
Comment #124
rgpublicAh. Sorry for the spam. Just as I wrote this, I discovered the problem. The "scripts" section of my composer.json file causes these methods to be run. I removed the scripts section altogether as I doesn't seem to be in a fresh installation of drupal/legacy-project. Perhaps this useful for others and IMHO certainly sth. to be aware of. If you have older projects, you might want to check your composer.json script section if you still encounter problem with Composer 2.
Comment #125
maxilein CreditAttribution: maxilein commented#124 helped! Thanks