[backport, needs scheduling] Don't pin the composer/installers version in drupal/core-recommended

So … what to do if this already happened on a production site? Yesterday we only pushed two CSS files to the server without running composer update locally before (shame on me). The rollout did its job, though, leaving us with core 8.9.x-dev on the live site.

Because there were db updates, I think I could probably not come back to 8.8.5 without a db backup, right?
system module : 8901 - Update the stored schema data for entity identifier fields.
action module : Removes action settings.
views module : Update field names for multi-value base fields

What do you suggest? Thanks in advance.

You shouldn't try to downgrade a database that's already updated, so you have a choice of restoring a backup or staying on 8.9. The release candidate for 8.9.x will be released next week so if you're not able to restore a backup, you're at least on something that's about-to-be-stable.

In general I would not recommend running composer update as part of a deployment process at all - this should be part of the development rather than deployment process.

Sounds good, thanks for the quick answer. That was indeed what I intended to do, since the last db backup was written a few hours before the rollout … Will stay with 8.9 for now.

Regarding the rollout process itself – because I read a lot of different opinions about this:
Do you say I should better run composer update only locally and therefore also commit the "/vendor" directory and "composer.lock" to git?

Thanks again, Anton

@ABaier on a project you don't have to commit vendor. Committing the lock file should be enough.

I've re-opened #3126566: Allow Drupal to work with Composer 2. Assuming we're able to recommit with a 1.7 pinned dependency, we can repurpose this to tracking the upstream composer bug and/or finding another way to allow 1.9

@ABaier: Check out the Update core via Composer page for more details.

Thanks for clarifying @alexpott & @ressa and sorry for disturbing the context! As I understood correctly I should only run 'composer install --no-dev' on production and commit composer.lock to our git … instead of running 'composer update' on production.

Will change our rollout to this:

git clone/pull
[REMOVE: composer update] composer install --no-dev
drush -y updatedb
drush -y cim
drush cr
drush cron

Thanks for sharing your new rollout @ABaier, that looks like a better and safer process! Yesterday I watched Composer 101 by Michael Miles. It was a great refresher for me, about the basics of Composer. This is my process:

Update Drupal and modules locally

https://www.drupal.org/docs/8/update/update-core-via-composer
https://www.drupal.org/docs/8/update/update-modules

