Skip to content
This repository has been archived by the owner on Mar 15, 2020. It is now read-only.

Update package to use Composer\Semver and more robust version sorting/comparing #34

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,16 @@ the version string used by Github. This can follow any standard practice with
recognisable pre- and postfixes, e.g.
`v1.0.3`, `1.0.3`, `1.1`, `1.3rc`, `1.3.2pl2`.

Note: All version strings must follow [SemVer v2.0.0](http://semver.org/) to allow
accurate comparison. The sole exception is that allowance is made for versions pre-
or post-fixed with `dev` to mark development versions. Development versions are
compared solely on the basis of the version string excluding the `dev` label. As
a convenience, versions formatted using, for example, `git describe` (e.g.
`1.0.0alpha.2-26-ge67g3d`) have a `-dev` postfix added (i.e. becoming
`1.0.0alpha.2-dev`). If you frequently update across development versions, it is
strongly recommended to use the SHA-1 or SHA-256 strategies, or implement a custom
strategy that can compare versions based on such metadata.

If you wish to update to a non-stable version, for example where users want to
update according to a development track, you can set the stability flag for the
Github strategy. By default this is set to `stable` or, in constant form,
Expand Down
3 changes: 2 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@
],
"require": {
"php": "^5.6|^7.0",
"padraic/humbug_get_contents": "^1.0"
"padraic/humbug_get_contents": "^1.0",
"composer/semver": "^1.4.2"
},
"require-dev": {
"phpunit/phpunit": "^5.5|^6.0"
Expand Down
30 changes: 28 additions & 2 deletions src/Updater.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

namespace Humbug\SelfUpdate;

use Humbug\SelfUpdate\VersionParser;
use Humbug\SelfUpdate\Exception\RuntimeException;
use Humbug\SelfUpdate\Exception\InvalidArgumentException;
use Humbug\SelfUpdate\Exception\FilesystemException;
Expand Down Expand Up @@ -320,13 +321,38 @@ protected function hasPubKey()
return $this->hasPubKey;
}

/**
* Compares local and remote versions. If version strings do not follow
* Semver, i.e. hashes, the strings are just checked for equality.
* @return bool
*/
protected function newVersionAvailable()
{
$this->newVersion = $this->strategy->getCurrentRemoteVersion($this);
$this->oldVersion = $this->strategy->getCurrentLocalVersion($this);

if (!empty($this->newVersion) && ($this->newVersion !== $this->oldVersion)) {
return true;
try {
if (!empty($this->newVersion)
&& !VersionParser::equals($this->newVersion, $this->oldVersion)
) {
return true;
}
} catch (\UnexpectedValueException $e) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

UnexpectedValueException thrown by Composer\Semver. I should add a check that we do the following !== comparison only if the strategy in play is NOT github. Otherwise, we'll silently fail on invalid semver when there MUST be a valid semver version string.

if ($this->getStrategy() instanceof GithubStrategy) {
throw new RuntimeException(
'The current reported version or the current remote version '
. 'does not adhere to Semantic Versioning and cannot be compared.'
. PHP_EOL
. sprintf('Current version: %s', $this->oldVersion)
. PHP_EOL
. sprintf('Remote version: %s', $this->newVersion)
);
}
if (!empty($this->newVersion)
&& ($this->newVersion !== $this->oldVersion)
) {
return true;
}
}
return false;
}
Expand Down
66 changes: 38 additions & 28 deletions src/VersionParser.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@

namespace Humbug\SelfUpdate;

use Composer\Semver\Semver;
use Composer\Semver\VersionParser as Parser;

class VersionParser
{

Expand All @@ -20,17 +23,23 @@ class VersionParser
*/
private $versions;

/**
* @var Composer\VersionParser
*/
private $parser;

/**
* @var string
*/
private $modifier = '[._-]?(?:(stable|beta|b|RC|alpha|a|patch|pl|p)(?:[.-]?(\d+))?)?([.-]?dev)?';
const GIT_DATA_MATCH = '/.*(-\d+-g[[:alnum:]]{7})$/';

/**
* @param array $versions
*/
public function __construct(array $versions = array())
{
$this->versions = $versions;
$this->parser = new Parser;
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend not omitting () during class instantiation without constructor parameters.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not part of the PSR-2 standard which is enforced. We might adopt PSR-12 one day, if it ever leaves draft to get voted on, but I won't expect any contributor to follow a standard which we don't advertise/enforce upfront. That includes me and my own bias towards wondering why the seven hells I need to include parentheses when there are zero parameters ;).

Copy link
Member

Choose a reason for hiding this comment

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

I remember the first time I saw this I was confused as I didn't know it was possible, and when you call a function foo which has no arguments you call foo(), not foo. So I did wonder why the seven hells people took the liberty to change it with new :trollface:

In any case, it's my own bias as well and I don't think anyone would feel strongly about it 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

That's why I don't like PSR-2 - too much room for improvement. Anyway please make it consistent across the code. If in all other places the () are not added, then don't added it here too.

}

