Problem/Motivation

In update.compare.inc in update_process_project_info() there is

if (preg_match('/^(\d+\.x-)?(\d+)\..*$/', $info['version'], $matches)) {
  $info['major'] = $matches[2];
}
elseif (!isset($info['major'])) {
  // This would only happen for version strings that don't follow the
  // drupal.org convention. We let contribs define "major" in their
  // .info.yml in this case, and only if that's missing would we hit this.
  $info['major'] = -1;
}

This was added in the update_status module when it was in contrib in #149918: add support for "Also available" entries in the status report in 2007, over 12 years ago 🙀. It basically will look for a major: key in the project info if it is not able to determine the major from the version number.

I don't think this functionality is needed anymore, probably not for a long time. I searched I didn't find any info.yml files that have a major: key. Although searching doesn't tell us anything, since this is about projects not hosted on d.o...

For this to work drupal.org would have to also be parsing info.yml file for the major: key to set version_major in the update XML. Otherwise otherwise the update calculations in update_calculate_project_update_status() would not work when evaluating releases for the project. -- Irrelevant to this feature, which is specifically for projects not on drupal.org.

In #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates we will be switching to an XML feed that does not have versIon_major so the major version will be determined by the version string. So local modules should also be treated the same way.

And thanks to #3118087: If any extension has a missing or invalid version, Update manager throws errors and is confused about site update status Update Manager now flags projects with invalid version strings and basically ignores them. So there's no way anything (even a custom module) could be relying on 'major' now, since they have to have either SemVer or BespokeVer version strings for update manager to do anything with the project.

Proposed resolution

Remove this logic. Do not check $info['major'] before setting it to -1 if unable to determine the major version from the version string.

Remaining tasks

User interface changes

None

API changes

This wasn't really an API

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

tedbow’s picture

Issue summary: View changes
tedbow’s picture

tedbow’s picture

Status: Active » Needs review
dww’s picture

This might be worth ripping out, but I don't agree with the analysis in the summary.

This code was added to support contrib modules *not on d.o in the first place*. So it doesn't matter what d.o supports or not. This is for projects that define their own release history URL, etc. Years ago, that was a thing. I doubt anyone is still doing that, but it's basically impossible to know. Grepping d.o's Git repos won't tell us. /shrug

joachim’s picture

Status: Needs review » Reviewed & tested by the community

It's not even documented: https://www.drupal.org/docs/8/creating-custom-modules/let-drupal-8-know-...

Then again, the ability to use 'project status url' in the info.yml file to host a contrib project somewhere other than d.org isn't documented either!

dww’s picture

Issue summary: View changes

+1 to removing this / RTBC. I made a few summary updates, though, to be more accurate for when a core committer sees this.

dww’s picture

catch’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs change record

While this is a very obscure feature, we should still have a change record for removing it.

dww’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

https://www.drupal.org/node/3129456

Needs the version info filled in (depending on how far back this gets backported) when published.

Thanks,
-Derek

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

xjm’s picture

So let's say your site has a custom or GitHub-hosted module installed with version 83 or elephant or something that doesn't match the regex, and is using this major key to specify their API version. Then suddenly this patch is committed and their major version turns into -1. What happens to their site?

dww’s picture

Issue summary: View changes

TL;DR:

Thanks to #3118087: If any extension has a missing or invalid version, Update manager throws errors and is confused about site update status it's impossible to need a major key, since Update Manager can only handle version strings that it can parse (either SemVer or BespokeVer).

Long version:

Nothing happens to their site, since Update Manager doesn't have a 'project' key to use. Therefore, Update Manager ignores the custom project. It doesn't show up on their available updates report at all, and never has.

Or the custom module is defining 'project', too, Now it shows up as "Invalid version: Unknown", since there's no version key, either.

Or, they define "version" themselves to 83, and then (thanks to #3118087: If any extension has a missing or invalid version, Update manager throws errors and is confused about site update status) it shows up as:

custom 83                    Invalid version: 83
Includes: custom

Or maybe they define version: VERSION, and reuse core's version numbers. Now it looks like this:

custom 8.8.6-dev No available releases found

That's because when Update Manager queries updates.d.o for release history, it fails because there's no d.o project that matches 'custom'. Unless they define 'project' to point to a real d.o project, in which case, WTF? Not our problem. ;) PEBKAC.

Or, they define a parsable version, project, and project status url and are running their own update release history server (#3120655: Determine if there are other update servers besides updates.drupal.org, if not deprecate the ability to have different servers) in which case they're a) in the 0.000001% use case, and b) they still don't need 'major' since the version string has to include a major version for the Update Manager to parse it.

Cheers,
-Derek

xjm’s picture

Status: Reviewed & tested by the community » Needs review

OK, #13 makes sense, thanks!

However, since we're now after 9.0.0, we should do this in 9.1.x.

Can we raise a deprecation warning if the module does set this key?

dww’s picture

Status: Needs review » Reviewed & tested by the community

Re: #14: We could. But I thought the point of this issue was to remove dead code, not add more of it. ;)

Also, maybe something else looks for a 'major' key and cares? I don't know of any other place in core that inspects .info.yml files and flags unexpected values. I thought folks were free to put anything they want in there, so long as the values core cares about are accurate. Seems like a dangerous precedent to be flagging unknown .info.yml values.

The thing we know is that we no longer need Update manager to care about this, so let's just remove the code for that and be done.

Thanks,
-Derek

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

  • catch committed e66a386 on 9.2.x
    Issue #3105353 by tedbow, dww, xjm, joachim: Remove the ability for...
catch’s picture

Status: Reviewed & tested by the community » Fixed

I think #15 is a reasonable answer. We allow arbitrary .info.yml keys, and this is one that currently doesn't do anything useful, the patch just makes the behaviour more consistent when it's set. If it was previously doing something useful and/or removing the code was going to change behaviour more significantly, then we should add a deprecation message, but here we're relinquishing ownership formally of something that was de-facto relinquished a long time ago.

Committed e66a386 and pushed to 9.2.x. Thanks!

tedbow’s picture

🎉For small victories! Thanks everyone!

Status: Fixed » Closed (fixed)

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