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

Fixes #20: Version normalization adds unnecessary .0 #21

Closed
wants to merge 2 commits into from

Conversation

grasmash
Copy link
Contributor

@grasmash grasmash commented May 6, 2022

Fix #20

@grasmash grasmash changed the title Update SelfUpdateCommand.php Fixes #20: Version normalization adds unnecessary .0 May 6, 2022
@danepowell
Copy link
Collaborator

@greg-1-anderson could you please review and merge this, and cut a new release if everything looks good?

We've been using it downstream for a while: https://github.com/acquia/cli/blob/ebb1cca2cefbc981a7bd7b9fc80b8ca52f71d408/composer.json#L111

Tests are failing in HEAD and I don't think the test failures here are caused by the PR.

@danepowell
Copy link
Collaborator

danepowell commented Feb 21, 2023

In main, running self-update will always download the latest release even if the current version is the latest (i.e. no update is required), because Composer's normalized version format includes this trailing zero, which doesn't generally match semver.

I've confirmed this is the behavior today in Terminus. It's also the behavior in Acquia CLI if we remove this patch.

I'd argue this is a bug, but I could see how it might be a feature (if a user suspects their phar is corrupt and requests an update, they should get an update even if it's not strictly necessary).

Fixing this bug here could be disruptive, as evidenced by the failing tests. I think the safest solution is for Acquia CLI to just append a zero to its internal version and abandon this patch, unless Terminus also wants an upstream fix.

@greg-1-anderson
Copy link
Member

I don't think that this would hurt Terminus, but Terminus always uses three-digit version numbers, so I don't think it is necessary.

@danepowell
Copy link
Collaborator

In hindsight, this PR was a valid issue, it just needed a little more work: #25

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.

Version normalization adds unnecessary .0
3 participants