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

Manifest file strategy #15

Open
wants to merge 2 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
178 changes: 178 additions & 0 deletions src/Strategy/ManifestStrategy.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,178 @@
<?php
namespace Humbug\SelfUpdate\Strategy;

use Humbug\SelfUpdate\Exception\HttpRequestException;
use Humbug\SelfUpdate\Exception\JsonParsingException;
use Humbug\SelfUpdate\Updater;
use Humbug\SelfUpdate\VersionParser;

class ManifestStrategy implements StrategyInterface
Copy link
Member

Choose a reason for hiding this comment

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

I would make this class final

{
/** @var array */
private static $requiredKeys = array('sha1', 'version', 'url');
/** @var string */
private $manifestUrl;
/** @var array */
private $manifest;
/** @var array */
private $availableVersions;
/** @var string */
private $localVersion;
/** @var bool */
private $allowMajor = false;
/** @var bool */
Copy link
Member

Choose a reason for hiding this comment

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

let's keep it like:


/**
 * @var bool
 */
private $allowUnstable = false;

private $allowUnstable = false;

/**
* ManifestStrategy constructor.
Copy link
Member

Choose a reason for hiding this comment

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

I'm personally not very found of noisy comments (i.e. comment not providing any value), so I would remove that line unless @padraic prefer to keep it for consistency. Although I would argue in favour of changing that practice and fixing that inconsistency later :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Opinions can vary, but I'm fairly verbose myself in comments. I just find it a PITA, esp. in open source projects, to spend too time familiarising myself with how something works so a bit of hand holding can be welcome.

Also, easy to copy/paste to a README and THEN delete the comments ;).

*
* @param string $localVersion The local version.
* @param string $manifestUrl The URL to a JSON manifest file. The
* manifest contains an array of objects, each
* containing a 'version', 'sha1', and 'url'.
* @param bool $allowMajor Whether to allow updating between major
* versions.
* @param bool $allowUnstable Whether to allow updating to an unstable
* version. Ignored if $localVersion is unstable
* and there are no new stable versions.
*/
public function __construct($localVersion, $manifestUrl, $allowMajor = false, $allowUnstable = false)
{
$this->localVersion = $localVersion;
$this->manifestUrl = $manifestUrl;
$this->allowMajor = $allowMajor;
$this->allowUnstable = $allowUnstable;
}

/**
* {@inheritdoc}
*/
public function getCurrentLocalVersion(Updater $updater)
{
return $this->localVersion;
}

/**
* {@inheritdoc}
*/
public function download(Updater $updater)
{
$version = $this->getCurrentRemoteVersion($updater);
if ($version === false) {
throw new \RuntimeException('No remote versions found');
}
$versionInfo = $this->getAvailableVersions();
Copy link
Member

Choose a reason for hiding this comment

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

missing blank line between L63-L64

if (!isset($versionInfo[$version])) {
throw new \RuntimeException(sprintf('Failed to find manifest item for version %s', $version));
}

$fileContents = file_get_contents($versionInfo[$version]['url']);
if ($fileContents === false) {
throw new HttpRequestException(sprintf('Failed to download file from URL: %s', $versionInfo[$version]['url']));
}

$tmpFilename = $updater->getTempPharFile();
if (file_put_contents($tmpFilename, $fileContents) === false) {
throw new \RuntimeException(sprintf('Failed to write file: %s', $tmpFilename));
}

$tmpSha = sha1_file($tmpFilename);
if ($tmpSha !== $versionInfo[$version]['sha1']) {
unlink($tmpFilename);
throw new \RuntimeException(
sprintf(
'SHA-1 verification failed: expected %s, actual %s',
$versionInfo[$version]['sha1'],
$tmpSha
)
);
}
}

/**
* {@inheritdoc}
*/
public function getCurrentRemoteVersion(Updater $updater)
{
$versions = array_keys($this->getAvailableVersions());
if (!$this->allowMajor) {
$versions = $this->filterByLocalMajorVersion($versions);
}

$versionParser = new VersionParser($versions);

$mostRecent = $versionParser->getMostRecentStable();

// Look for unstable updates if explicitly allowed, or if the local
// version is already unstable and there is no new stable version.
if ($this->allowUnstable || ($versionParser->isUnstable($this->localVersion) && version_compare($mostRecent, $this->localVersion, '<'))) {
$mostRecent = $versionParser->getMostRecentAll();
}

return version_compare($mostRecent, $this->localVersion, '>') ? $mostRecent : false;
}

/**
* Get available versions to update to.
Copy link
Member

Choose a reason for hiding this comment

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

Gets

*
* @return array
* An array keyed by the version name, whose elements are arrays
Copy link
Member

Choose a reason for hiding this comment

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

the comment should be right after array (like for params)

Copy link
Member

Choose a reason for hiding this comment

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

nice phrasing, I like that keyed :P

* containing version information ('name', 'sha1', and 'url').
*/
private function getAvailableVersions()
{
if (!isset($this->availableVersions)) {
Copy link
Member

Choose a reason for hiding this comment

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

when possible, I would recommend an early return to avoid too much indentation :)

if (isset($this->availableVersions)) {
    return $this->availableVersions;
}

// Keep going

Applies to other places as well

$this->availableVersions = array();
foreach ($this->getManifest() as $key => $item) {
if ($missing = array_diff(self::$requiredKeys, array_keys($item))) {
throw new \RuntimeException(sprintf('Manifest item %s missing required key(s): %s', $key, implode(',', $missing)));
}
$this->availableVersions[$item['version']] = $item;
}
}

return $this->availableVersions;
}

