improve versionCompare() #1595
improve versionCompare() #1595
Conversation
mambax7
requested review from
ForXoops,
GregMage,
alain01,
geekwright,
ggoffy and
ihackcode
There was a problem hiding this comment.
Pull request overview
This PR improves the versionCompare() method in the module class to correctly handle version strings with suffixes like -beta, -alpha, -rc, and -stable. The key improvement is that versions with the same base number are now treated as equal regardless of their suffix, which fixes the issue where 2.5.12-beta was incorrectly rejected when the minimum version was set to 2.5.12.
Key Changes:
- Refactored version comparison to extract and compare only base version numbers (stripping suffixes)
- Versions with identical base numbers are now treated as equal regardless of suffix
- Simplifies the logic by normalizing both versions consistently before comparison
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
htdocs/kernel/module.php
Outdated
| public function versionCompare($version1 = '', $version2 = '', $operator = '<') | ||
| { | ||
| $version1 = strtolower($version1); | ||
| $version2 = strtolower($version2); | ||
| if (false !== strpos($version2, '-stable')) { | ||
| $version2 = substr($version2, 0, strpos($version2, '-stable')); | ||
| } | ||
| if (false !== strpos($version1, '-stable')) { | ||
| $version1 = substr($version1, 0, strpos($version1, '-stable')); | ||
| $normalize = static function ($ver) { | ||
| $ver = strtolower($ver); | ||
| // strip suffix like -beta, -alpha, -rc, -stable | ||
| if (false !== ($pos = strpos($ver, '-'))) { | ||
| $ver = substr($ver, 0, $pos); | ||
| } | ||
| return $ver; | ||
| }; | ||
|
|
||
| $base1 = $normalize($version1); | ||
| $base2 = $normalize($version2); | ||
|
|
||
| // Compare only the base versions first | ||
| $cmp = version_compare($base1, $base2); | ||
|
|
||
| if ($cmp === 0) { | ||
| // Same base version → treat as equal, regardless of suffix | ||
| return version_compare('0', '0', $operator); | ||
| } | ||
| return version_compare($version1, $version2, $operator); | ||
|
|
||
| // Otherwise, just use the base numeric comparison | ||
| return version_compare($base1, $base2, $operator); | ||
| } |
There was a problem hiding this comment.
The updated version comparison logic lacks test coverage. Since this repository has existing PHPUnit test infrastructure (tests/unit/), and this is a critical function used for version validation across the system, unit tests should be added to verify the new behavior handles all edge cases correctly, including:
- Same base version with different suffixes (e.g., 2.5.12 vs 2.5.12-beta)
- Different base versions with suffixes (e.g., 2.5.11-stable vs 2.5.12-beta)
- All comparison operators (<, <=, >, >=, ==, !=)
- Edge cases like empty strings or malformed version strings
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
htdocs/kernel/module.php
Outdated
| // Normalize legacy "-stable" (keep behavior consistent) | ||
| if (($pos = strpos($ver, '-stable')) !== false) { | ||
| $ver = substr($ver, 0, $pos); | ||
| $ver = rtrim($ver); |
There was a problem hiding this comment.
The rtrim($ver) call on line 203 is redundant because line 211 already trims the entire string at the end of the normalization function. Additionally, the initial trim() on line 199 removes leading and trailing whitespace. This extra trim operation has no effect and can be removed.
| $ver = rtrim($ver); |
htdocs/kernel/module.php
Outdated
| // Normalize legacy "-stable" (keep behavior consistent) | ||
| if (($pos = strpos($ver, '-stable')) !== false) { | ||
| $ver = substr($ver, 0, $pos); | ||
| $ver = rtrim($ver); | ||
| } | ||
|
|
||
| // Strip any suffix after first '-': -beta, -alpha, -rc, -dev, etc. |
There was a problem hiding this comment.
The special handling for -stable on lines 201-204 is redundant since lines 207-209 already strip any suffix starting with -, which would include -stable. The comment mentions "keep behavior consistent" but the behavior would be identical without this block. Consider removing lines 201-204 to simplify the logic and avoid confusion.
| // Normalize legacy "-stable" (keep behavior consistent) | |
| if (($pos = strpos($ver, '-stable')) !== false) { | |
| $ver = substr($ver, 0, $pos); | |
| $ver = rtrim($ver); | |
| } | |
| // Strip any suffix after first '-': -beta, -alpha, -rc, -dev, etc. | |
| // Strip any suffix after first '-': -beta, -alpha, -rc, -dev, -stable, etc. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| return version_compare($version1, $version2, $operator); | ||
|
|
||
| return (bool)version_compare($n1, $n2, $op); |
There was a problem hiding this comment.
The explicit cast to (bool) on line 215 is unnecessary. The version_compare() function already returns a boolean when an operator is provided, so the cast is redundant.
| return (bool)version_compare($n1, $n2, $op); | |
| return version_compare($n1, $n2, $op); |
|
versionCompare only takes into account the basic version number. the comparison of |
|
The main role of In real use, module requirements almost always depend on which core APIs and behaviors are available in a given base version. They do not depend on whether that version is marked as beta, RC, or stable. Treating 2.5.12-beta as compatible with 2.5.12 reflects how modules actually work and avoids rejecting users who deliberately run pre-release versions for testing. Including version suffixes in the default comparison would bring back the very problem this change is meant to solve. It would also be fragile, because suffixes are written inconsistently across versions and projects. If a module author truly needs to require a specific release stage, such as stable-only, that requirement should be stated explicitly and enabled by choice. The current approach keeps the common case simple and predictable, while still allowing stricter rules to be added later if they prove necessary. But just to make it clear - is this what you're looking for?
|
2 participants
Solves the issue of XOOPS reporting that
2.5.12-betais wrong version when the min. was set for2.5.12