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

Fix incorrect version extraction when extracting php version from composer.json constraints #139

Merged

Conversation

internalsystemerror
Copy link
Member

@internalsystemerror internalsystemerror commented Nov 1, 2022

Q A
Documentation no
Bugfix yes
BC Break no
New Feature no
RFC no
QA yes

Description

A bug was discovered with several repositories and #136 managed to highlight the issue. This PR proposes a fix for this.

Gary Lockett added 6 commits October 29, 2022 17:59
Signed-off-by: Gary Lockett <gary@creativecow.uk>
Signed-off-by: Gary Lockett <gary@creativecow.uk>
Signed-off-by: Gary Lockett <gary@creativecow.uk>
Signed-off-by: Gary Lockett <gary@creativecow.uk>
Signed-off-by: Gary Lockett <gary@creativecow.uk>
Signed-off-by: Gary Lockett <gary@creativecow.uk>
@internalsystemerror
Copy link
Member Author

internalsystemerror commented Nov 2, 2022

Actually this appears to break <=8.1.0 returning 8.1 :/. Should that be the case? A version of 8.1.12 would not actually match that constraint anyway?

Or maybe it should be if it satisfies ${version.0} OR if it satisfies ${version}.99?

@weierophinney
Copy link
Member

Actually this appears to break <=8.1.0 returning 8.1

I'd prefer that break.

I see no good reason to indicate compatibility with 8.1.0, but not later versions in the 8.1 series.

@Ocramius
Copy link
Member

Ocramius commented Nov 2, 2022

Indeed, broken, but "less broken than now" :D

@internalsystemerror
Copy link
Member Author

I'll modify the test that fails to indicate that <=8.1.0 should not match the minor 8.1 then.

Signed-off-by: Gary Lockett <gary@creativecow.uk>
@internalsystemerror
Copy link
Member Author

internalsystemerror commented Nov 2, 2022

Actually ... I think I came up with a "better" way, at least in terms of not breaking anything that is currently expecting this operation. I believe the original intention was for the patch number to essentially be ignored, so that's exactly what I've done here.

Any X.x.x version is normalised to X.x.0 and the comparison is then made to ${installableVersion}.0. This seems to satisfy most of the current functionality whilst also fixing the issues where the patch number > 0.

See https://github.com/laminas/laminas-ci-matrix-action/pull/139/files#diff-7c294c22c7ebe5a7822160fad8ac3789ab3cd7629c7b7637920ef3c72e2cc7b9R7-R20 for how this change is intended to function.

@internalsystemerror internalsystemerror added this to the 1.19.0 milestone Nov 2, 2022
@Ocramius Ocramius added Bug Something isn't working Enhancement labels Nov 2, 2022
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚢

@Ocramius Ocramius self-assigned this Nov 2, 2022
@Ocramius Ocramius changed the title Fix incorrect version extraction Fix incorrect version extraction when extracting php version from composer.json constraints Nov 2, 2022
@Ocramius Ocramius merged commit e6724c7 into laminas:1.19.x Nov 2, 2022
@internalsystemerror internalsystemerror deleted the fix-incorrect-version-extraction branch November 2, 2022 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants