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 -- Irrelevant to this feature, which is specifically for projects not on drupal.org.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.
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
Comment | File | Size | Author |
---|---|---|---|
#3 | 3105353-3.patch | 787 bytes | tedbow |
Comments
Comment #2
tedbowComment #3
tedbowComment #4
tedbowComment #5
dwwThis 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
Comment #6
joachim CreditAttribution: joachim commentedIt'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!
Comment #7
dww+1 to removing this / RTBC. I made a few summary updates, though, to be more accurate for when a core committer sees this.
Comment #8
dwwComment #9
catchWhile this is a very obscure feature, we should still have a change record for removing it.
Comment #10
dwwhttps://www.drupal.org/node/3129456
Needs the version info filled in (depending on how far back this gets backported) when published.
Thanks,
-Derek
Comment #12
xjmSo let's say your site has a custom or GitHub-hosted module installed with version
83
orelephant
or something that doesn't match the regex, and is using thismajor
key to specify their API version. Then suddenly this patch is committed and their major version turns into-1
. What happens to their site?Comment #13
dwwTL;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:
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
, andproject 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
Comment #14
xjmOK, #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?
Comment #15
dwwRe: #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
Comment #18
catchI 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!
Comment #19
tedbow🎉For small victories! Thanks everyone!