Problem/Motivation

In the core-recommended package, as well as the core-dev-pinned metapackages we use self.version to represent the desired version of drupal/core.

self.version can be *very* problematic in several situations, leading to a frustrating situation where your site cannot be updated unless you are updating multiple things at a time, or at all if you happen to be on a branch that may or may not resolve properly to a composer version.

Now that we're generating the packages inside of core itself, we can generate the exact version declaration.

Proposed resolution

Change the generators to use the version as declared in core/lib/Drupal.php

Remaining tasks

Release process changes

When a release is being tagged, we must regenerate the metapackages to reflect the tag, and return them to their original -dev state after the release is pushed.

User interface changes

none

API changes

none

Data model changes

none

Release notes snippet

The core version in the core-recommended and core-dev-pinned metapackages will now be pinned to exact versions, and not rely on composer to extrapolate the version.

CommentFileSizeAuthor
#18 3090906-9.0.x-18.patch3.86 KBalexpott
#18 3090906-8.8.x-18.patch3.86 KBalexpott
#18 3090906-8.9.x-18.patch3.86 KBalexpott
#9 3090906-9.patch3.86 KBravi.shankar
#7 3090906-7.patch3.5 KBravi.shankar
#5 3-to-5-interdiff.txt1.29 KBgreg.1.anderson
#5 3090906-5.patch4.11 KBgreg.1.anderson
#3 3090906_8.9.x.patch2.81 KBgreg.1.anderson
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mixologic created an issue. See original summary.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented
Status: Active » Needs review

Here's a patch.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented
FileSize
3090906_8.9.x.patch2.81 KB

No, really, here's a patch.

Status: Needs review » Needs work

The last submitted patch, 3: 3090906_8.9.x.patch, failed testing. View results

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented
Status: Needs work » Needs review
FileSize
3090906-5.patch4.11 KB
3-to-5-interdiff.txt1.29 KB

Update the test to match the patch.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented
Status: Needs review » Needs work 
+++ b/composer/Generator/Builder/DrupalPinnedDevDependenciesBuilder.php
@@ -2,13 +2,15 @@
+   * {@inheritdoc}Composer

Minor typo

ravi.shankar’s picture

ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented
Status: Needs work » Needs review
FileSize
3090906-7.patch3.5 KB

I have fixed it.

Status: Needs review » Needs work

The last submitted patch, 7: 3090906-7.patch, failed testing. View results

ravi.shankar’s picture

ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented
Status: Needs work » Needs review
FileSize
3090906-9.patch3.86 KB
Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented
Status: Needs review » Reviewed & tested by the community

Hey, thanks for the reroll, this is great.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

The means that the meta packages will change when we tag a Drupal release. I think we need to update https://www.drupal.org/core/maintainers/create-core-release and https://github.com/xjm/drupal_core_release/pull/8 has landed.

However the requirement to do this exists already so I think we can proceed here.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +8.8.0 release, +Needs release note

This should be mentioned in the beta1 release notes (and needs a release note).

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

