Posted by dww on July 28, 2009 at 5:09pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | task |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
Follow-up from #211747: Allow specifying version information as part of module dependencies...
That patch does not properly support versions with "extra": alpha, beta, rc, etc. We need to add some more code to handle this.
Also, I'm pretty sure PHP's version_compare() is going to barf on "unstable", which is another allowed "extra" identifier on version strings which is a drupalism. Not sure if we care, but perhaps we'll need to special-case that, and if we see "unstable" to try to do our own version_compare() logic. That'd be a bit of a drag, but probably worth it.
Comments
#1
#211747: Allow specifying version information as part of module dependencies
#2
In the other comment (which I'm going to unpublish due to confusion), Paul wrote:
Re: points B and C: read http://www.debian.org/doc/debian-policy/ch-controlfields.html#s-f-Version. It seems here it would work to interpret an empty part as zero ("2.4" is "2.4-0") and ensure a numerical part sorts later (i.e. is considered newer than) a part beginning with a letter ("alpha1" or "beta1").
Thus, in proper sort order:
2.4-beta1
2.4-rc1
2.4 (as "2.4-0")
2.5+cvs20090728 (as "2.5+cvs20090728-0"), a dev release. This is seen frequently in Debian & co.
2.5-alpha1 (as "2.5+0-alpha1")
2.5 (as "2.5-0")
...etc., so that:
#4
#5
PHP's version_compare() already handles extra identifiers in version numbers properly.
The only issue we'd have to tackle here is to transform branch versions, i.e.
2.x, into a comparable version number for version_compare(), because2.xis interpreted completely different than2.1-beta2. The most trivial and most dirty way would be to transform2.xinto2.999internally for comparison. That way:2.x >= 2.0 === FALSE2.999 >= 2.0 === TRUE
2.999 >= 2.0-beta1 === TRUE
2.0-beta1 >= 2.0-alpha === TRUE
2.0-beta2 >= 2.0-beta1 === TRUE
#6
sun, we already have code to handle 2.x -- and yes it can be changed so that preg is not necessary but that's a whole another issue -- and there is no 2.x-beta or some such so your followup does not pertain to this issue.
#7
testing version compare for arbitrary strings:
<?php$a = '2.3-foo2';
$b = '2.3-foo1';
var_dump(version_compare($a, $b, '>'));
$a = '2.3-dev1';
$b = '2.3-foo1';
var_dump(version_compare($a, $b, '>'));
?>
both are TRUE.when using the same version string the comparison works well.
dev < alpha = a < beta = b < RC < plsays the documentation which can now amend asarbitrary string < dev < alpha = a < beta = b < RC < pl.Edit: I read the relevant C code and it converts these special strings to numbers so the order is:
any string not in the following list < dev < alpha = a < beta = b < RC = rc < # < pl = p#8
great. so "unstable" == "any string not in the following list", and it honors our (suggested) ordering of things. Of course, nothing forces module maintainers to go in order from unstable to alpha to beta to rc, but we can't do anything about that (beyond education). Looks like this should be quite trivial...
#9
Our version info is 6.x-2.x-dev, not 6.x-2.3-dev.
#10
This is not about -dev. Screw -dev. This is about official releases only.
#11
Needs tests. We had TONS of fun with version_compare.
#12
The last submitted patch failed testing.
#13
#14
The last submitted patch failed testing.
#15
#16
This looks good to me. Should we add a couple more tests? I.e. comparing rc1 and rc2, comparing unstable with alpha, etc?
#17
Yes, more tests would be very important, especially given how weird version_compare() is. ;) Should be trivial to add.
It's not clear to me why the special case to handle minor == 'x' only needs to deal with $op '>' and '<='. What about '>=' and '<'? Comment here would be useful, but probably a code comment explaining would be better.
#18
Derek, the >= and the < are dealt with the resetting of minor and extra. And there is a comment IMO which already explains that but I will check.
#19
@chx: I'm talking about this:
<?phpif ($matches['minor'] == 'x') {
// If a module is newer than 2.x then it's at least
// 3.0-unstable0.
$matches['minor'] = 0;
$matches['extra'] = 'unstable0';
if ($op == '>') {
$matches['major']++;
$op = '>=';
}
// If a module is older or equivalent than 2.x then it is older
// than 3.0-unstable0.
if ($op == '<=') {
$matches['major']++;
$op = '<';
}
// Equivalence is checked by preg.
if ($op == '=' || $op == '==') {
$value['versions'][] = array('preg' => '/^' . $matches['major'] . '\./');
$op = '';
}
}
?>
I don't see how that's going to properly handle (>= 2.x) nor (< 2.x). Can you explain?
Thanks!
-Derek
#20
Oh, I see now. But it's just a bit confusing. ;) Sorry.
Still needs work for more tests... Might not hurt to explain this code block better. I might re-roll later for that (but I can't right now).
Thanks,
-Derek
#21
More tests, more comments.
#22
This is a whole new approach because a question from Nick Lewis made me test 2.0 vs 2.x in version compare to find that numbers are higher than strings. Yay. Simplify++
#23
Now, that's some comment...
#24
The last submitted patch failed testing.
#25
Simplified comments -- we do not need a novel about internal implementation details. There is enough comment so that people can figure out if they want.
#26
Looks great, with one question.
What is the difference between the = and == operator? I couldn't find any docs on php.net on this, and not quite sure what it could be.
Since we allow this op, and special case it for .x version number, we should probably have a test for it.
Otherwise, RTBC
#27
Actually after looking at this, I'm not sure the == operator makes much sense. In the case of 2.x it is identical to =.
With specific versions (not .x branches), we require the point release in dependency strings anyway*, and packaged releases will always have it as well, and we don't allow patch level, so there is no need to say 2.0.0 vs. 2.0 aren't equal.
*(>=2) doesn't work, must be 2.x
#28
== and = are the very same. Why not allow both? version_compare allows both. There's not much point in testing that.
#29
That looks MUCH better.
I made some more minor polish to the comments and committed to HEAD.
It looks like mikey_p was looking at a different hunk of this file that the patch doesn't touch. We could open a separate issue for that.
#30
Automatically closed -- issue fixed for 2 weeks with no activity.