Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates

I haven’t done a full look through of the code lately. I suspect there may be some logic that ignores the API compatibility prefix, assuming it is the same all the time. Be sure that testing for 8.x-1.0 > 7.x-2.0 exists.

The changes look fine.

But … only somebody who knows the existing translation infrastructure can truly sign off on this. I think that's @drumm and @Gábor Hojtsy, who else?

only somebody who knows the existing translation infrastructure can truly sign off on this

This has nothing to do with the translation infrastructure, that would be #3016471: Make localization file download major version agnostic. This is about fetching available update data for modules. We should check some scenarios:

1. Changing to /all/ will return 7.x, etc. releases of the modules. This will increase processing need on the client on the data. Not sure if we have projects which had LOTs of releases to the extent this could be a penalty but would be good to check. @drumm can maybe help query the Drupal.org DB for projects ordered by number of releases.

2. Changing to /all/ will return 9.x releases in 8.x and vice versa. Do we have code elsewhere to filter those out? Will d.o allow 9.x release prefixes? If so, will those be explicitly only 9.x+ compatible or could those be also 8.x compatible? If so how does Drupal core tell which release to suggest as viable new update to install. (While update module currently does not resolve the requirements/dependencies of projects (nor info.yml nor composer.json), at least it only suggests projects compatible with the same major core version. Changing this to /all/ changes that behaviour once people release 9.x branched projects (if we allow that).

Oops, right. Sorry. Mixing up issues 🙃

@drumm can maybe help query the Drupal.org DB for projects ordered by number of releases.

mysql> SELECT n.title project, n.type, count(1) releases, concat('https://www.drupal.org/i/', n.nid) link FROM node n INNER JOIN field_data_field_release_project fdf_rp ON fdf_rp.field_release_project_target_id = n.nid WHERE n.status = 1 GROUP BY n.nid ORDER BY releases DESC LIMIT 50;

See also #3009338: [META] Support semantic versioning for extensions (modules, themes, etc) in Drupal core, and allow modules to be compatible with Drupal 8 and 9 at the same time which effectively removes the core compatibility version number component, making 8 in 9 / 9 in 8 irrelevant.

We will want change all to current when #2688479: Update update status XML for Drupal 9 and contrib semantic versioning is done. Figuring out how to get this committed is still very important, especially in the 9.0.x branch, so update status can keep working in general.

I'm pretty concerned about the bandwidth and RAM required for slurping down those 'all' XML feeds. :/ Some of them are completely huge. If we suspect that most sites use 10-50 contrib modules, huge * 20 is Really Huge(tm). ;) Although there is (was?) code to batch fetch the feeds, we still have to load *all* of it into RAM to process the available updates report. We don't (yet?) do the logic one module at a time, independently. Maybe we could. It's been a long time since I looked at any of it.

Therefore, I think a new 'current' feed would be much better for everyone, once it exists. TBD.

However, all this code is going to get much more complicated, since there's no way for update.module on your site to know everything about the requirements/compatibilities/etc of all the future releases coming down those feeds. This whole system was designed ~14 years ago when contrib was totally locked to specific versions of core. Now that we're breaking that assumption, update.module is going to have a really hard time keeping up.

e.g. I'm running token 3.2.5 on 8.9.3 core. Token 4.0.0 is released, which is only compatible with 9.1.x and up. How will update.module know that? What's it going to tell me? That a new release of token is available, but I better click on the release notes and read the fine print? Or are we somehow going to teach update.module all about the dependency web of everything? Or do we hope that all contrib maintainers everywhere actually implement SemVer properly, and update.module never recommends (blindly) that you jump major versions? Or what?

We're getting into the territory of Wrong Tool For the Job(tm).

Either we're going to find ourselves re-implementing all the dependency reconciliation logic of composer, or we're going to have a system that gives people bad advice.

Or we're going to realize that update.module has served us well for a long time, but maybe should now be put out to pasture.

Perhaps we should focus on the composer initiative, and have a UI that talks to that, and can give you a report of what would happen if you did a "composer update"...

Love you, update.module. You were probably my biggest contribution to core. But maybe you need to die. ;)

-Derek

p.s. If we want to keep update.module for the usage stats on projects, we've got the telemetry initiative for that, right? We could still have folks opt-in to a system that lets core "phone home" to send info about what's in use, PHP versions, etc, but stop linking that to the available updates functionality. That can (should?) be a different system from an in-core web UI around composer update --dry-run (or equivalent).