/**
* Download and decode the JSON manifest file.
*
* @return array
*/
private function getManifest()
Copy link
Member

Choose a reason for hiding this comment

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

retrieveManifest()? I dislike get* here because although natural it's too mixed with the getter/setter idea which are 90% just about returning a property without any logic. retrieveManifest() kinda conveys that it's not just about returning a property value.

{
if (!isset($this->manifest)) {
$manifestContents = file_get_contents($this->manifestUrl);
if ($manifestContents === false) {
throw new \RuntimeException(sprintf('Failed to download manifest: %s', $this->manifestUrl));
}

$this->manifest = (array) json_decode($manifestContents, true);
Copy link
Member

Choose a reason for hiding this comment

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

$this->manifest = json_decode($manifestContents, true, 512, JSON_OBJECT_AS_ARRAY);

if (json_last_error() !== JSON_ERROR_NONE) {
throw new JsonParsingException(
'Error parsing manifest file'
. (function_exists('json_last_error_msg') ? ': ' . json_last_error_msg() : '')
);
}
}

return $this->manifest;
Copy link
Member

Choose a reason for hiding this comment

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

I would rather return a $manifest property and do $this->manifest = $this->retrieveManifest()

}

/**
* Filter a list of versions to those that match the current local version.
*
* @param string[] $versions
*
* @return string[]
*/
private function filterByLocalMajorVersion(array $versions)
{
list($localMajorVersion, ) = explode('.', $this->localVersion, 2);
return array_filter($versions, function ($version) use ($localMajorVersion) {
list($majorVersion, ) = explode('.', $version, 2);
return $majorVersion === $localMajorVersion;
Copy link
Member

Choose a reason for hiding this comment

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

I personally like (but maybe @humbug/core team has another opinion on that) to have a blank line before a return/exit statement. It makes it easier to identify

});

Copy link
Member

Choose a reason for hiding this comment

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

unecessary blank line

}
}
76 changes: 76 additions & 0 deletions tests/Humbug/Test/SelfUpdate/UpdaterManifestStrategyTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
<?php

namespace Humbug\Test\SelfUpdate;

use Humbug\SelfUpdate\Updater;
use Humbug\SelfUpdate\Strategy\ManifestStrategy;

class UpdaterManifestStrategyTest extends \PHPUnit_Framework_TestCase
{

Copy link
Member

Choose a reason for hiding this comment

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

unnecessary blank line

private $files;
Copy link
Member

Choose a reason for hiding this comment

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

missing phpdoc for the props


/** @var Updater */
private $updater;

private $manifestFile;

private $tmp;

public function setup()
{
$this->tmp = sys_get_temp_dir();
$this->files = __DIR__ . '/_files';
$this->updater = new Updater($this->files . '/test.phar', false);
$this->manifestFile = $this->files . '/manifest.json';
}

public function testGetLocalVersion()
{
$strategy = new ManifestStrategy('1.0.0', $this->manifestFile);
$this->assertEquals('1.0.0', $strategy->getCurrentLocalVersion($this->updater));
}

public function testSuggestMostRecentStable()
{
$strategy = new ManifestStrategy('1.0.0', $this->manifestFile);
$this->assertEquals('1.2.0', $strategy->getCurrentRemoteVersion($this->updater));
}

public function testSuggestNewestUnstable()
{
$strategy = new ManifestStrategy('1.0.0', $this->manifestFile, false, true);
$this->assertEquals('1.3.0-beta', $strategy->getCurrentRemoteVersion($this->updater));
}

public function testSuggestNewestStableFromUnstable()
{
$strategy = new ManifestStrategy('1.0.0-beta', $this->manifestFile);
$this->assertEquals('1.2.0', $strategy->getCurrentRemoteVersion($this->updater));
}

public function testSuggestNewestUnstableFromUnstable()
{
$strategy = new ManifestStrategy('1.2.9-beta', $this->manifestFile);
$this->assertEquals('1.3.0-beta', $strategy->getCurrentRemoteVersion($this->updater));
}

public function testUpdate()
{
copy($this->files . '/test.phar', $this->tmp . '/test.phar');
$updater = new Updater($this->tmp . '/test.phar', false);
$strategy = new ManifestStrategy('1.0.0', $this->manifestFile);
$updater->setStrategyObject($strategy);
$updater->setBackupPath($this->tmp . '/backup.phar');
$cwd = getcwd();
chdir(__DIR__);
$updater->update();
chdir($cwd);
}

public function teardown()
Copy link
Member

Choose a reason for hiding this comment

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

I would move it just under setUp(), they go together :)

and also there's a missing @inheritdoc for both setUp() and tearDown()

{
@unlink($this->tmp . '/test.phar');
@unlink($this->tmp . '/backup.phar');
}
}
42 changes: 42 additions & 0 deletions tests/Humbug/Test/SelfUpdate/_files/manifest.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
[
{
"sha1": "",
"url": "",
"version": "2.0.0"
},
{
"sha1": "",
"url": "",
"version": "2.0.0-beta"
},
{
"sha1": "",
"url": "",
"version": "1.3.0-beta"
},
{
"sha1": "0bc24f886bc0c7563187167b334e56cfb8e1151a",
"url": "_files/build/nosig.phar",
"version": "1.2.0"
},
{
"sha1": "",
"url": "",
"version": "1.1.0"
},
{
"sha1": "",
"url": "",
"version": "1.0.0"
},
{
"sha1": "",
"url": "",
"version": "1.0.0-beta"
},
{
"sha1": "",
"url": "",
"version": "0.9.0"
}
]