# check for available updates
composer outdated drupal/*
# update everything, core and modules, add --dry-run to test first
# to update only core, add drupal/core, or a module add drupal/modulename
composer update --with-dependencies -v
drush updatedb
drush cache:rebuild
drush config:status
drush config:export --diff
# check the changes, excluding composer.lock
git diff -- . ':(exclude)composer.lock'

Put in production on server

Note: Always run DB updates before importing configuration.

# change in to Drupal directory, then get the latest code
git pull
# add --dry-run, if you just want to see which packages will be installed
composer install --no-dev -v
drush updatedb
drush config:import --diff
drush core:cron
drush cache:rebuild

Re-titling to be more specific. Drupal 8.9.x can work with composer installers v1.9.0 but the root composer.lock file is at v.1.7.0 and therefore thats the version that's used in composer/Metapackage/CoreRecommended/composer.json.

Since there is a buy-in for the decision to remove composer/installers (and it makes sense to me too), here it goes.

Here are patches for 9.x.x. branches.

There's something wrong with the 9.0.x patch. There are a lot of unrelated changes in it. Possibly other changes got in there?

Forgot to update my 9.0.x HEAD whoops...

Here's a fixed patch - also we can unit test this.

Rerolled 8.9.x now that #3122112: Update dependencies for Drupal 8.9 landed.

I have a concern. I think that the patches here will solve the problem but I think we need to do this in 8.8.x too and make sure that it's committed there first and that 8.8.6 is released before 8.9.0. The reason we need to make this change is so that when composer tries to solve for a project built off the drupal/recommended-project project the composer.json looks like this:

"composer/installers": "^1.2",
"drupal/core-composer-scaffold": "^8.8",
"drupal/core-project-message": "^8.8",
"drupal/core-recommended": "^8.8"

The most recent composer/installers version is 1.9. The bug in composer meant that when we upgrade 8.9.x to support 1.9 composer determined that therefore we need to upgrade to 8.9.x-dev for drupal/core-recommended. Therefore if we commit the above patch the same is going to happen because the 8.9.x-dev version of drupal/core-recommended will allow 1.9 again but the 8.8.5 version does not. I think this means we need an 8.8.6 version that does.

Here's a patch for 8.8.x.

@greg.1.anderson re #23. I'm not sure about that. I think the moment we commit this to 8.8.x then the 8.8.x-dev release will become compatible with composer/installers 1.9.0 and therefore update to it. Therefore, I think we need to commit this to 8.8.x and tag a release asap. And also not commit this to 8.9.x until there is a tagged 8.8 release with this commit in.

It can be committed to 9.x whenever because the drupal/core constraints in core-recommended don't allow for update to 9.

Thanks for the additional test in #26, that coverage looks helpful.

There are a handful of CS issues and doc fixes needed. Instead of a nit-picky review, I'm attaching a cleaned up version of it.

Also attaching just #22 and #26 applied, and an interdiff between that and my version of #26.

Sorry, hadn't seen the unused use results when I uploaded #28. This is better.

Whoops, missed one more:

+++ b/core/tests/Drupal/Tests/Composer/Generator/OverlapWithTopLevelDependenciesTest.php
@@ -0,0 +1,50 @@
+      $this->assertArrayNotHasKey($project, $top_level_composer_json['require'], "Pinned project $project is also a top-level dependnecy of $template_project_path. This can expose a Composer bug. See https://www.drupal.org/project/drupal/issues/3134648 and https://github.com/composer/composer/issues/8882");

s/dependnecy/dependency/

Adding a bit about #24 to the title because otherwise we might forget and commit it the normal way. It's not clear to me whether #25 is validating that suggestion or not since it references this issue as if it were a different issue? Confirmation would be good. Thanks!

Also, we shouldn't assume people are on their 8.8.5 now or that they'll update to 8.8.6 when it comes out. I guess the idea is that we're trying to change the behavior of Packagist by ensuring the 8.8.6 tag exists with this change, which should benefit all sites.

What about 8.7? Does the fact that 8.8.0 - 8.8.5 exist prevent 8.7 sites from being affected by thisw?

Re 8.7.x drupal/core-recommended does not exist there so we don't have this problem fortunately - otherwise we would have hit when we updated 8.8.x to use composer/installers 1.7 (8.7.x is on 1.6).

I disagree that this test in #19

only ensures that composer/installers is being pinned at the expected sha.

It's testing that composer installers is excluded from the core-recommended build which is exactly what we want to test. The sha and everything doesn't actually need to be updated. This is a unit test of the \Drupal\Composer\Generator\Builder\DrupalCoreRecommendedBuilder class which imo is exactly what we want to do.

TIL run-tests.sh runs unit tests from inside the core directory.

The test added by #26 is nice though because it prevents us making the same mistake with another dependency so +1

Reworked the new test to loop round the smaller loop (tiny optimisation but why not) and fixed test comments to match and for correctness.

Re: #34 - Ugh, yeah. The test passed locally when I ran it via phpunit. When I saw the failure, I figured it was some run-tests.sh weirdness. Thanks for sorting it out!

Here's patches for all the other branches with the latest changes.

I've only added tests and test fixes to this patch. The fix was made by @hussainweb in #14. Both @greg.1.anderson (who has also added tests) think this rtbc - see #36. Therefore this is rtbc.

I think we need to commit this to 8.8.x first and wait for the github mirror / packagist to get a new 8.8.x-dev and the test running composer update on a core-remmended project. Once we confirm that you're not updated to 8.8.x-dev (which not as bad as been updated to 8.9.x-dev) then we can commit this to the other branches.

Well, today I ran composer update on my 8.9.x-dev environment (#3) today ... where I wanted to stay for now. But it was downgraded back to 8.8.5. Is this on purpose?

Thanks for your help, Greg! I will
try it like that.

Committed/pushed #35 to 8.8.x, let's see what happens with packagist. Leaving RTBC for the other branches. We should also make sure to explicitly test what happens after the 8.9.x commit too prior to tagging a new release.

So step 1 looks like this...

$ composer create-project drupal/recommended-project my-project
Installing drupal/recommended-project (8.8.5)
  - Installing drupal/recommended-project (8.8.5): Loading from cache
.... lots of guff...

$ cd my-project
$ composer update                                                                                                                                                                                                                                   Mon 11 May 
Loading composer repositories with package information
Updating dependencies (including require-dev)
Package operations: 0 installs, 3 updates, 0 removals
  - Updating composer/installers (v1.7.0 => v1.9.0): Loading from cache
  - Removing drupal/core (8.8.5)
  - Installing drupal/core (8.8.x-dev 4f21d44): Cloning 4f21d44382 from cache
  - Updating drupal/core-recommended (8.8.5 => 8.8.x-dev 33bda32)

So we need to revert and push harder on the composer issue.

Confirmed that the revert fixes #49.

So one thing we can do here is commit #41 to Drupal 9.x because at least we can avoid some of this mess there. Moving to 9.x for that whilst we work out how to deal with Drupal 8

Changing the issue title to reflect #52. The important thing is that we don't commit this (on any other patch that allows anything other than composer/installers 1.7.0 for the core-recommended project to Drupal 8.x.x. Otherwise we'll end with people being updated to dev releases.

This is good for Drupal 9 because we have no stable Drupal 9 releases yet and core-recommended in Drupal 8 does at this minute result in updating to Drupal 9 (if it did then everyone would have been updating to Drupal 9 the moment we pinned composer/installers to 1.9.0 :D). I think this is more than "good" for Drupal 9 because this will stop us getting into the same trouble once composer/installers 1.10.0 comes out.

OK. Committed/pushed to 9.1.x and 9.0.x

me:

I think the solution was for there to be an 8.8.x release with it first? Does that mean committing it to 8.8 right before 8.8.7, tagging 8.8.7, and then committing to 8.9.0 and tagging 8.9.0?

greg.1.anderson:

Yeah, so if someone has:

• Drupal 8.8.6
• Minimum-stability: dev (default) with “prefer-stable”
• composer/installers pinned to a specific version (as it is in drupal/core-recommended:8.8.6)
• composer/installers as a top-level dependency (default — also, this might not be necessary)

Then, if Drupal 8.9.x-dev does NOT pin composer/installers, then Composer will prefer 8.9.x-dev over staying on 8.8.6.
The solution is, as you said, to make sure there is an 8.8.7 w/out composer/installers pinned before the 8.9.x-dev has an unpinned composer/installers

So should we plan to commit this to 8.8.x... now? Then tag 8.8.7 Wednesday, then only after 8.8.7, commit it to 8.9.x and tag 8.9.0.

We need to commit this to 8.8.x and tag 8.8.7 as close as possible. Whilst this is committed to 8.8.x and we don't have a tag then people on 8.8.6 will be updated to 8.8.x-dev.

And once we have 8.8.7 I think (not 100% sure) that we need to commit this to 8.9.x asap because anyone on 8.9.x might have a chance of being downgraded to 8.8.x-dev. Because 8.8.x-dev will be compatible with composer/installers:1.9.0 but 8.9.x-dev will still only allow 1.7.0.

I think in an ideal world we'd push the 8.8.x and 8.9.x commits at the same time and then tag and push 8.8.7 in the next moment.

We have seen a similar issue with a user updating both drupal/core and drupal/core-dev at the same time, which had the same effect of updating to 8.9.x-dev. So I think the long term fix is to ensure that we no longer have minimum stability:dev and advise people to use stability flags in their requirements.

Considering that composer/installers is a top level dependency, this one is at a much higher risk of causing this sort of snafu, so the plan outlined in 58/59 looks like a reasonable way to shield us from composer selecting different versions if we upgraded the installers requirement for composer 2.

Given that, Im RTBC for this to go back to 8.8.x/Tag 8.8.7, then go into 8.9.x.

Uploading the correct patches for 8.8.x and 8.9.x so they can have another run at the tests and so it's obvious what the correct patches are.

Would be great to have a change record so we can link it from the release notes :) Thanks!

Been trying to work out what to put on the CR. But it’s hard to write one and know what to say. Like there is nothing a site, module developer or core developer to do different in the future.

For me this is more of a bugfix. If the release team’s plan works then no one should notice anything. Fingers-crossed.

If we did add a CR it would say exactly what the release notes say ie.

The project composer/installers is no longer pinned in drupal/core-recommended. It may now be upgraded whenever a new version is available, and a Composer-managed site runs composer update.

Which kinda boils down to: when you run composer update something is updated.

Do we believe there could be side effects or conflicts? What to do then? Why would they happen?

Saving issue credits.

Very glad to see this issue fixed.

I tested this by doing

composer create-project drupal/recommended-project blah2 8.8.4
cd blah2
composer update

-> went to 8.9.0 as expected

And then

composer create-project drupal/recommended-project blah2 8.8.4
cd blah2
// Edit composer.json to only allow 8.8 ie. change ^8.8 to ~8.8.0
composer update

-> went to 8.8.7 as expected

And the root composer.json has

    "minimum-stability": "dev",
    "prefer-stable": true,

as expected.