[META] Regressions in project_release handling of version element fields and release-history XML

I'm open to changes in Drush to better identify stable releases. In the meanwhile, I think this needs to change on the drupal.org side. We have thousands of Drush clients out there who will upgrade very slowly.

In project_release.drush.inc's project_release_history_generate_project_xml():

      foreach (array('major', 'minor', 'patch', 'extra') as $type) {
        $vers_type = 'version_' . $type;
        if (isset($release->{'field_release_' . $vers_type}[$release->language][0]['value'])) {
          $output .= "  <$vers_type>" . check_plain($release->{'field_release_' . $vers_type}[$release->language][0]['value']) . "</$vers_type>\n";
        }

I think the if condition should be changed to !empty(), as long as that works without ever throwing notices.

Scor mentions that the update module in core is also affected by this. That is, we pick the wrong release there too. Pretty bad since we just had a security release of 7.24 Bumping priority

What seems to be happening with Update Status (based on some testing by several of us in IRC):

-If you are running Drupal 7.23 it correctly tells you to update to Drupal 7.24.
-If you are running Drupal 7.19 it tells you to update to Drupal 7.19 (?!).

Looking at http://updates.drupal.org/release-history/drupal/7.x (at least as this is being written), "version_extra" appears in Drupal 7.20 through 7.24 currently, so it's consistent with somehow causing the above.

Now fixed with http://drupalcode.org/project/project.git/commit/8437736
and deployed with http://localhost:8081/view/D.o/job/deploy_drupal.org_util/

And http://updates.drupal.org/release-history/drupal/7.x looks good

I'm going to leave this as open for now for review, but I think the bug is fixed.

One note: a project will only have good update xml data if the xml is rebuilt. That happens either manually by calling http://localhost:8081/view/D.o/job/update_release_history_feeds/ or as part of the packaging process. So, either ping me in irc to rebuild http://localhost:8081/view/D.o/job/update_release_history_feeds/ for your project to test this out OR...make a new release.

The Update Status module issue does still seem to be occurring:

With an install of Drupal 7.19, it's recommending Drupal 7.19.

With an install of Drupal 6.27, it's recommending Drupal 6.27.

Though it does correctly list the newer security updates also.

I can confirm the bug described by David is also happening on my local install:
7.18 recommends 7.19 - drush upc upgrades to 7.19
7.19 recommends 7.19 - drush upc upgrades to 7.19
7.20 recommends 7.24 - drush upc upgrades to 7.24

It seems Greggles' patch fixed part of the issue though, since drush dl drupal now downloads 7.24.

Fixed affected project links

Also, Drush issues https://github.com/drush-ops/drush/issues/268 and https://github.com/drush-ops/drush/issues/271 are caused by this

Maybe related in some way: #2140621: Invalid version type for 7.24 and 7.23 error when installing via drush

Here's how it looks on D6:

Core's update status reports 6.27 as the recommended version when I'm already on 6.27, but shows 'Latest version 6.29' in the list of other versions below.

Drush says that 6.27 needs an update, but offers to update it to 6.27.

I can confirm the bug described by David is also happening on my local install:
7.19 recommends 7.19 - drush upc upgrades to 7.19

I have the exact same problem here.

Specifying the most recent version seems to have resolved this issue for me:

drush up drupal-7.24

This isn't just affecting Drush. It also has Drupal's built in Update Manager screwed up. I have several sites reporting the Drupal 7.19 is the latest version.

What changes to the XML are needed?

Sorry for being late to the party.

One quick note about how #6 is broken -- that patch results in a missing <version_patch> entry for contrib releases like 7.x-2.0 or core releases like 7.0. We need a different way to test for this. Either special-case version_extra to use the !empty(), or special-case version_patch to use !isset() (or something).

Meanwhile, I don't know what's causing core/drush to get confused about 7.19 being the most recent release. A very quick look at the live XML, based on my (rusty) memory of how Update Manager figures out WTF is going on, and it seems like the current XML should be okay for Update Manager to work. I'd have to walk through the update manager code as it's trying to decide what to do and see where things are going wrong.

I took a step through the update_calculate_project_update_status() and what I am seeing is that 7.24 and other recent releases are not surviving this if() check due to a missing version_patch element. The XML for drupal-7.24 release now looks as follows (see for yourself at http://updates.drupal.org/release-history/drupal/7.x):

<name>drupal 7.24</name>
<version>7.24</version>
<tag>7.24</tag>
<version_major>7</version_major>
<version_minor>24</version_minor>
<status>published</status>
<release_link>http://drupal.org/drupal-7.24-release-notes</release_link>
<download_link>http://ftp.drupal.org/files/projects/drupal-7.24.tar.gz</download_link>
<date>1384982905</date>
<mdhash>c1ddb37155e4b6160f6508636c06f2a7</mdhash>
<filesize>3195735</filesize>

The 7.19 and earlier releases DO have version_patch:

<name>drupal 7.19</name>
<version>7.19</version>
<tag>7.19</tag>
<version_major>7</version_major>
<version_patch>19</version_patch>
<status>published</status>
<release_link>http://drupal.org/drupal-7.19-release-notes</release_link>
<download_link>...</download_link>
<date>1358374871</date>
<mdhash>c1dd3960f1555df208c80ef612e0c53a</mdhash>
<filesize>3163130</filesize>

If we had a copy of an Update XML from before the upgrade we could verify my hypothesis. In any case, I feel pretty good that this missing version_patch element is the root of this issue.

Excellent, thanks! Yeah, that totally makes sense to me. Probably the release nodes themselves have the wrong value for these hidden version element fields. A few quick queries on d.o about Drupal core 7.24 confirms this:

mysql> select field_release_version_minor_value from field_data_field_release_version_minor where entity_id = 2140383;
+-----------------------------------+
| field_release_version_minor_value |
+-----------------------------------+
|                                24 | 
+-----------------------------------+
1 row in set (0.00 sec)

mysql> select field_release_version_patch_value from field_data_field_release_version_patch where entity_id = 2140383;
+-----------------------------------+
| field_release_version_patch_value |
+-----------------------------------+
|                              NULL | 
+-----------------------------------+
1 row in set (0.00 sec)

There's presumably a bug in the project_release code related to this, or perhaps a problem during migration, or both. But thanks, this is a great start towards finding a solution here!

That said, we definitely still need to fix the generation script to handle when either version minor or version patch are 0. That's a totally legit value and we need to include the corresponding field in the XML.

Cheers,
-Derek

project_release_parse_version() parses version numbers. Core's version format is !major%minor%patch#extra.

The % means the part is preceded by a .. The constructed regexp is matching for patch instead of minor. Maybe the optional groups can be made more greedy.

Core shouldn't be using !major%minor%patch#extra (until the semver stuff happens, if ever). Core should be this:

!major%patch#extra

In D6, there was a per-project config knob for this. Not sure if that got lost in the D7 porting.

Okay, yeah, it's still there on the core project edit page, under the releases vtab:

https://drupal.org/node/3060/edit

Not sure the implications of changing that, in terms of how to get all the release nodes to re-save themselves with all the proper version fields.

Maybe this is what to use:

Quantifiers followed by + are "possessive". They eat as many characters as possible and don't return to match the rest of the pattern. Thus .*abc matches "aabc" but .*+abc doesn't because .*+ eats the whole string. Possessive quantifiers can be used to speed up processing.

From http://us2.php.net/manual/en/regexp.reference.repetition.php.

In our code, I think that would be adding the +

$regexp .= '))?+';

in project_release_parse_version().

I think !major%minor%patch#extra may be correct since we had versions like 4.7.3. Really, we should check test-drupal.redesign.devdrupal.org, which is D6.

Hrm, strange. Yeah, test-drupal.redesign.devdrupal.org has the same !major%minor%patch#extra and yet 7.23 correctly has major: 7, minor: NULL, patch: 23. So something clearly changed in the behavior of how all this works in D7.

And yeah, I was thinking about 4.7.x releases. However, they're all unpublished at this point, so as a (potentially quick) work-around, I think we could safely take out the %minor for now. We'd still have to get all the release nodes to re-consider themselves and update their hidden fields appropriately. Not sure the best way to do that.

But, it's going to re-become a problem if 8.0(.0)? ships with all 3 digits. So, perhaps it's better to somehow fix this in the general case now.

Perhaps the real problem lies at the drupalorg_versioncontrol side of the world? In D6, versioncontrol_release was responsible for parsing the Git tag and figuring out the proper version elements, those were then composed into the final human-visible version string using this version format string, and finally everything was stuffed into the release node. This original Git tag -> version mapping was handled by plugins, and drupalorg_versioncontrol implemented a plugin with all the special-cases for d.o tags and version strings in all their ugly glory. Is that reversed in D7? Honestly, I'm not clear why project_release is parsing version strings at all. Maybe I can find drumm on IRC and we can hash this out in real-time.

Stay tuned. ;)

Had a chance to chat with drumm about all of this over IRC. There are in fact a whole bunch of inter-related problems here. I'm converting this issue into a meta issue, and did a massive update to the summary to explain the problem and the proposed solution. In a little while, I'll submit a bunch of child issues for the various parts, and when those are all existing, I'll update the summary to add the remaining tasks section. Stay tuned...

All the sub-tasks are now separate issues in the right queues. I've filled in the Remaining tasks section of the summary with everything, in the order they mostly need to be done. In particular, #2145667: project_release_node_presave() should only parse the version string if the version elements are not already set is blocking both #2145679: Manually repair version elements in broken core releases and #2145675: Alter the release node form to set values for the version element fields as determined by the label_version_mapper plugin (which in turn blocks #2145677: Ensure DrupalorgVersioncontrolLabelVersionMapperGit.class.php is working properly in D7).

Also note that my objection to patch #6 from comment #17 now lives at #2145681: Fix release history XML generation to consider 0 a valid value for various version elements..

Note: I just uploaded patches for both of these:

#2145667: project_release_node_presave() should only parse the version string if the version elements are not already set
#2145675: Alter the release node form to set values for the version element fields as determined by the label_version_mapper plugin

It's a bit ugly, since I couldn't figure out how to get the values on the version subfields to "stick" if I set them via a form alter. Those subfields are all configured to use a hidden widget by default, so that appears to change how the field form elements behave. So, I got it working via a slightly hacky approach. If anyone has a better suggestion, I'm all ears. However, local testing seemed to work well, so I'm inclined to call this Good Enough(tm) to resolve the critical bug.

That said, reviews very welcome! I've put both patches on dww7-drupal.redesign.devdrupal.org if anyone wants to try testing there.

Thanks,
-Derek

Both of these are now fixed and deployed live:

#2145667: project_release_node_presave() should only parse the version string if the version elements are not already set
#2145675: Alter the release node form to set values for the version element fields as determined by the label_version_mapper plugin

In testing those, I decided #2145677: Ensure DrupalorgVersioncontrolLabelVersionMapperGit.class.php is working properly in D7 is working nicely, too.

So, the main thing that remains here is #2145679: Manually repair version elements in broken core releases which I just assigned to myself.

All child issues are now fixed, deployed and confirmed.

Although I manually triggered a rebuild of the core release history XML feeds (since that was the most urgent and confusing problem) not every project that was harmed by stopgap patch #6 and #2145681: Fix release history XML generation to consider 0 a valid value for various version elements. will be fixed until the feeds rebuild themselves (which is basically happening all the time these days). In theory, within about 6 hours, everything should be resolved, so I'm going to call this fixed.

Added #2150865: Saving release notes makes release disappear from project feed and project page release table since that's now killing us (and perhaps made worse after the other changes in here).

#2150865: Saving release notes makes release disappear from project feed and project page release table is fixed and deployed, so I think this is fixed again. Hopefully for good this time!

-Derek

This is still appearing for new core releases.

dww's solution works well for node forms. I think the version number is reset when release packaging programmatically saves the release node. On a dev site, the version number changes with:

$release = node_load(2060023);
node_save($release);

I think http://drupalcode.org/project/project.git/commit/def0f5b will fix this for future releases. Deploying and manually cleaning up 7.24 and 7.25.

Should be all fixed again.

(And gave me a chance to run into #2173779: Update DrupalorgVersioncontrolLabelVersionMapperGit for semantic versioning in Drupal 8.)