phar-io/version

[Regression] (in 3.0.1) Version::getVersionString will not return "v" prefix if used

addshore opened this issue · 3 comments

A change happened in 3.0.1 which means that versions will not be output with a prefixed "v" if they started with one.

This was caused by the following change

8cef51d#diff-9cb52248cb807e881c8c2b95169d98aa095f95ee40639b13af3b9f28cc553122L42

Adding this as a regression as the CHANGELOG doesn't mention it.
I spotted this while looking into deprecated-packages/symplify#2972

I'm happy to write a patch (if this is deemed to be a regression).
Also happy to add a CHANGELOG entry if this is to be the new accepted behaviour!

This indeed is a change in behavior.

But, as hard as it may be, the previous behavior was technically speaking wrongly supporting invalid version strings. Any prefix to the version string is not semver compliant (see https://semver.org/#is-v123-a-semantic-version => NO.) and - theoretically - we shouldn't allow it, even when parsing.

So, rather than the output being now "broken" (as compared to before), the parsing shouldn't have even allowed it to begin with. That way, it would at least have been consistent.

Unfortunately, we do support the v-Prefix since day one for input.

As I see it, we have the following options to address this:

  • Leave it as is and report it in the Changelog as a potential BC break
  • Restore the previous behavior by keeping track of the v and prepend it on output
  • Add an optional parameter to Version::getVersionString() for prefixing
  • Add a new method Version::getOriginalString() (or similar) to get the prefix back in case it was specified

I'm not sure which one I like best. As of right now, I'm leaning to preferring the last mentioned option..

Add a new method Version::getOriginalString() (or similar) to get the prefix back in case it was specified

Sounds quite good to me too, and this would suite the usecase in symplify/symplify quite well.