p.p.s. Is there a more appropriate meta to be having this discussion in?

Therefore, I think a new 'current' feed would be much better for everyone, once it exists.

I think I have the code for this all ready to deploy. It needs a bit more testing, and documentation. I expect I’ll be doing that on Monday when I’m back at home.

I am removing the <version_*> tags, so progress on #3085717: [PP-1] Do not rely on version_* tags would be appreciated.

The other big change will be from <supported_majors> to <supported_branches>. “branch” in this case is not a Git/etc branch (releases are not actually aware of Git history, or require logical use of tags and branches); it is the version number with everything after the last . removed. So core using semver may have <supported_branches>7.,8.8.,8.9.,9.0.</supported_branches>, and a contrib module using both semver and legacy versions may have <supported_branches>7.x-1.,8.x-1.,1.1.</supported_branches>. The code for generating these in project_release on Drupal.org is https://git.drupalcode.org/project/project/blob/d4fba0cab5cc6c3b8cd920bc...

<recommended_major> is also going away. If we need a <recommended_branches>, I have that ready to go: https://git.drupalcode.org/project/drupalorg/blob/81b4df451532067dd19d19...

#3074998: Add explicit information about core compatibility to update data is also coming into availability. It will take a few days for the data to get fully re-parsed, and xml regenerated. The tag added there is only in the current XML.

General concern before we get too much further in here. Apologies if this is discussed elsewhere that I missed.

What's the long-term plan for the "current" feeds? They're briefly introduced/described at #2688479-7: Update update status XML for Drupal 9 and contrib semantic versioning thusly:

• Is generally the same XML format.
• Has releases with 8.x or 9.x compatibility, plus releases without API compatibility. Contrib releases made in the future with semver will not have the API compatibility part of their version number #2681459: Support contrib semver releases
• Removes any tags that core doesn’t use and aren’t otherwise useful.
• Adds any newly-necessary data, like support for #2991207: Drupal core should inform the user of the security coverage for the site's installed minor version including final 8.x LTS releases

So what's going to happen 8 years from now when a site is still on 8.8.N, queries for updates.d.o/project/drupal/current/current.xml (or whatever it is) and only D9-D11 releases are in that feed?

Is "Current" just going to grow from here on out, eventually becoming so huge and hard to work with that we're not much better off than using "All"?

Do we want something in "current" to indicate a "version" of the API between update.module and updates.d.o? NOT a Drupal Core API compatibility term (which is indeed on the way to the dustbin of history), but a update.module app vs. server API version #? current-v1, then someday we can move to current-v2, etc.

We have to think about this crap now, for whatever we put in 8.8.0, or it's going to come back to haunt future-us in N years. ;)

Thanks,
-Derek

Is "Current" just going to grow from here on out, eventually becoming so huge and hard to work with that we're not much better off than using "All"?

That is the current plan, yes. current includes any release with an 8.x- API compatibility prefix to the version number, or any without an API compatibility prefix.

Bandwidth isn’t too much of a concern here. What can be hard to work with is the general finickiness of formatting - I saw some evidence that release ordering in the XML being significant tripped people up at least a couple times.

Do we want something in "current" to indicate a "version" of the API between update.module and updates.d.o? NOT a Drupal Core API compatibility term (which is indeed on the way to the dustbin of history), but a update.module app vs. server API version #? current-v1, then someday we can move to current-v2, etc.

I’ve never encountered schema documentation, I went ahead and made some: https://www.drupal.org/drupalorg/docs/apis/update-status-xml

current is indeed essentially a new API version. I only added <core_compatiblity> to current and a few things are taken away. I also removed the <release>-level <mdhash> and <filesize> elements, https://git.drupalcode.org/project/drupalorg/commit/25d1c79c84fc75d306d5...

Now is a good time to remove other inessential elements, like <release>’s <status> looks like it is always published. And <download_link> is duplicitous, but core does currently use it.

I don’t want to add much more changed that affect core’s implementation since this does need to get into core for update status to work properly for Drupal 9. There is no https://updates.drupal.org/release-history/drupal/9.x and https://updates.drupal.org/release-history/drupal/8.x does not contain 9.x releases.

The next version of this API might be something completely different, like using Drupal.org’s composer endpoint directly.

So what's going to happen 8 years from now when a site is still on 8.8.N, queries for updates.d.o/project/drupal/current/current.xml (or whatever it is) and only D9-D11 releases are in that feed?

This is probably OK I think? - In eight years, 8.8.N will be more than five years out of support.

Re #30

So what's going to happen 8 years from now when a site is still on 8.8.N, queries for updates.d.o/project/drupal/current/current.xml (or whatever it is) and only D9-D11 releases are in that feed?

https://updates.drupal.org/release-history/drupal/current will indefinitely contain all 8.* & later releases. The simplicity of generating static files is still a bigger win than trying to keep the number of releases in a file down.

Re #28

Add core_compatibility to current feed that uses 8.x.

Done, #3074998-29: Add explicit information about core compatibility to update data. All release history XML should be regenerated within the next 20h.

So in effect our tests are testing live updates.drupal.org XML. Is this intentional? It doesn't seem like a good thing.

This has caused tests to randomly fail in the past. Testing shouldn’t also be an uptime and connectivity check with Drupal.org. Notably, one of the core tests was downloading translations, and failing often enough to be temporarily be removed until a replacement using bundled artifacts was made.

I think this is not a great time to be changing more things the update xml because current functionality on the core side is not covered with comprehensive testing.

I do have one more change I’d like to consider. I was unable to find a code path which outputs anything other than “published” for the release status, <releases><release><status>published</status>.

@drumm re: #26:

• Bandwidth for the downloads isn't my only concern. Update.module also loads all these into RAM. That gets costly on a site with a lot of projects that have a lot of releases...
• Indeed, the order of releases in the feed was significant to the original implementation of update.module. Probably remains that way now. Doesn't have to stay that way forever.
• Yeah, I'm not saying we ever documented the schema between updates.d.o and update.module. ;) Just wondering if we should start now. Thanks!
• Agreed that /current is basically a new schema API version, and if we change it again, we can point it somewhere else and that'll implicitly be a new schema version.

@catch re: #30: I just invented 8 years. ;) Regardless, I'm worried by "that's okay, X will no longer be supported" -- we want update.module to tell people they're not supported and what to upgrade to so they return to the happy pasture of supported Drupalness. So I think whatever we do, we have to plan to support it working far into the future, even past official "supported" status.

@tedbow re: #31: great find! I commented at #3095755: Tests should not do requests to updates.drupal.org.

@drumm re: #33: Fair enough. I think that might come back to haunt us some day, but that's probably the best we can hope for.

Does it make sense to only put 8.8.x and higher core releases in the https://updates.drupal.org/release-history/drupal/current feed, since only core 8.8.x and higher will be consuming it?

Thanks all,
-Derek

@dww: by switching to this "current" "API-version" of the update info, we don't attach to always use it in the future in Drupal 10, 11, etc. For Drupal 10 we can choose to stop growing "current" with items that are not Drupal 9 compatible and switch to composer endpoint data, or whatnot as @drumm indicated above. As you pointed out elsewhere update module is magically holding up but is less and less desirable as a feature set given the much more complicated nature of dependencies both in the back-backend (shorter PHP release cycles) and in Drupal. So I would be surprised if update module as-is survives too long.

Reparenting to the alpha1 issue as this is currently listed as an alpha blocker.

I am not sure #3085717: [PP-1] Do not rely on version_* tags needs to be its own issue now though it would make this patch a bit smaller if we want to do it first.

Agreed, I actually started that as an experiment to help check how doable it might be. Whatever strategy makes the whole switch to /current most committable is best.

Removes recommeded_major and replaces it with recommeded_branch

This is actually multiple recommeded_branches if we want to flip this on.

recommeded_major was the recommended major version for each API compatibility, but we do not have specific API compatibility anymore. Switching this to “branch” helps, we’re not doing something like recommending 3.x.x for everyone, when 3 might only work with a certain range of core and other dependencies.

Currently, there are no rules for what can be recommended, other than it had to be supported. I couldn’t think of a good reason to have a restriction, since I wasn’t sure if the whole “recommended” concept still made sense, I don’t know of an analogue* in other projects, and implementing it that way in our form was just easier. (The legacy …/8.x, …/7.x, etc XML generation picks the highest major version that’s recommended in the API compatibility.)

I know it is a larger change, but I’m still not sure that we need “recommended” at all. If we do need it, I don’t know if there’s any way around having multiple recommended branches.

* marking branches as LTS is sort of an analogue, it is a meaningful label for a branch. That’s a whole other issue.

A reroll was needed.

Updated IS to summarize the XML changes.

Went through the patch and only found nits, but this could probably use a bit of clarification as to whether #50 necessitates any additional steps. (There are also some additional possible use cases that @tedbow brought up in a discussion, but it's not yet clear if those are in-scope for this issue)

1. +++ b/core/modules/update/tests/src/Unit/ModuleVersionParserTest.php
   @@ -0,0 +1,136 @@
   +   * Dataprovider for expected version information.
   

   Split "Dataprovider" into two words.

2. +++ b/core/modules/update/tests/src/Unit/UpdateFetcherTest.php
   @@ -71,20 +67,20 @@ public function providerTestUpdateBuildFetchUrl() {
   +    $expected = 'http://www.example.com/' . $project['name'] . '/current';
   

   Since a constant is no longer used, this and other instances can now be built inside double quotes instead of dot concatenation.

Clarifying recommended_branches in the issue summary.

The nits in #52 still need to be addressed

And one other thing:

+++ b/core/modules/update/update.compare.inc
@@ -122,12 +123,14 @@ function update_calculate_project_data($available) {
+ * current version is used. If the current
+ * version is not in supported branch, the project maintainer's recommended
+ * branch is used to determine the major version to use. There's also a check to

There's a line break and missing 'the' here, but that may be irrelevant as this function no longer takes into account the project maintainers recommended branch.

With that last bit of stuff addressed in #61 this can get RTBC'd.

1. Love that is immutable. I wonder if it'd be better to call the object ModuleVersion - as once it is constructed that's what it represents.

2. +++ b/core/modules/update/src/ModuleVersionParser.php
   @@ -0,0 +1,118 @@
   +    $version_parts = explode('.', $this->getVersionStringWithoutCoreCompatibility());
   

   I wonder if we should do this on construct as when we parsing the XML files we do ->getMajorVersion() followed by ->getMinorVersion() and as this is a value object there's no harm of a $this->versionParts property getting out-of-sync with $this->version.

3. +++ b/core/modules/update/src/ModuleVersionParser.php
   @@ -0,0 +1,118 @@
   +    $version = strpos($this->version, '8.x-') === 0 ? str_replace('8.x-', '', $this->version) : $this->version;
   

   I know there are plans to remove \Drupal::CORE_COMPATIBILITY which make total sense but this code is analogous to \Drupal\Component\Version\Constraint::parseConstraint() and that's getting its core compatibility from the constant. I think whilst we still have the constant we should be using it.

4. +++ b/core/modules/update/tests/src/Unit/UpdateFetcherTest.php
   @@ -5,10 +5,6 @@
   -if (!defined('DRUPAL_CORE_COMPATIBILITY')) {
   -  define('DRUPAL_CORE_COMPATIBILITY', '8.x');
   -}
   

   Nice.

@tedbow changes look good. Re questions:
1. Starting out @internal is a good idea. That said I can't imagine making an incompatible API change.
2. Not sure it matters too much given the method is private - but there's also no harm in inlining - up to you.

Reviewed the interdiffs and confirmed the feedback since the last RTBC was addressed, and that the test XML matches the structure of the actual content in /current feeds for Drupal and contrib.

Reroll has straightforward changes, so this can go back to RTBC.

One question, one missing test case, and some nits. The patch as a whole looks great!

1. +++ b/core/modules/update/src/ModuleVersion.php
   @@ -0,0 +1,125 @@
   +    $this->versionParts = explode('.', $this->getVersionStringWithoutCoreCompatibility());
   ...
   +    return count($this->versionParts) === 2 ? NULL : $this->versionParts[1];
   

   It strikes me that there are a lot of count() calls and ternary operators and numeric array access.

   Instead of keeping versionParts as a property, why not initialize 3 properties?

   Not trying to derail an RTBC issue, but figured I'd ask.

2. +++ b/core/modules/update/src/ModuleVersion.php
   @@ -0,0 +1,125 @@
   +    return $patch === 'x' ? NULL : $patch;
   
   +++ b/core/modules/update/tests/src/Unit/ModuleVersionTest.php
   @@ -0,0 +1,151 @@
   +  public function providerVersionInfos() {
   

   This is missing a case like 1.2.x. Otherwise there's full unit test coverage.

3. +++ b/core/modules/update/src/ModuleVersion.php
   @@ -0,0 +1,125 @@
   +    return new static ($branch . 'x');
   

   Nit: no space between static and (

4. +++ b/core/modules/update/src/ModuleVersion.php
   @@ -0,0 +1,125 @@
   +    $version = strpos($this->version, \Drupal::CORE_COMPATIBILITY) === 0 ? str_replace('8.x-', '', $this->version) : $this->version;
   +    return $version;
   

   Nit: no local variable needed

5. +++ b/core/modules/update/tests/src/Unit/ModuleVersionTest.php
   @@ -0,0 +1,151 @@
   +  public function testGetMajorVersion($version, $excepted_version_info) {
   

   Nit: here and throughout "expected" is spelled "excepted"

+++ b/core/modules/update/src/ModuleVersion.php
@@ -142,13 +166,12 @@ public function getVersionExtra() {
   public function getSupportBranch() {
-    $version = $this->version;
-    if ($extra = $this->getVersionExtra()) {
-      $version = str_replace("-$extra", '', $version);
+    $branch = $this->useCorePrefix ? static::CORE_COMPATIBILITY_PREFIX : '';
+    $branch .= $this->majorVersion . '.';
+    if ($this->minorVersion) {
+      $branch .= $this->minorVersion . '.';
     }
-    $parts = explode('.', $version);
-    array_pop($parts);
-    return implode('.', $parts) . '.';
+    return $branch;
   }

The changes above are great and address my concern. But I especially like the resulting change to this method!

Setting back to RTBC

I think we need a test-only patch for #75 to show it failing.

@kim.pepper the need for #75 is shown by the test failures in #72.

I think the first issue in #83 was supposed to have been something else, since currently it's referring to this issue?

(Bear with me; I'm just starting to review this issue and parts of the IS are hard to understand.)

1. +++ b/core/modules/update/src/ModuleVersion.php
   @@ -0,0 +1,100 @@
   +class ModuleVersion {
   

   I like this class, but there's a number of eyebrow-raisers in it (comments follow).

2. +++ b/core/modules/update/src/ModuleVersion.php
   @@ -0,0 +1,100 @@
   +  /**
   +   * The core compatibility prefix used in version strings.
   +   *
   +   * @var string
   +   */
   +  const CORE_COMPATIBILITY_PREFIX = \Drupal::CORE_COMPATIBILITY . '-';
   

   Immediately confused here... isn't one of our goals to deprecate CORE_COMPATIBILITY? Is there a followup related to this, and how will ModuleVersion work after? Shouldn't we be handling the case where that constant no longer exists?

3. +++ b/core/modules/update/src/ModuleVersion.php
   @@ -0,0 +1,100 @@
   +   * The version extra string.
   

   An example would be good, e.g.:

   If the module version is '2.0.3-alpha1', then the version extra string is 'alpha1'.

   (Or whatever a correct example is here.)

4. +++ b/core/modules/update/src/ModuleVersion.php
   @@ -0,0 +1,100 @@
   +    $version_string = strpos($version_string, static::CORE_COMPATIBILITY_PREFIX) === 0 ? str_replace(static::CORE_COMPATIBILITY_PREFIX, '', $version_string) : $version_string;
   

   This seems to recognize that the module version may not start with CORE_COMPATIBILITY, but (again) not that core itself will deprecate the constant later.

   Also, what about cases where the module is 7.x-1.2? Should we be throwing an exception or otherwise handling prefixes that are not based on the core compatibility constant?

   Finally, I think we should also at least have a code path and @todo for modules with 9.x- prefixes. Right now we're depending on d.o disallowing that branch, but core should also hardcode that expectation so we don't get undefined behavior. The reason I think a followup might be needed is those ≈10 projects that managed to create a 9.x- branch before we disallowed it. We have to decide how we're going to deal with those first. There might already be an infra issue about those projects.

5. +++ b/core/modules/update/src/ModuleVersion.php
   @@ -0,0 +1,100 @@
   +    return new static($major_version, $version_extra);
   ...
   +  protected function __construct($major_version, $version_extra) {
   +    $this->majorVersion = $major_version;
   +    $this->versionExtra = $version_extra;
   

   Wait but... what about semver? It's not supported yet, but we're trying to support it. Also, shouldn't this also support core modules, which do have a minor version?

   Maybe we don't need it yet, but shouldn't we set things up such that a BC break isn't necessary once we do?

6. +++ b/core/modules/update/src/ModuleVersion.php
   @@ -0,0 +1,100 @@
   +   * Constructs a module version object from a support branch.
   

   Does "support branch" in this context mean "supported branch of core"?

7. +++ b/core/modules/update/src/ModuleVersion.php
   @@ -0,0 +1,100 @@
   +   * This can be used to determine the major and minor versions. The patch
   +   * version will always be NULL.
   

   This is the only mention of minor versions in the class.

   Also, will the patch version actually always be NULL?

8. +++ b/core/modules/update/src/ModuleVersion.php
   @@ -0,0 +1,100 @@
   +   *   The version extra string if available otherwise NULL.
   

   Nit: This is a run-on. I'd change it to:
   The version extra string if available, or otherwise NULL.

1. +++ b/core/modules/update/update.compare.inc
   @@ -139,12 +140,13 @@ function update_calculate_project_data($available) {
   + * branches, so the first step is to make sure the development branch of the
   + * current version is still supported. If so, then the major version of the
   

   "currently supported development branches" seems like an oxymoron to me. In core and in the CI branch labels, we use "development branch" to mean the branch that hasn't had a stable release nor pre-release milestone yet, and is therefore totally unsupported.

   Would "currently supported core branches" be accurate? (Or "currently supported branches of the project" if it's about the project's branches and not core?)

2. +++ b/core/modules/update/update.compare.inc
   @@ -353,18 +355,19 @@ function update_calculate_project_update_status(&$project_data, $available) {
        // Note: Some projects have a HEAD release from CVS days, which could
        // be one of those being compared. They would not have version_major
        // set, so we must call isset first.
   

   We don't call isset() first anymore, so we should update this comment to reflect that we're checking that it's not NULL instead.

   Also, I wonder if any such projects from CVS days are even remotely supported anymore? Maybe we can file a followup to clean that up?

3. +++ b/core/modules/update/update.compare.inc
   @@ -381,32 +384,37 @@ function update_calculate_project_update_status(&$project_data, $available) {
   -    // Look for the 'recommended' version if we haven't found it yet (see
   ...
   +    // Look for the 'recommended' version if we haven't found it yet (SEE
   

   I don't think we mean to change this line to put SEE in all caps? :) Rolling that back should also un-weirdify the diff.

Next step of reviewing this is of course going to be to read update.compare.inc again, which I last did several years ago and which has haunted me ever since. 👻

I read through all the test fixture changes just to help verify that the only changes were removing the defunct keys (and I guess that @tedbow wasn't inserting any trolling about llama skeletons or something). I didn't double-check the exact supported branches for each fixture as yet.

A few general questions about the new feed structure (that, by definition, are probably all actually out of scope for this issue):

1. +++ b/core/modules/update/tests/modules/update_test/aaa_update_test.1_1-alpha1.xml
   @@ -3,10 +3,7 @@
   -  <api_version>8.x</api_version>
   
   @@ -17,15 +14,10 @@
   -      <mdhash>b966255555d9c9b86d480ca08cfaa98e</mdhash>
   -      <filesize>1073761824</filesize>
   

   The IS doesn't mention these things being removed from the feed, but I'm guessing they must be since this issue removes them. Is the new feed and what was changed and why documented somewhere, maybe in a closed infra issue?

2. +++ b/core/modules/update/tests/modules/update_test/aaa_update_test.1_1-alpha1.xml
   @@ -3,10 +3,7 @@
   +  <supported_branches>8.x-1.</supported_branches>
   

   Whoa, funky, the data from d.o actually ends in a dot?

   Like, does d.o actually strip off the x that actually exists on the ends of the branch names I create in git? 😲

   And like. What if someone creates a 8.x-1.y branch? Or literally calls it 8.x-1. or any other such thing? I guess that probably fails some regex and therefore never ends up in the feed nor other d.o functionality. (Hopefully.)

3. +++ b/core/modules/update/tests/modules/update_test/aaa_update_test.2_0-beta1.xml
   @@ -3,10 +3,7 @@
   -<default_major>1</default_major>
   

   Is the concept of a default branch also going away?

4. +++ b/core/modules/update/tests/modules/update_test/aaa_update_test.sec.8.x-2.2_1.x_secure.xml
   @@ -106,15 +83,11 @@
   -   <terms>
   +  <terms>
   

   Now I feel like I need to go back and check the indentation of this in all the fixtures. :P

(I've now set myself up for @tedbow to insert an Easter egg into one of the fixtures and see if I notice it before commit.)

Is the new feed and what was changed and why documented somewhere, maybe in a closed infra issue?

https://www.drupal.org/drupalorg/docs/apis/update-status-xml

Whoa, funky, the data from d.o actually ends in a dot?

Like, does d.o actually strip off the x that actually exists on the ends of the branch names I create in git? 😲

And like. What if someone creates a 8.x-1.y branch? Or literally calls it 8.x-1. or any other such thing? I guess that probably fails some regex and therefore never ends up in the feed nor other d.o functionality. (Hopefully.)

Yes. Specifically, everything after the last dot is stripped to determine a release’s “branch”. This is not the same as a Git branch, it is grouping versions into a series.

Nothing is actually preventing people from making non-sensical releases, like tagging 8.x-1.0 on a 7.x-3.x branch. You can make a 8.x-1.y branch, or any other branch/tag you want, but you can only make releases when those match the version number formatting, which is indeed mostly regexps.

Is the concept of a default branch also going away?

Yes. The D7 code comments show this was some legacy layer:

    // Older release history XML file without supported or recommended.
    $supported_majors[] = $available['default_major'];

    // Older release history XML file without recommended, so recommend
    // the currently defined "default_major" version.
    $target_major = $available['default_major'];

Drupal.org can generate all update status XML files within 12 hours, so any we are producing would have been updated. The legacy update status XML still includes default_major since there’s probably a few outdated sites which can’t understand the replacement.

And all of these API elements related to “major” versions just don’t make sense as we move toward semver.

“Default branch” as a property of a Git repository remains, that is part of Git. It never has affected what goes in update status XML. That’s just the branch you get from git clone without a --branch parameter.

Let's @see https://www.drupal.org/drupalorg/docs/apis/update-status-xml.

1. Normally I'd worry about still not having a policy for how we use private (technically the docs still say we don't use it and the issue's still open) but given that this is a totally new, totally internal, immutable value-object-thing with factory methods (and not injecting services nor of any conceivable use to contrib with the constructor), I think it's actually totally justifiable in this case under a current read of core policy.

   The only remaining thought I'd have with the class then is that the name itself sounds much more generic and globally useful than it is, but I'm also not sure I agree with myself about making the name something more specific either.

2. +++ b/core/modules/update/src/ModuleVersion.php
   @@ -7,14 +7,14 @@
   -   * The core compatibility prefix used in version strings.
   +   * The '8.x-' prefix is used on contrib module version numbers.
   

   Maybe this could say "...for both Drupal 8 and 9"? (Or whatever of that fits in 80 characters.)

3. +++ b/core/modules/update/tests/src/Unit/ModuleVersionTest.php
   @@ -203,4 +225,28 @@ public function providerVersionInfos() {
   +      ['6.x-1.0'],
   +      ['7.x-1.x'],
   +      ['9.x-1.x'],
   +      ['10.x-1.x'],
   ...
   +      ['6.x-1.'],
   +      ['7.x-1.'],
   +      ['9.x-1.'],
   +      ['10.x-1.'],
   

   Should we be testing that 8.x branches aren't invalid as well?
    

4. Should we add unit test coverage about non-dot-ended branch names?

5. +++ b/core/modules/update/update.compare.inc
   @@ -140,8 +140,8 @@ function update_calculate_project_data($available) {
   + * series to consider. The project defines the currently supported branches for
   + * the project, so the first step is to make sure the development branch of the
   

   With the rewriting I now realize it says "The project defines... for the project..." We could say "The projecrt defines its currently supported branches in its Drupal.org configuration" or something along those lines.

6. +++ b/core/modules/update/update.compare.inc
   @@ -254,7 +254,13 @@ function update_calculate_project_update_status(&$project_data, $available) {
   +  catch (UnexpectedValueException $exception) {
   +    // If the version has an unexpected value we can't determine updates.
   +    return;
   
   @@ -274,10 +280,23 @@ function update_calculate_project_update_status(&$project_data, $available) {
   +      catch (UnexpectedValueException $exception) {
   +        continue;
   +      }
   
   @@ -315,7 +334,12 @@ function update_calculate_project_update_status(&$project_data, $available) {
   +    catch (UnexpectedValueException $exception) {
   +      continue;
   +    }
   

   +1 for this; I had originally second-guessed my suggestion throwing an exception because of potential disruptions, but this mitigates it.

7. +++ b/core/modules/update/update.compare.inc
   @@ -274,10 +280,23 @@ function update_calculate_project_update_status(&$project_data, $available) {
   +      // If there are no valid support branches us the current major.
   

   "us the current major" indeed. We totally are!

   A comma's also needed, so:
   If there are no valid supported branches, use the current major.

#91 does not appear to be addressed yet.

This would actually would fail for to specific version numbers.

dev snapshot 8.x-8.x-dev because it has 2 8.x- that would be replaced.

dev snapshot 8.x-dev in this case 8.x- is actually not a core prefix but a snapshot release for support branch 8. branch

Nice catch on that!

1. +++ b/core/modules/update/src/ModuleVersion.php
   @@ -40,23 +40,34 @@ final class ModuleVersion {
   +          throw new \UnexpectedValueException("Unexpected version core prefix in $version_string. The only core prefix expected in \Drupal\update\ModuleVersion is '8.x-.");
   
   +++ b/core/modules/update/tests/src/Unit/ModuleVersionTest.php
   @@ -194,59 +131,233 @@ public function providerVersionInfos() {
   +    $this->expectExceptionMessage("Unexpected version core prefix in $version_string. The only core prefix expected in \Drupal\update\ModuleVersion is '8.x-.");
   

   Nitpick here, but we have an allowed value at the end of the string, related to a different value that can end in a literal .. Can we end this and any such exception messages with something like:
   The only core prefix expected... is: 8.x-

   That way we don't need to put a period at the end grammatically and we don't confuse sentence-ending periods with the literal . that we have to deal with also from the update XML.

2. +++ b/core/modules/update/src/ModuleVersion.php
   @@ -40,23 +40,34 @@ final class ModuleVersion {
   +      throw new \UnexpectedValueException("Unexpected version number in $original_version.");
   

   Same suggestion here, "...in: $var" rather than "...in $var."

+++ b/core/modules/update/tests/src/Unit/ModuleVersionTest.php
@@ -194,59 +131,233 @@ public function providerVersionInfos() {
+  /**
+   * @covers ::createFromSupportBranch
+   *
+   * @dataProvider providerCreateFromSupportBranch
+   */
+  public function testCreateFromSupportBranch($branch, $expected_major) {
...
+  /**
+   * Data provider for providerCreateFromSupportBranch().
+   */
+  public function providerCreateFromSupportBranch() {

(Here and elsewhere.) If we add parameter documentation for the test method (i.e., explaining each key passed from the provider), that goes a long ways to making the strings of numbers more clear in terms of what's being tested. Inline comments in the provider for each case can help too.

NW for #91 and #97.

Fixes nits in #91 and #97.

I last RTBC'd this in #73.
Reviewed every comment and response since then, only two bits of remaining work to be done:

#85.3
Missing examples on docs of \Drupal\update\ModuleVersion::$versionExtra

+++ b/core/modules/update/tests/src/Unit/ModuleVersionTest.php
@@ -277,6 +280,12 @@ public function providerInvalidBranchCorePrefix() {
+   * @param string $branch
+   *   The branch to test.
+   *
+   * @param string $expected_major
+   *   The expected major version.

Extra blank line here, and only 2 of the 7 methods had the docs added.
Additionally, none of the data providers document themselves. See \Drupal\Tests\layout_builder\Unit\OverridesSectionStorageTest::providerTestExtractEntityFromRoute() for an example.

With those two docs changes made, I am comfortable RTBCing this.

Thanks Ted!
This is ready to go

Committed and pushed to 9.0.x and 8.9.x, yay!

I think we might want to backport this even further, so setting to PTBP while I discuss with @catch.

@catch and I agreed to backport this (and it'd be great to do before 8.8.2); however, we need to backport #3096078: Display core compatibility ranges for available module updates first, which in turn needs to have its usability issues resolved before being backported to a production release: #3102724: Improve usability of core compatibility ranges on available updates report. So postponing for now.

Re #107, #3102724: Improve usability of core compatibility ranges on available updates report has not actually been committed to Drupal 8.8 yet. Which one depends on which one?

Ok that makes sense given that #3102724: Improve usability of core compatibility ranges on available updates report is a reroll away. Reroll-party! Thanks!

Since this is an important change and could have disruptive impacts compared to a typical patch release, it should be mentioned in the release notes.