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

Don't try to parse aliases versions which doesn't contain exact version #76

Merged
merged 3 commits into from
Jan 13, 2020

Conversation

adrianosferreira
Copy link
Contributor

This has been reported in: composer/composer#8338

Originally it was accepting such format "8.3.3 as ^8.3" when using composer require (it was taking the 8.3.3 version) and then saving it on composer.json. However, when you run any other command after, you were getting the message "the alias must be an exact version".

So, I choose to throw an exception whenever an alias is used in the version without passing exact versions in order to comply with the message that we get.

@codecov-io
Copy link

codecov-io commented Jan 6, 2020

Codecov Report

Merging #76 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #76      +/-   ##
============================================
+ Coverage     88.05%   88.08%   +0.03%     
  Complexity      206      206              
============================================
  Files             7        7              
  Lines           385      386       +1     
============================================
+ Hits            339      340       +1     
  Misses           46       46
Impacted Files Coverage Δ Complexity Δ
src/VersionParser.php 95.25% <100%> (+0.02%) 131 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2667cf1...a8ea125. Read the comment docs.

@adrianosferreira adrianosferreira force-pushed the not-exact-alias-version branch from 77285ee to cc9fca3 Compare January 7, 2020 00:20
.gitignore Outdated Show resolved Hide resolved
src/VersionParser.php Outdated Show resolved Hide resolved
@adrianosferreira adrianosferreira force-pushed the not-exact-alias-version branch from fb541e8 to 7922406 Compare January 7, 2020 10:50
@stof
Copy link
Contributor

stof commented Jan 7, 2020

I see 2 issues here:

  • your validation does not only restrict the alias (on the right of as), but also the constraint
  • what happens if I use a <1.2 range as alias ?

IMO, the validation of the alias should be done by parsing it as a version (similar to what is done in some places in Composer), not by trying to apply some regex on the whole string to detect specific broken cases.

@adrianosferreira
Copy link
Contributor Author

@stof in fact it was doing this before, trying to parse it as version, so 8.3.3 as ^8.3 becomes 8.3.3 and it was installing 8.3.3, however, saving 8.3.3 as ^8.3 in the composer.json and causing future problems.

IMO trying to parse as a version at this point is not an option, do you think I could extend it to other operators like you said <?

@stof
Copy link
Contributor

stof commented Jan 7, 2020

@adrianosferreira the alias is the part on the right of as, not on the left.

@adrianosferreira adrianosferreira force-pushed the not-exact-alias-version branch 3 times, most recently from a563619 to a8ea125 Compare January 7, 2020 11:32
@adrianosferreira
Copy link
Contributor Author

adrianosferreira commented Jan 7, 2020

@stof right, I think it is better now. Can you double-check? I've called the normalizer method for the right side of the string (the alias) which then throws an exception in case we don't have an exact version.

[UnexpectedValueException]                                                           
  Could not parse version constraint 3.3.0 as ^3.3.1: Invalid version string "^3.3.1"

@Seldaek
Copy link
Member

Seldaek commented Jan 13, 2020

Thanks

@Seldaek Seldaek merged commit 48bf3c3 into composer:master Jan 13, 2020
@adrianosferreira
Copy link
Contributor Author

@Seldaek thanks for merging.
I've noticed just now that I forgot to squash commits, the actual fix is in the commit a8ea125 (fixup) If you could squash it with the first one cc9fca3 it would be really great. Otherwise, people looking at the git history might wrongly thing that the solution on cc9fca3 is the correct one.

@Seldaek
Copy link
Member

Seldaek commented Jan 13, 2020

Too late but no big deal really..

@adrianosferreira
Copy link
Contributor Author

@Seldaek I usually resolve with force push on my repos. However, not sure of the impact of force pushing in a widely used repo such as semver.

@Seldaek
Copy link
Member

Seldaek commented Jan 13, 2020

There is no way I am force pushing :) Especially not after it's been tagged.

@adrianosferreira
Copy link
Contributor Author

@Seldaek oh yes, that's true! Thanks!

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.

4 participants