Skip to content

Conversation

@mambax7
Copy link
Collaborator

@mambax7 mambax7 commented Jan 1, 2026

Solves the issue of XOOPS reporting that 2.5.12-beta is wrong version when the min. was set for 2.5.12

  • Anything below 2.5.12 → ❌ reject
  • Anything 2.5.12 (stable, beta, rc, alpha, etc.) → ✅ accept
  • Anything above 2.5.12 (2.5.13+, even alpha/beta) → ✅ accept

Copilot AI review requested due to automatic review settings January 1, 2026 16:27
Copy link
Contributor

Copilot AI left a 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.

Copy link
Contributor

Copilot AI left a 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.

Copy link
Contributor

Copilot AI left a 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);
Copy link

Copilot AI Jan 1, 2026

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.

Suggested change
return (bool)version_compare($n1, $n2, $op);
return version_compare($n1, $n2, $op);

Copilot uses AI. Check for mistakes.
@ggoffy
Copy link
Contributor

ggoffy commented Jan 2, 2026

versionCompare only takes into account the basic version number. the comparison of $modversion['min_xoops'] = '2.5.12 Stable'; and a current XOOPS version 2.5.12 Beta gives back true
shouldn't the state also be taken into consideration?

@mambax7
Copy link
Collaborator Author

mambax7 commented Jan 2, 2026

The main role of min_xoops is to prevent incompatibility. It is not meant to enforce release channels or act as a quality-assurance filter.

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?

  • Required 2.5.12

    • Current 2.5.12-beta ✅ (base-only)
  • Required 2.5.12 stable

    • Current 2.5.12-beta
    • Current 2.5.12 ✅ (no suffix treated as stable)
  • Required 2.5.12 rc1

    • Current 2.5.12-rc2
    • Current 2.5.12-beta
  • Required 2.5.12 stable

    • Current 2.5.12-nightly ❌ (unknown suffix treated as dev)

@ggoffy
Copy link
Contributor

ggoffy commented Jan 3, 2026

hi, you are right, in principal it would be ok without checking the suffix ;)

@mambax7
Copy link
Collaborator Author

mambax7 commented Jan 13, 2026

@ggoffy If you're OK with it, could you approve it?

@mambax7 mambax7 merged commit fb92e2c into XOOPS:master Jan 13, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants