As part of #2688479: Update update status XML for Drupal 9 and contrib semantic versioning, I’d like to see if we can remove the <version_major>, <version_minor>, <version_patch>, and <version_extra> tags from update status XML. These version number components are redundant with <version>.

packages.drupal.org treats 8.x-1.2 as major = 1, minor = 2. Update status XML omits version_minor and version_patch = 2. To support semantic versioning for contrib, #2681459: Support contrib semver releases, we might want to migrate version_patch → version_minor, so this lines up all the way down.

CommentFileSizeAuthor
#11 3085717-11.patch83.11 KBtedbow
#11 interdiff-9-11.txt1.91 KBtedbow
#9 3085717-9.patch83.04 KBtedbow
#9 interdiff-2-9.txt10.62 KBtedbow
#8 3085717-8.patch83.02 KBtedbow
#8 interdiff-2-8.txt10.6 KBtedbow
#2 3085717.patch72.41 KBdrumm

Comments

drumm created an issue. See original summary.

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Active » Needs review
StatusFileSize
new 3085717.patch72.41 KB

This simply removes the tags from the test fixtures. Looking at the code, I fully expect this to fail. Let’s see how badly.

Status: Needs review » Needs work

The last submitted patch, 2: 3085717.patch, failed testing. View results

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

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

dww’s picture

dww
we/he/they
commented

It's been *ages* since I looked at update.module code, but when I first wrote it, it was heavily tied to these separate tags. The fact we only see 6 failures suggests we don't have very thorough test coverage of update.module, not that this isn't that big of a breaking change. ;)

drumm’s picture

drumm
he/him
Location NY, US
commented
Related issues: +#3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates

As far as I could tell:

  • The tests are only testing version_major, it is used.
  • version_minor is not used.
  • version_patch and version_extra are used, but not tested.

I think the most productive direction for this issue would be adding testing for the version_patch and version_extra bits, and replace using them.

For example, empty($release['version_extra']) could use https://github.com/composer/semver/blob/master/src/VersionParser.php#L51: VersionParser::parseStability($version) === 'stable'

(version_major is tied up in supported/recommended major versions, so that’s a whole other project for #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates or another related issue.)

drumm’s picture

drumm
he/him
Location NY, US
commented
Assigned: drumm » Unassigned

I don’t think I am actively working on this.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs work » Needs review
Issue tags: +Needs tests
StatusFileSize
new interdiff-2-8.txt10.6 KB
new 3085717-8.patch83.02 KB

Here is patch that replaces checks for version_major, version_patch, and version_extra with calls to methods on a new value object class \Drupal\update\ProjectInfo

It looks like version_extra is tested in \Drupal\Tests\update\Functional\UpdateCoreTest::testNormalUpdateAvailable() but we should test also in UpdateContribTest

We should have failing tests for the changes first

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
StatusFileSize
new interdiff-2-9.txt10.62 KB
new 3085717-9.patch83.04 KB

forgot @group for the new test

Status: Needs review » Needs work

The last submitted patch, 9: 3085717-9.patch, failed testing. View results

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-9-11.txt1.91 KB
new 3085717-11.patch83.11 KB 
+++ b/core/modules/update/src/ProjectInfo.php
@@ -0,0 +1,84 @@
+  public function getVersionExtra() {
+    $version_parts = explode('.', $this->getVersionString());
+    $last_version_parts = explode('-', count($version_parts) === 2 ? $version_parts[1] : $version_parts[2]);
+    return count($last_version_parts) === 1 ? NULL : $last_version_parts[1];

+++ b/core/modules/update/tests/src/Unit/ProjectInfoTest.php
@@ -0,0 +1,120 @@
+          'extra' => '',
...
+          'extra' => '',

I originally had getVersionExtra() returning a empty string if none available but changed it to use NULL. I didn't update the tests or the @return type.

Fixing

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Title: Do not rely on version_* tags » [PP-1] Do not rely on version_* tags
Status: Needs review » Postponed
Related issues: +#3094304: Create tests that cover contrib non-full releases and contrib patch versions

I have created #3085717: [PP-1] Do not rely on version_* tags so that we can first create the test coverage that we are missing

I think we should not add the test coverage for existing functionality and change how it is implement in the same issue.

This is similar to where we broke out the missing tests from #2804155: If the next minor version of core has a security release, status still says "Security update required!" even if the site is on an equivalent, secure release already and first did them in their own issue #2990511: Add comprehensive test coverage for update status for security releases

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Postponed » Closed (duplicate)

We did this in #3074993: Use "current" in update URLs instead of CORE_COMPATIBILITY to retrieve all future updates. It now needs a backport to 8.8.x