Topic: 2.0 RC5 Function matchPackageVersion(): only allows one interval

[davidhs]

Function matchPackageVersion() only allows one interval, i.e.

matchPackageVersion('3.0.5', '3.0, 3.0.4-3.0.6') => MATCH
matchPackageVersion('3.0.5', '3.0, 3.0.2-3.0.3, 3.0.4-3.0.6') => NO MATCH

Version SMF: 2.0 RC5 and lower
File: Sources/Subs-Package.php
Function: matchPackageVersion()
Line: 1468
Code:

Code: [Select]

$lower = explode('.', $lower);
$upper = explode('.', $upper);
$version = explode('.', $version);

foreach ($upper as $key => $high)
{
// Let's check that this is at or below the upper... obviously.
if (isset($version[$key]) && trim($version[$key]) > trim($high))
return false;

// OK, let's check it's above the lower key... if it exists!
if (isset($lower[$key]))
{
// The version either needs to have something here (i.e. can't be 1.0 on a 1.0.11) AND needs to be greater or equal to.
// Note that if it's a range lower[key] might be blank, in that case version can not be set!
if (!empty($lower[$key]) && (!isset($version[$key]) || trim($version[$key]) < trim($lower[$key])))
return false;
}
}
return true;Must be:

Code: [Select]

$lower_array = explode('.', $lower);
$upper_array = explode('.', $upper);
$version_array = explode('.', $version);

// Upper must be something, else version can not be set!
if (!$upper_array)
continue; // Continue in foreach $for.

foreach ($upper_array as $key => $high)
{
// Version must be something, else can not be set!
if (!isset($version_array[$key]))
continue 2; // Exit foreach $upper and continue in foreach $for.

// Let's check that this is at or below the upper... obviously.
if (trim($version_array[$key]) > trim($high))
continue 2; // Exit foreach $upper and continue in foreach $for.

// Lower must be something, else version can not be set!
// (i.e 1.0-1.0.3 not is valid; 1.0,1.0.1-1.0.3 is valid)
if (!isset($lower_array[$key]) || (empty($lower_array[$key]) && !is_numeric($lower_array[$key])))
continue 2; // Exit foreach $upper and continue in foreach $for.

// Let's check that this is at or upper the lower... obviously.
if (trim($version_array[$key]) < trim($lower_array[$key]))
continue 2; // Exit foreach $upper and continue in foreach $for.
}

// All keys of version match between lower and upper
return true;
Examples:

original matchPackageVersion('3.0.5', '3.0, 3.0.4-3.0.6') => MATCH
new matchPackageVersion('3.0.5', '3.0, 3.0.4-3.0.6') => MATCH

original matchPackageVersion('3.0.5', '3.0, 3.0.2-3.0.3, 3.0.4-3.0.6') => NO MATCH
new matchPackageVersion('3.0.5', '3.0, 3.0.2-3.0.3, 3.0.4-3.0.6') => MATCH

[Arantor]

Incidentally what happens if you use 3.0 RC2 - 3.0 RC4 rather than 3.0 RC2-3.0RC4 (noting the difference in spaces)?

[davidhs]

This is the same. It use trim() function to remove spaces.

i.e. you can write:
v1,v2    ,    v3    -v4
but no
v1, v2, v3-v4, v5-v6
(and this is correct!)

Edit: 2.0RC4 (without spaces) not is correct name of version). The correct is 2.0 RC4. trim() function only is used in text between version, not in name of version.

[Arantor]

Spaces in the middle of strings?

[davidhs]

Spaces in the middle of strings?

Like example above (I edit my last replay)

[Arantor]

That wasn't what I meant.

3.0 RC2 is not the same as 3.0RC2

[Yoshi]

3.0 RC# is not even out by far...

[Arantor]

3.0 RC# is not even out by far...

*shrug* It doesn't change how this function runs and thus the validity of this bug report. Pretend we said 2.0 RC for everything and it's just as valid.

[davidhs]

That wasn't what I meant.

3.0 RC2 is not the same as 3.0RC2

trim() function only is used in " , " and " - " like

Code: [Select]

$versions = "v1, v2, v3-v4";

$for = explode(',', strtolower($versions));
for ($i = 0, $n = count($for); $i < $n; $i++)
$for[$i] = trim($for[$i]);and in " . " of version (this only in SMF 1.1.12 and 2.0).

[Arantor]

Yes, I realise that. But 3.0 RC2 and 3.0RC4 are not the same, and no amount of trim() will fix it. So, please, do me a favour and retest as I asked you, using the same spacing in the middle of the string. Since I suspect that's possibly contributing to your bug...

