-
-
Notifications
You must be signed in to change notification settings - Fork 61
improve versionCompare() #1595
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
improve versionCompare() #1595
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
|
|
hi, you are right, in principal it would be ok without checking the suffix ;) |
|
@ggoffy If you're OK with it, could you approve it? |
Solves the issue of XOOPS reporting that
2.5.12-betais wrong version when the min. was set for2.5.12