/**
Expand Down Expand Up @@ -112,6 +121,20 @@ public function isDevelopment($version)
return $this->development($version);
}

/**
* Checks if two version strings are the same normalised version.
*
* @param string
* @param string
* @return bool
*/
public static function equals($version1, $version2)
{
$parser = new Parser;
return $parser->normalize(self::stripGitHash($version1))
=== $parser->normalize(self::stripGitHash($version2));
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Composer\Semver does not take kindly to versions containing git metadata tagged on to the end of it. This can be injected by Box, for example, when building PHARs. We use a small method to strip it out before passing to Semver.


private function selectRecentStable()
{
$candidates = array();
Expand Down Expand Up @@ -159,44 +182,31 @@ private function selectRecentAll()

private function findMostRecent(array $candidates)
{
$candidate = null;
$tracker = null;
foreach ($candidates as $version) {
if (version_compare($candidate, $version, '<')) {
$candidate = $version;
}
}
return $candidate;
$sorted = Semver::rsort($candidates);
return $sorted[0];
}

private function stable($version)
{
$version = preg_replace('{#.+$}i', '', $version);
if ($this->development($version)) {
return false;
}
preg_match('{'.$this->modifier.'$}i', strtolower($version), $match);
if (!empty($match[3])) {
return false;
}
if (!empty($match[1])) {
if ('beta' === $match[1] || 'b' === $match[1]
|| 'alpha' === $match[1] || 'a' === $match[1]
|| 'rc' === $match[1]) {
return false;
}
if ('stable' === Parser::parseStability(self::stripGitHash($version))) {
return true;
}
return true;
return false;
}

private function development($version)
{
if ('dev-' === substr($version, 0, 4) || '-dev' === substr($version, -4)) {
return true;
}
if (1 == preg_match("/-\d+-[a-z0-9]{8,}$/", $version)) {
if ('dev' === Parser::parseStability(self::stripGitHash($version))) {
return true;
}
return false;
}

private static function stripGitHash($version)
{
if (preg_match(self::GIT_DATA_MATCH, $version, $matches)) {
$version = str_replace($matches[1], '-dev', $version);
}
return $version;
}
}
49 changes: 49 additions & 0 deletions tests/Humbug/Test/SelfUpdate/VersionParserTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -167,4 +167,53 @@ public function testIsDevelopment()
$this->assertTrue($parser->isDevelopment('1.0.0-dev'));
$this->assertTrue($parser->isDevelopment('1.0.0-alpha1-5-g5b46ad8'));
}

public function testEqualsWithSameSemverVersion()
{
$v1 = '1.2.3';
$v2 = '1.2.3';
$this->assertTrue(VersionParser::equals($v1, $v2));
}

public function testEqualsWithDifferentSemverVersion()
{
$v1 = '1.2.3';
$v2 = '1.2.4';
$this->assertFalse(VersionParser::equals($v1, $v2));
}

public function testEqualsWithSameSemverVersionButPrefixed()
{
$v1 = '1.2.3';
$v2 = 'v1.2.3';
$this->assertTrue(VersionParser::equals($v1, $v2));
}

public function testEqualsWithSameSemverVersionButGitData()
{
$v1 = '1.2.3-5-g5b46ad8';
$v2 = '1.2.3-5-g5b46ad8';
$this->assertTrue(VersionParser::equals($v1, $v2));
}

public function testEqualsWithDifferentSemverVersionButGitData()
{
$v1 = '1.2.3-5-g5b46ad8';
$v2 = '1.2.4-5-g5b46ad8';
$this->assertFalse(VersionParser::equals($v1, $v2));
}

public function testEqualsWithSameSemverVersionButStabilityDiffers()
{
$v1 = '1.2.3-alpha';
$v2 = '1.2.3';
$this->assertFalse(VersionParser::equals($v1, $v2));
}

public function testEqualsWithSameSemverVersionButStabilitySameButNumberedOff()
{
$v1 = '1.2.3-alpha';
$v2 = '1.2.3-alpha2';
$this->assertFalse(VersionParser::equals($v1, $v2));
}
}