Skip to content
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

Inconsistent application of stability flags in parseConstraint #154

Open
Xerkus opened this issue Oct 4, 2023 · 6 comments
Open

Inconsistent application of stability flags in parseConstraint #154

Xerkus opened this issue Oct 4, 2023 · 6 comments
Labels

Comments

@Xerkus
Copy link

Xerkus commented Oct 4, 2023

VersionParser::parseConstraint() does not take into consideration stability flag when parsing constraint with ~ tilde or ^ caret range:
Constraint ~1.2.3@RC is parsed as >=1.2.3.0-dev and <1.3.0.0-dev
However ~1.2.3-RC is parsed as >=1.2.3.0-RC and <1.3.0.0-dev as expected.

I believe this inconsistency is a bug since ~ or ^ returns early from a conditional block but stability flags are considered again much later in the method where it apples to not equal and less/greater operators.

At the same time I see edge case with the way stability flag is handled with pre-release identifiers that are considered stable by composer:
>1.2.3@rc -> 1.2.3.0-rc
>1.2.3-beta@rc -> 1.2.3.0-beta
>1.2.3-stable@RC -> 1.2.3.0-rc
>1.2.3-patch1@RC -> 1.2.3.0-patch1-rc
I believe last constraint is invalid and as such a bug.

Hyphen ranges do not consider stability flags but also do not allow them on either side. No issue here.

Constraint with wildcards 1.2.*@rc does not allow pre-release identifiers but allows stability flags which it then ignores.

@Xerkus
Copy link
Author

Xerkus commented Oct 5, 2023

less than and less than equal with stability flag produce results that I question as well.

<1.2.0@rc -> < 1.2.0.0-rc
With that constraint as upper bound first will match alpha or beta in 1.2.0 assuming minimum-stability or flag on another part of multi-constraint allows those. Somehow this does not feel right. Is it intended outcome?

<=1.2.0@rc -> <= 1.2.0.0-rc
This will allow unstable matches but will not match stable 1.2.0.

@Seldaek
Copy link
Member

Seldaek commented Oct 11, 2023

I think you're probably right that all these are wrong, they're very edge casey, and as such probably not super critical fixes. But yeah it should be fixed if possible.

@Seldaek Seldaek added the bug label Oct 11, 2023
@Xerkus
Copy link
Author

Xerkus commented Oct 11, 2023

@Seldaek I was wondering if stability flags should affect constraints at all, considering they are not restricted to a single part of constraint and really handled at repository filtering level with the lowest taking the effect. It should probably have no effect on indirect constraints in dependencies so be stripped from parsed constraint entirely.

@Xerkus
Copy link
Author

Xerkus commented Oct 12, 2023

@Seldaek to clarify, I am willing to provide a PR as soon as I know the preferred direction for the change.

@Seldaek
Copy link
Member

Seldaek commented Oct 12, 2023

Yeah I'm not sure myself. Need to think about it some more and first finish my vacation ;)

@Xerkus
Copy link
Author

Xerkus commented Oct 12, 2023

Have a nice vacation.

As I was researching this for renovate I collected following composer behavior examples that are relevant here:

  • 1.0.0@rc requires precise version and only exactly 1.0.0 will be considered by composer. Stability flag has no effect. Pretty straightforward.
  • ^1.0.0-rc1 allows any release candidate within range, eg 1.2.0-rc1. Npm on the other hand allows release candidate only for that specific version and would not allow say 1.0.1-rc1.
  • ^1.0.0-rc1 with minimum-stability set to dev would allow any dev/alpha/beta/rc within range. It will not allow 1.0.0-alpha1 but 1.0.1-alpha1 is fair game.
  • ^1.0.0-rc1 with minimum-stability set to stable would allow any release candidate within range.
  • ^1.0.0-rc1 or ^1.0.0@RC declared by dependency will not allow release candidates if minimum-stability set to stable.
  • 1.0.0-rc1@stable in root composer.json will not be installed at all as minimum-stability is forced to stable and required version does not match it. Minimum-stability setting has no effect.
  • 0.1.0@rc || 1.0.0-rc1@stable will install 1.0.0-rc1 because lowest stability flag takes precedence.
  • Similarly, ^1.0.0@rc || ^2.0.0@alpha would allow any stability alpha or higher for the whole range, even for 1.0.0

Because stability affects whole range it makes very little sense to cut out just the very edge of the range. So, unless parsed constraints are changed to deal with minimum stability within range, I believe sensible approach is not to treat stability flag as part of the version and outright strip it for the parsed constraint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants