phar-io/version

Tilde operator without patch level

seyon opened this issue · 3 comments

seyon commented

We are currently using the 2.x version and wanted to update to the new 3.x version. In the process, however, our unit tests failed.
The original statement no longer works:

Tilde operator: ~1.0.0 can be written as >=1.0.0 <1.1.0 and read as »every version within minor version 1.1. The behavior of tilde operator depends on whether a patch level version is provided or not. If no patch level is provided, tilde operator behaves like the caret operator: ~1.0 is identical to ^1.0.

When parsing a version number, "0" is now always set instead of "null" for the patch default value, so that the "getPatch()->isAny()" method returns true every time. It then assumes, for example, "~7.0.0" even if you pass "~7.0" and a comparison with 7.1 then fails.

$this->patch = isset($matches['Patch']) ? new VersionNumber((int)$matches['Patch']) : new VersionNumber(0);

Finally having some time to look into this, but I'm no longer convinced this issue is valid.

  1. The description and title of this issue refer to the use of the tilde operator. That makes it deal with a version constraint, not, as referenced with a link to the code, with a version.

  2. A version is required to always have a patch level as per semver specification (read, format is - at least - required to be x.y.z) and thus it can never be null. The fact we allow that in the parsing logic of Version qualifies as a bug of its own. I'm considering to tighten this.

  3. I don't see how in your unit test you can get true for getPatch()->isAny() when coming from Version. The value is never null thus it should always return false, given the check reads return $this->value === null.

Nevertheless, the code is problematic: Version as well as VersionConstraintParser use the same VersionNumber implementation despite their different contexts. From the constraint's perspective, VersionNumber::isAny can make sense as a part in the constraint can be optional, from the version's perspective it's not and thus the method probably shouldn't be available. Given it always will return false when used with Version though, it's not exactly a wrong - just pointless.

Okay, it is a valid issue as ConstraintParser uses Version internally and thus requires the optional patch level.

Fixed with commit 9dc75c0.