(Also I can't decide if it needs a CR? Maybe?)

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +Amsterdam2019
Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented
Priority: Normal » Critical
Issue summary: View changes
Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented
Priority: Critical » Major
ressa’s picture

ressa CreditAttribution: ressa at Ardea commented
Issue summary: View changes

Fixed minor typo in Issue Summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs work » Needs review
FileSize
3090906-8.9.x-18.patch3.86 KB
3090906-8.8.x-18.patch3.86 KB
3090906-9.0.x-18.patch3.86 KB

Here's the patches for all the supported versions. I'm not sure what the purpose of a CR or a release note would be here. We're changing the meta-packages and as such I don't think there is anything for a user to do. @Mixologic am I correct that a user will not need to make any changes as a result of this? I think the only people this affects is Drupal core release managers.

The 8.9.x patch is exactly the same as #9 but included here for completeness and simplicity.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: -8.8.0 release, -Needs release note +Needs release note, +Needs change record, +8.8.0 release notes

Cleaning up tags. (Did someone lowercase a bunch of issue tags recently? Violates our text standards and is disruptive... off-topic.) The release note needs to explain the disruption, and the CR at least needs to describe the change required for core development process: new command to run every time a release is created or a module is added, and the new files it will change. It might make sense to append this to the other CR from the previous issue.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

I've lost track of which issue it is that adds the test files that also get automatically updated; can we reference it here?

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented
Issue tags: -needs release notes, -Needs change record

This change is a bugfix, and therefore there wont be any disruption. We're fixing a bug that will prevent some edge case scenarios where people will not be able to upgrade. Now that beta1 is out, if people upgrade from beta1 to a version with this fix, they will get this pinned requirement instead of self.version, which should result in no change. The release note snippet that I wrote already is all I can even think of to document this change for end users.

I wrote a change record, and noted that the applicable audience is for release managers and the release process: There are neither API, nor user interface changes.

I went ahead and updated the core release document (https://www.drupal.org/core/maintainers/create-core-release) with the additional steps. The steps are non interfering or destructive on earier versions of drupal core (8.6/8.7 etc), so at worst they're just extra time. -> That doc could probably use some refreshing (old links to cgit etc).

There are no test files that get automatically updated, the versions that used to be in the tests were incidental, and not relevant to how the tests function, and were not something that required changes or maintenance, thus we removed them to reduce concern that they were something that mattered. https://www.drupal.org/project/drupal/issues/3087626#comment-13320847 is the comment where you expressed this concern, and later in the issue the version numbers were removed to alleviate that concern.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented
Issue summary: View changes

Core docs updated,

Release script PR here: https://github.com/xjm/drupal_core_release/pull/9

Ghost of Drupal Past’s picture

Ghost of Drupal Past
he/him
CreditAttribution: Ghost of Drupal Past commented

I feel like I should resurrect the happy chx tag...

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented
Status: Needs review » Reviewed & tested by the community

I think this can go back to RTBC.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

I just combined this patch with #3086644-55: LegacyProject composer templates wrongly reference 8.x + fix test coverage which will probably be the eventual test for all this.

Running COMPOSER_ROOT_VERSION=8.9.x-dev composer update --lock yielded changes like this:

diff --git a/composer/Metapackage/CoreRecommended/composer.json b/composer/Metapackage/CoreRecommended/composer.json
index aef8a4008f..3e6d102798 100644
--- a/composer/Metapackage/CoreRecommended/composer.json
+++ b/composer/Metapackage/CoreRecommended/composer.json
@@ -7,7 +7,7 @@
         "webflo/drupal-core-strict": "*"
     },
     "require": {
-        "drupal/core": "self.version",
+        "drupal/core": "8.9.x-dev",

After that, the tests still pass, so +1 RTBC.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +Needs release manager review

Still testing to ensure this will work with release scripts, so this is blocked until that's resolved.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

So I applied this locally and tested it with the changes to the tagging script that @Mixologic submitted a PR for. I got this diff if I tagged it as 8.8.0:

[ibnsina:drupal | Fri 15:35:39] $ git show
commit f3a4251ffa0fe82c02d23cacdd180c10bcd72b35 (HEAD -> 8.8.x)
Author: xjm <xjm@65776.no-reply.drupal.org>
Date:   Fri Nov 15 15:34:27 2019 -0600

    Back to dev.

diff --git a/composer/Metapackage/CoreRecommended/composer.json b/composer/Metapackage/CoreRecommended/composer.json
index 55ca7579cb..755b7fe691 100644
--- a/composer/Metapackage/CoreRecommended/composer.json
+++ b/composer/Metapackage/CoreRecommended/composer.json
@@ -7,7 +7,7 @@
         "webflo/drupal-core-strict": "*"
     },
     "require": {
-        "drupal/core": "8.8.0",
+        "drupal/core": "8.8.x-dev",
         "asm89/stack-cors": "1.2.0",
         "composer/installers": "v1.7.0",
         "composer/semver": "1.5.0",
diff --git a/composer/Metapackage/PinnedDevDependencies/composer.json b/composer/Metapackage/PinnedDevDependencies/composer.json
index 4d74ebf6ba..959b64d6db 100644
--- a/composer/Metapackage/PinnedDevDependencies/composer.json
+++ b/composer/Metapackage/PinnedDevDependencies/composer.json
@@ -7,7 +7,7 @@
         "webflo/drupal-core-require-dev": "*"
     },
     "require": {
-        "drupal/core": "8.8.0",
+        "drupal/core": "8.8.x-dev",
         "behat/mink": "1.8.0 | 1.7.1.1 | 1.7.x-dev",
         "behat/mink-browserkit-driver": "1.3.3",
         "behat/mink-goutte-driver": "v1.2.1",
diff --git a/core/lib/Drupal.php b/core/lib/Drupal.php
index 780ff3060e..ca0f8c3e29 100644
--- a/core/lib/Drupal.php
+++ b/core/lib/Drupal.php
@@ -82,7 +82,7 @@ class Drupal {
   /**
    * The current system version.
    */
-  const VERSION = '8.8.0';
+  const VERSION = '8.8.1-dev';

So on the one hand... yay, something worked. On the other, though, the values in the metapackages themselves look wrong. They don't match the core version constant. Is that intentional or is it just a bug in the script? The correct version constant for 8.8 at present is 8.8.0-dev, not 8.8.x-dev. After 8.8.1 it's 8.8.1-dev, etc.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

There were also no changes to core's own composer.json or composer.lock, and I was expecting there to be.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

It's because there is no 8.8.1 dev branch. We only have the 8.8.x branch. If this was 8.8.1-dev composer wouldn't be able to work out that this means get the 8.8.x branch.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented

So it looks like it worked for the tag: when VERSION was 8.8.0, then drupal/core was also required as 8.8.0.

As far as VERSION = 8.8.1-dev, I think that 8.8.x-dev is the correct version constraint for the drupal/core package, as @alexpott explained in #29.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

+1 on #29 with the disclaimer that I don't know enough about the release script to really know all the implications.

Here's what \Drupal\Composer\Composer::drupalVersionBranch() does with \Drupal::VERSION:

    return preg_replace('#\.[0-9]+-dev#', '.x-dev', \Drupal::VERSION);

So if you set \Drupal::VERSION to 8.8.1-alpha1, then that will make it through to the metapackages because it's expected to be a tagged release, and Composer can figure out a tagged release. Otherwise dev is major.minor.x-dev, and then Composer knows to just get that branch.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Alright, so according to #30, #27 is WAD. But what #28, about all the self.version in composer.lock and core/composer.json?

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented

self.version is not harmful in "replace", so core's composer.json does not have to change. Core does not have its own composer.lock; the project-level composer.lock serves this purpose for the entire project, including core.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

So we do or do not expect composer.lock to change each time core issues a new release?

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented

If I am not mistaken, issuing a release will modify core/lib/Drupal.php, but not composer.json or core/composer.json, so the metapackages will change, but composer.lock will not.

I did not test to verify tho.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

It's possible that we won't see a change to composer.lock, because maybe no dependencies were changed since the last tagged release. But even if there's no change, we should see a change to the metapackages to reflect the tagged release.

So, for instance, after applying the patch in #18, and then changing \Drupal::VERSION to 8.8.1-alpha1, I do composer update -lock. There's no change to composer.lock, but I do see this change, and it's the right one:

diff --git a/composer/Metapackage/CoreRecommended/composer.json b/composer/Metapackage/CoreRecommended/composer.json
index 8ff7b01629..c00b5f36c5 100644
--- a/composer/Metapackage/CoreRecommended/composer.json
+++ b/composer/Metapackage/CoreRecommended/composer.json
@@ -7,7 +7,7 @@
         "webflo/drupal-core-strict": "*"
     },
     "require": {
-        "drupal/core": "self.version",
+        "drupal/core": "8.8.1-alpha1",
         "asm89/stack-cors": "1.2.0",
         "composer/installers": "v1.7.0",
         "composer/semver": "1.5.0",
diff --git a/composer/Metapackage/PinnedDevDependencies/composer.json b/composer/Metapackage/PinnedDevDependencies/composer.json
index 06e6cf8640..0c44f64086 100644
--- a/composer/Metapackage/PinnedDevDependencies/composer.json
+++ b/composer/Metapackage/PinnedDevDependencies/composer.json
@@ -7,7 +7,7 @@
         "webflo/drupal-core-require-dev": "*"
     },
     "require": {
-        "drupal/core": "self.version",
+        "drupal/core": "8.8.1-alpha1",
         "behat/mink": "1.8.0 | 1.7.1.1 | 1.7.x-dev",
         "behat/mink-browserkit-driver": "1.3.3",
         "behat/mink-goutte-driver": "v1.2.1",
diff --git a/core/lib/Drupal.php b/core/lib/Drupal.php
index 4de3044b23..5b6b49aabc 100644
--- a/core/lib/Drupal.php
+++ b/core/lib/Drupal.php
@@ -82,7 +82,7 @@ class Drupal {
   /**
    * The current system version.
    */
-  const VERSION = '8.8.0-dev';
+  const VERSION = '8.8.1-alpha1';
 
   /**
    * Core API compatibility.

So then let's change \Drupal::VERSION to 8.8.2-dev, and after composer update -lock we see this, because 8.8.2 dev work will be commits after the last tagged release on the 8.8.x branch:

diff --git a/composer/Metapackage/CoreRecommended/composer.json b/composer/Metapackage/CoreRecommended/composer.json
index 8ff7b01629..755b7fe691 100644
--- a/composer/Metapackage/CoreRecommended/composer.json
+++ b/composer/Metapackage/CoreRecommended/composer.json
@@ -7,7 +7,7 @@
         "webflo/drupal-core-strict": "*"
     },
     "require": {
-        "drupal/core": "self.version",
+        "drupal/core": "8.8.x-dev",
         "asm89/stack-cors": "1.2.0",
         "composer/installers": "v1.7.0",
         "composer/semver": "1.5.0",
diff --git a/composer/Metapackage/PinnedDevDependencies/composer.json b/composer/Metapackage/PinnedDevDependencies/composer.json
index 06e6cf8640..959b64d6db 100644
--- a/composer/Metapackage/PinnedDevDependencies/composer.json
+++ b/composer/Metapackage/PinnedDevDependencies/composer.json
@@ -7,7 +7,7 @@
         "webflo/drupal-core-require-dev": "*"
     },
     "require": {
-        "drupal/core": "self.version",
+        "drupal/core": "8.8.x-dev",
         "behat/mink": "1.8.0 | 1.7.1.1 | 1.7.x-dev",
         "behat/mink-browserkit-driver": "1.3.3",
         "behat/mink-goutte-driver": "v1.2.1",
diff --git a/core/lib/Drupal.php b/core/lib/Drupal.php
index 4de3044b23..7ad133815e 100644
--- a/core/lib/Drupal.php
+++ b/core/lib/Drupal.php
@@ -82,7 +82,7 @@ class Drupal {
   /**
    * The current system version.
    */
-  const VERSION = '8.8.0-dev';
+  const VERSION = '8.8.2-dev';
 
   /**
    * Core API compatibility.

This is all without changing the lock file, but still reflecting the release (or non-release) version of core in the metapackages.

The product (drupal/legacy-project or drupal/recommended-project) won't have a lock file. Only the dev version (drupal/drupal) will.

greg.1.anderson’s picture

greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented

Thanks for running the update, @mile23. #36 LGTM.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Issue tags: +DrupalSouth 2019
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: -Needs release manager review

Alright, I've tested the updates I made to the script with a normal security release, and confirmed that the necessary merges are handled correctly. Thanks for your patience on this issue.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Updating issue credit for everyone who helped with testing, docs updates, etc.

  • xjm committed ed5986f on 9.0.x 
    Issue #3090906 by alexpott, greg.1.anderson, ravi.shankar, xjm,...

  • xjm committed cdf8cf1 on 8.9.x 
    Issue #3090906 by alexpott, greg.1.anderson, ravi.shankar, xjm,...

  • xjm committed e588966 on 8.8.x 
    Issue #3090906 by alexpott, greg.1.anderson, ravi.shankar, xjm,...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

Committed to 9.0.x, 8.9.x, and 8.8.x. Thanks!

I'm leaving the DrupalSouth tag on even though the event hasn't started yet because I'm on a plane flying there... that counts, right?

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Published the CR.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.