Skip to content
This repository was 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

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bdaaa86
Offload version parsing to Composer\Semver
padraic May 8, 2017
2d75629
Leverage Semver sorting/normalisation of versions
padraic May 8, 2017
c24dae9
Constant visibility is PHP 7.1 only...
padraic May 8, 2017
b359561
Merge branch 'master' into feature/semver
padraic May 8, 2017
64d0232
Throw exception for invalid semantic versioning
padraic May 12, 2017
a4d1d9c
Postfix git-metadata containing versions with -dev to mark dev status
padraic May 12, 2017
479065a
Update tests
padraic May 12, 2017
7feead0
Update to README to document limitations with respect to development …
padraic May 12, 2017
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
3 changes: 2 additions & 1 deletion composer.json
View file
Open in desktop
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
20 changes: 18 additions & 2 deletions src/Updater.php
View file
Open in desktop
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 @@ -314,13 +315,28 @@ 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

@padraic padraic May 12, 2017

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 (!empty($this->newVersion)
&& ($this->newVersion !== $this->oldVersion)
) {
return true;
}
}
return false;
}
Expand Down
66 changes: 38 additions & 28 deletions src/VersionParser.php
View file
Open in desktop
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

@aik099 aik099 May 8, 2017

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

@padraic padraic May 8, 2017

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

@theofidry theofidry May 8, 2017

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

@aik099 aik099 May 9, 2017

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

@padraic padraic May 8, 2017

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], '', $version);
}
return $version;
}
}
53 changes: 51 additions & 2 deletions tests/Humbug/Test/SelfUpdate/VersionParserTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ public function testIsPreRelease()
$this->assertFalse($parser->isPreRelease('1.0.0'));
$this->assertTrue($parser->isPreRelease('1.0.0b'));
$this->assertFalse($parser->isPreRelease('1.0.0-dev'));
$this->assertFalse($parser->isPreRelease('1.0.0-alpha1-5-g5b46ad8'));
$this->assertTrue($parser->isPreRelease('1.0.0-alpha1-5-g5b46ad8'));
}

public function testIsUnstable()
Expand All @@ -165,6 +165,55 @@ public function testIsDevelopment()
$this->assertFalse($parser->isDevelopment('1.0.0'));
$this->assertFalse($parser->isDevelopment('1.0.0b'));
$this->assertTrue($parser->isDevelopment('1.0.0-dev'));
$this->assertTrue($parser->isDevelopment('1.0.0-alpha1-5-g5b46ad8'));
$this->assertFalse($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));
}
}