Problem/Motivation

Ever since the d.o D7 upgrade, certain core release nodes (basically, any that have been created or edited) are now getting their version element fields parsed improperly, such that we're setting version_major and version_minor instead of version_major and version_patch. The root of the problem is that historically, core has had both 3 and 2 digit version strings. So, the version format string is set to !major%minor%patch#extra. In the current D7 implementation of project_release, versioncontrol_release and friends, instead of parsing the Git tags via custom, site-specific code that can special case this sort of thing, we're just letting the git tag basically become the full version string directly, and then trying to parse that into the version elements. Since the parsing is greedy, we always match the first two elements (!major and %minor), whereas for core, we really need to match !major and %patch.

These faulty values in the release nodes get propagated into the release history XML feeds, which in turn confuse all the clients in the wild that were written to use those feeds (principally, the Update manager in Drupal core, and various parts of the drush). This results in bogus behavior of various sorts.

Additionally, the original release history XML feeds were including a <version_extra> entry for releases that didn't have any version_extra value, which further confused the clients of these feeds.

Proposed resolution

  1. project_release_node_presave() needs to be changed so it does not parse the full version string into the component elements if the components are already set.
  2. versioncontrol_release needs to alter the release node form to set values for all the version element fields, not just the full version string.
  3. Ensure that the drupalorg_versioncontrol plugin is still working properly and parsing the Git tags into the right components in a way that versioncontrol_release gets the right values.
  4. "Manually" repair the broken release nodes (currently, 13 of them, all from core).

Remaining tasks

#2145667: project_release_node_presave() should only parse the version string if the version elements are not already set (blocks other issues, must happen first)
#2145675: Alter the release node form to set values for the version element fields as determined by the label_version_mapper plugin
#2145677: Ensure DrupalorgVersioncontrolLabelVersionMapperGit.class.php is working properly in D7
#2145679: Manually repair version elements in broken core releases
#2145681: Fix release history XML generation to consider 0 a valid value for various version elements.
#2150865: Saving release notes makes release disappear from project feed and project page release table

Original report by znerol

Minor changes of the XML feed format for project releases influence the way drush pm-download picks the best stable release. Before the D7 upgrade, the version_extra key never was included into a release-entry in the XML feed for stable releases. The selection mechanism implemented in drush relies on this fact:

See commands/pm/release_info/updatexml.inc:

<?php
/**
 * Given a list of candidate releases, return the best one.
 * This will be the first stable release if there are stable
 * releases; otherwise, it will be any available release.
 */
function updatexml_best_release_found($releases) {
 
// If there are releases found, let's try first to fetch one with no
  // 'version_extra'. Otherwise, use all.
 
if (!empty($releases)) {
   
$stable_releases = array();
    foreach (
$releases as $one_release) {
      if (!
array_key_exists('version_extra', $one_release)) {
       
$stable_releases[] = $one_release;
      }
    }
    if (!empty(
$stable_releases)) {
     
$releases = $stable_releases;
    }
  }
?>

However releases packaged or edited after the D7 upgrade now have empty version_extra keys for stable releases. This results in drush pm-download selecting outdated releases.

Affected projects:

Files: 
CommentFileSizeAuthor
#6 2137201_project_release_drush_3.patch764 bytesgreggles

Comments

moshe weitzman’s picture

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.

tvn’s picture

Issue tags:+Drupal.org 7.1
drumm’s picture

Project:[Archive] Drupal.org D7 upgrade QA» Project
Version:» 7.x-2.x-dev
Component:Code» Releases

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

<?php
     
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.

moshe weitzman’s picture

Title:drush pm-download picks wrong release» new version_extra element in update xml since d.o upgrade (was drush pm-download picks wrong release)
Priority:Normal» Critical

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

David_Rothstein’s picture

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.

greggles’s picture

StatusFileSize
new764 bytes
greggles’s picture

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.

greggles’s picture

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.

David_Rothstein’s picture

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.

scor’s picture

Status:Active» Needs work

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.

Josh The Geek’s picture

Issue summary:View changes

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

David_Rothstein’s picture

joachim’s picture

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.

tinto’s picture

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

rychannel’s picture

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.

drumm’s picture

What changes to the XML are needed?

dww’s picture

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.

moshe weitzman’s picture

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.

dww’s picture

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

drumm’s picture

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.

dww’s picture

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.

dww’s picture

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.

drumm’s picture

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 +

<?php
$regexp
.= '))?+';
?>

in project_release_parse_version().

drumm’s picture

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.

dww’s picture

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. ;)

dww’s picture

Title:new version_extra element in update xml since d.o upgrade (was drush pm-download picks wrong release)» [META] Regressions in project_release handling of version element fields and release-history XML
Issue summary:View changes
Status:Needs work» Active

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...

dww’s picture

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

dww’s picture

Assigned:Unassigned» dww
Issue summary:View changes
Status:Active» Fixed

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.

drumm’s picture

Status:Active» Fixed
dww’s picture

Issue summary:View changes
Status:Fixed» Active

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).

dww’s picture

Status:Active» Fixed

#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

Status:Fixed» Closed (fixed)

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

drumm’s picture

Assigned:Unassigned» drumm
Priority:Critical» Major
Status:Closed (fixed)» Active

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:

<?php
$release
= node_load(2060023);
node_save($release);
?>
drumm’s picture

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.

drumm’s picture

Status:Active» Fixed

Status:Fixed» Closed (fixed)

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