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
- 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.
- versioncontrol_release needs to alter the release node form to set values for all the version element fields, not just the full version string.
- 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.
- "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:
/**
* 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:
Comment | File | Size | Author |
---|---|---|---|
#6 | 2137201_project_release_drush_3.patch | 764 bytes | greggles |
Comments
Comment #1
moshe weitzman CreditAttribution: moshe weitzman commentedI'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.
Comment #2
tvn CreditAttribution: tvn commentedComment #3
drummIn
project_release.drush.inc
'sproject_release_history_generate_project_xml()
:I think the if condition should be changed to
!empty()
, as long as that works without ever throwing notices.Comment #4
moshe weitzman CreditAttribution: moshe weitzman commentedScor 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
Comment #5
David_Rothstein CreditAttribution: David_Rothstein commentedWhat 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.
Comment #6
gregglesComment #7
gregglesNow 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.
Comment #8
gregglesOne 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.
Comment #9
David_Rothstein CreditAttribution: David_Rothstein commentedThe 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.
Comment #10
scor CreditAttribution: scor commentedI 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.
Comment #11
Josh The Geek CreditAttribution: Josh The Geek commentedFixed 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
Comment #12
David_Rothstein CreditAttribution: David_Rothstein commentedMaybe related in some way: #2140621: Invalid version type for 7.24 and 7.23 error when installing via drush
Comment #13
joachim CreditAttribution: joachim commentedHere'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.
Comment #14
tintoI have the exact same problem here.
Specifying the most recent version seems to have resolved this issue for me:
drush up drupal-7.24
Comment #15
rychannel CreditAttribution: rychannel commentedThis 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.
Comment #16
drummWhat changes to the XML are needed?
Comment #17
dwwSorry 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.
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedI 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):
The 7.19 and earlier releases DO have version_patch:
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.
Comment #19
dwwExcellent, 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:
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
Comment #20
drummproject_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.Comment #21
dwwCore 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.
Comment #22
dwwOkay, 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.
Comment #23
drummMaybe this is what to use:
From http://us2.php.net/manual/en/regexp.reference.repetition.php.
In our code, I think that would be adding the
+
in
project_release_parse_version()
.Comment #24
drummI 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.Comment #25
dwwHrm, 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. ;)
Comment #26
dwwHad 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...
Comment #27
dwwAll 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..
Comment #28
dwwNote: 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
Comment #29
dwwBoth 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.
Comment #30
dwwAll 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.
Comment #31
drummComment #32
dwwAdded #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).
Comment #33
dww#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
Comment #35
drummThis 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:
Comment #36
drummI 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.
Comment #37
drummShould be all fixed again.
(And gave me a chance to run into #2173779: Update DrupalorgVersioncontrolLabelVersionMapperGit for semantic versioning in Drupal 8.)