[davidhs]

Yes, I realise that. But 3.0 RC2 and 3.0RC4 are not the same, and no amount of trim() will fix it. So, please, do me a favour and retest as I asked you, using the same spacing in the middle of the string. Since I suspect that's possibly contributing to your bug...

Now matchPackageVersion() only use trim() between version. i.e. in this string (s = spaces)
s2.0sRC1s,s2.0sRC2s-s2.0sRC4s
remove spaces
2.0sRC1,2.0sRC2-2.0sRC4
but
2.0sRC1
2.0sRC2
2.0sRC4
are correct name of versions (these name are in file DIR_FORUM/index.php , line 36

Code: [Select]

$forum_version = 'SMF 2.0 RC4';and $forum_version, without 'SMF ', is used for compare with string of versions in matchPackageVersion).

In my opinion 2.0RC4 (without space) or 2.0ssRC4 (with more than one space) are not correct name of version, but... could be improved to allow this. But it is not a bug.

[davidhs]

... could be improved to allow this.

This would be change line 1443 of same file

Code: [Select]

function matchPackageVersion($version, $versions)
{by

Code: [Select]

function matchPackageVersion($version, $versions)
{
$version = strtr($version, array(' ' => ''));
$versions = strtr($versions, array(' ' => ''));

[Arantor]

In my opinion 2.0RC4 (without space) or 2.0ssRC4 (with more than one space) are not correct name of version, but... could be improved to allow this. But it is not a bug.

Yes, but that's what your code had that was causing a failure. If you change it to what I suggested, does it work properly?

[davidhs]

Yes, but that's what your code had that was causing a failure.

I am sorry, I have seen that in my first post I wrote "3.0RC4" x instead of "3.0 RC4". It is a typo, I correct it now.

But if allowed "2. 0. 0" (correct name is "2.0.0" why not "2.0RC4" ? Perhaps would be part of this bug. I will see how re-write code of function.

[Arantor]

Perhaps would be part of this bug.

In fact, now that I think about it, you're not supposed to use - on RCs, it's designed for numeric comparison only, i.e. 1.1.1-1.1.10 and anything textual is about as useful as asking it to compare on apple-orange as a range...

[davidhs]

It is true. I will change the function to do this

[Arantor]

Hmm. I think this should be moved out of Bug Reports, because it's only buggy when you give it something that doesn't make sense to it... it works as intended AFAIK on version *numbers*.

[davidhs]

I have re-written the first post with the necessary changes.

Hmm. I think this should be moved out of Bug Reports, because it's only buggy when you give it something that doesn't make sense to it... it works as intended AFAIK on version *numbers*.

No, this is a bug. Not only afect to RCs. Also affects to numerical versions if they are not in interval.

matchPackageVersion('3.0.3', '3.0, 3.0.2-3.0. 3, 3.0.5') => MATCH
matchPackageVersion('3.0.5', '3.0, 3.0.2-3.0.3, 3.0. 5') => NO MATCH

[Arantor]

How is that a bug?! There's a space in it, no way it's supposed to match then.

Next you'll be telling me that 1.0Alpha should match 1.0 Alpha.

The only reason I haven't moved this into Bogus Bugs is because I don't want to seem biased.

[davidhs]

Well, I think there are two distinct problems:

1. Admit spaces only in middle of numeric version and only in interval.

In my opinion if '3.0.2-3.0. 3' is correct, these should be correct:
'3.0. 3' (space in numeric version without interval)
'3.0     RC1' (space in no-numeric version)

I think it is a bug. If admit spaces in the middle of numeric version in interval, should admit spaces in the middle on all type of version.
For this, source code is the same of above but replace

Code: [Select]

$version = strtr($version, array(' ' => ''));
$versions = strtr($versions, array(' ' => ''));by
(When I write this PHP code I can not write correct regular expression because failure to save. I write SPACES instead of [[:blank:] ], without space character between "]")

Code: [Select]

$regex = array('SPACES*,SPACES*' => ',', 'SPACES*-SPACES*' => '-', 'SPACES*\.SPACES*' => '.', 'SPACES+' => ' ');
foreach ($regex as $pattern => $replacement)
{
$version = ereg_replace($pattern, $replacement, $version);
$versions = ereg_replace($pattern, $replacement, $versions);
}
2. No spaces where there should be.

This is case of 3.0RC1 (correct is 3.0 RC1) or 3.0Beta (correct is 3.0 Beta).
This is a bogus bug and with source code of case 1 not is allowed.




I rewrote my first post to correct only the case 1. I hope that is correct ;)

[Arantor]

In my opinion if '3.0.2-3.0. 3' is correct, these should be correct:

It's not correct, that's the point.

I think it is a bug. If admit spaces in the middle of numeric version in interval, should admit spaces in the middle on all type of version.
For this, source code is the same of above but replace

No genuine version number from SMF would have that, so there's no legitimate reason for spaces there anyway.

I've done my part, the devs can make a decision for the rest of this, but as far as I'm concerned this isn't a bug, because it works when it's supposed to (i.e. on numeric ranges without spaces in when they're not supposed to have spaces, and it treats versions with spaces in as distinct non sequential ones, as intended)

[davidhs]

At last I understand!

If '3.0.2-3.0. 3' is not correct there are a bug in matchPackageVersion() because its source code allows this

Code: [Select]

foreach ($upper as $key => $high)
{
// Let's check that this is at or below the upper... obviously.
if (isset($version[$key]) && trim($version[$key]) > trim($high))
return false;

// OK, let's check it's above the lower key... if it exists!
if (isset($lower[$key]))
{
// The version either needs to have something here (i.e. can't be 1.0 on a 1.0.11) AND needs to be greater or equal to.
// Note that if it's a range lower[key] might be blank, in that case version can not be set!
if (!empty($lower[$key]) && (!isset($version[$key]) || trim($version[$key]) < trim($lower[$key])))
return false;
}
}
As I do not know whether or not this is a bug,
1. If '3.0.2-3.0. 3' is not correct there are a bug because '3.0.2-3.0. 3' is allowed.
2. If '3.0.2-3.0. 3' is correct there are a different bug because '3.0. 3' is not allowed (Why can interval have spaces and no-interval can not? )
I remove this "bug" of first post and only put the bug "only allows one interval".

Any developer to say whether or not '3.0.2-3.0. 3' & '3.0. 3' is correct and I will add the bug to the first post.

[Arantor]

-sigh-

The problem comes about because it doesn't know how to tell 3.0. 3 from 3.0. beta 1, for example...

[Joshua Dickerson]

With or without spaces it should be treated the same. There are other issues with matchPackageVersion() that need to be fixed. I think I will track this along with the other issues.

[Arantor]

With or without spaces it should be treated the same. There are other issues with matchPackageVersion() that need to be fixed. I think I will track this along with the other issues.

So you would argue that 3.0.3 == 3.0. 3?

[Joshua Dickerson]

In the way that it is being used... yes. Commas are used to delimit the version number, not spaces. Otherwise, they are completely different.

Why make a simple typo break an entire mod? Especially one that would be so hard to find and fix. Then we'd have to document that. It doesn't make sense when removing space characters makes so much more sense.

[Arantor]

That has consequences too.

OK, so let's say you strip spaces entirely. This means now that 2.0RC5 > 2.0.1 in version terms. I also bet there are issues with 2.0.9 being greater than 2.0.11 as well if you go down that road.

[Joshua Dickerson]

Code: [Select]

<?php
if (version_compare('2.0RC5', '2.0.1', '>'))
echo 'arantor is right';
else
echo 'groundup is right';
// returns 'groundup is right'
?>

[Arantor]

Oh, so you're actually going to move everything to use version_compare instead of using the custom function as was suggested how many moons ago?

That also assumes that version numbers stick to the method used by version_compare...

[davidhs]

I also bet there are issues with 2.0.9 being greater than 2.0.11

With matchPackageVersion() of SMF 1.1.11 and lower was 2.0.9 > 2.0.11 because compared strings.
In SMF 1.1.12 was fixed and now compare each number (2 = 2 AND 0 = 0 AND 9 < 11 => 2.0.9 < 2.0.11


I also think that is better to use the PHP function version_compare and so everything is fixed.

[Illori]

Joshua Dickerson do you recall if this was ever tracked?

[Joshua Dickerson]

I want to say yes, but I might just be thinking of a post in another beta testing board. I'll take a look later.

[emanuele]

I'd say that try to guess and fix too many things will most likely break others...

Any kind of typo can already break a mod (think about a typo in the code).
If there is a standard I think who is using that standard should be sure to respect it, otherwise what's the reason for a standard?

[[SiNaN]]

We introduced a completely new matchPackageVersion() just before 2.0 final release. I'm sure current function can handle pretty much any case you can think of (this one too) as expected.

[emanuele]

Thanks SiNaN!
Then this can go in fixed.