Skip to content
This repository has been archived by the owner on May 1, 2019. It is now read-only.

[WIP] Enhancement: Added GitHub Repository Service #287

Merged
merged 26 commits into from
Jan 11, 2015
Merged
Show file tree
Hide file tree
Changes from 21 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
1 change: 0 additions & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ language: php
sudo: false

php:
- 5.4
- 5.5
Copy link
Member

Choose a reason for hiding this comment

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

@ins0

The only clue I currently have about what PHP version is currently running on the production machine is #284 (comment), so I think it's not really safe yet to assume we can rely on the availability of PHP5.5.

Can you make the changes in this PR PHP5.4 compliant?

  • class keyword
  • generators?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

👍

- 5.6
- hhvm
Expand Down
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
"license": "BSD-3-Clause",
"homepage": "https://modules.zendframework.com",
"require": {
"php": "~5.4",
"php": "~5.5",
Copy link
Member

Choose a reason for hiding this comment

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

Can someone confirm

  • that we actually have PHP 5.5 running on the server onto which this is deployed

    or

  • that $ composer install is not part of the deployment process

otherwise this will break the deployment, I believe.

/cc @GeeH @Ocramius

Copy link
Contributor Author

Choose a reason for hiding this comment

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

confirmed here, no? #284 (comment) - on the other side i'm not sure if the deployment is working because a merged pr #292 is still broken on the live system. /cc @GeeH

Copy link
Member

Choose a reason for hiding this comment

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

👍

"bshaffer/oauth2-server-php": "dev-develop",
"zendframework/zendframework": "~2.3.0",
"rwoverdijk/assetmanager": "1.3.*",
Expand Down
1 change: 1 addition & 0 deletions module/Application/config/module.config.php
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
'service_manager' => array(
'factories' => array(
'translator' => 'Zend\I18n\Translator\TranslatorServiceFactory',
\Application\Service\RepositoryRetriever::class => \Application\Service\RepositoryRetrieverFactory::class,
Copy link
Member

Choose a reason for hiding this comment

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

👍 I like that a lot, been using it in all projects where PHP5.5 was available.

However, I'd avoid FCQNs and import Application\Service;

),
),
'translator' => array(
Expand Down
85 changes: 85 additions & 0 deletions module/Application/src/Application/Service/RepositoryRetriever.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
<?php

namespace Application\Service;

use EdpGithub\Client;
use EdpGithub\Collection\RepositoryCollection;
use EdpGithub\Listener\Exception\RuntimeException;

class RepositoryRetriever
{
/**
* @var Client
*/
private $githubClient;

/**
* @param Client $githubClient
*/
public function __construct(Client $githubClient)
{
$this->githubClient = $githubClient;
}

/**
* Return MetaData from User Repository
* @param $user
* @param $module
Copy link
Member

Choose a reason for hiding this comment

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

Can you add the types here? Should be string, I think, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nm

* @return array|null
*/
public function getUserRepositoryMetadata($user, $module)
{
return json_decode($this->githubClient->api('repos')->show($user, $module));
}

/**
* Get all Repositories from GitHub User
* @param $user
* @param array $params
* @return RepositoryCollection
*/
public function getUserRepositories($user, $params = array())
Copy link
Member

Choose a reason for hiding this comment

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

I guess it's safe to type-hint using array here, no?

{
return $this->githubClient->api('user')->repos($user, $params);
}

/**
* Get File Content from User Repository
* @param $user
* @param $module
* @param $filePath
Copy link
Member

Choose a reason for hiding this comment

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

Can you add the types here as well?

* @return string|null
*/
public function getRepositoryFileContent($user, $module, $filePath)
{
$contentResponse = $this->getRepositoryFileMetadata($user, $module, $filePath);

if (!isset($contentResponse->content)) {
return null;
}

return base64_decode($contentResponse->content);
}

/**
* Return File MetaData from User Repository
* @param $user
* @param $module
* @param $filePath
Copy link
Member

Choose a reason for hiding this comment

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

Can you add the types here as well?

* @return mixed
*/
public function getRepositoryFileMetadata($user, $module, $filePath)
{
return json_decode($this->githubClient->api('repos')->content($user, $module, $filePath));
}

/**
* Return all Repositories from current authenticated GitHub User
* @param array $params
Copy link
Member

Choose a reason for hiding this comment

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

Return type is missing (it's Generator)

* @return RepositoryCollection
*/
public function getAuthenticatedUserRepositories($params = array())
Copy link
Member

Choose a reason for hiding this comment

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

Type-hint with array?

{
return $this->githubClient->api('current_user')->repos($params);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?php

namespace Application\Service;

use EdpGithub\Client;
use Zend\ServiceManager\FactoryInterface;
use Zend\ServiceManager\ServiceLocatorInterface;

class RepositoryRetrieverFactory implements FactoryInterface
{
/**
* {@inheritDoc}
*
* @return RepositoryRetriever
*/
public function createService(ServiceLocatorInterface $serviceLocator)
{
/* @var Client $githubClient */
$githubClient = $serviceLocator->get(Client::class);
Copy link
Member

Choose a reason for hiding this comment

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

Need to make sure it's PHP5.4-compliant for now, happy to revisit as soon as someone has confirmed we've got PHP5.5+ running on the production machine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


return new RepositoryRetriever($githubClient);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
<?php

namespace ApplicationTest\Service;

use Application\Service\RepositoryRetriever;
use EdpGithub\Api;
use EdpGithub\Listener\Exception\RuntimeException;
use PHPUnit_Framework_TestCase;

class RepositoryRetrieverTest extends PHPUnit_Framework_TestCase
{
private $response;
private $headers;
private $httpClient;
private $client;

protected function setUp()
{
$this->response = $this->getMock('Zend\Http\Response');
$this->headers = $this->getMock('Zend\Http\Headers');
$this->httpClient = $this->getMock('EdpGithub\Http\Client');
$this->client = $this->getMock('EdpGithub\Client');
}

protected function tearDown()
{
$this->response = null;
$this->headers = null;
$this->httpClient = null;
$this->client = null;
}

private function getClientMock(Api\AbstractApi $apiInstance, $result)
{
$this->response->expects($this->any())
->method('getBody')
->will($this->returnValue($result));

$this->response->expects($this->any())
->method('getHeaders')
->will($this->returnValue($this->headers));

$this->httpClient->expects($this->any())
->method('get')
->will($this->returnValue($this->response));

$this->client->expects($this->any())
->method('getHttpClient')
->will($this->returnValue($this->httpClient));

$apiInstance->setClient($this->client);

$this->client->expects($this->any())
->method('api')
->will($this->returnValue($apiInstance));

return $this->client;
}

private function prepareService(Api\AbstractApi $apiInstance, $result)
Copy link
Member

Choose a reason for hiding this comment

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

Can you inline this method? I'd personally not mind the duplication in each test (arrange, act, assert) because then I think it's easier to understand what's going on and especially, what the system under test actually is (see DAMP vs. DRY).

{
return new RepositoryRetriever(
$this->getClientMock($apiInstance, $result)
);
}

public function testCanRetrieveUserRepositories()
{
$payload = [
['name' => 'foo'],
['name' => 'bar'],
['name' => 'baz']
];

$service = $this->prepareService(new Api\User, json_encode($payload));

$repositories = $service->getUserRepositories('foo');
$this->assertInstanceOf('EdpGithub\Collection\RepositoryCollection', $repositories);

$count = 0;
foreach ($repositories as $repository) {
$this->assertEquals(current($payload), (array)$repository);
next($payload);
++$count;
}

$this->assertEquals(count($payload), $count);
}

public function testCanRetrieveUserRepositoryMetadata()
{
$payload = [
'name' => 'foo',
'url' => 'http://foo.com'
];

$service = $this->prepareService(new Api\Repos, json_encode($payload));
$metadata = $service->getUserRepositoryMetadata('foo', 'bar');

$this->assertInstanceOf('stdClass', $metadata);
$this->assertEquals($payload, (array)$metadata);
}

public function testCanRetrieveRepositoryFileContent()
{
$payload = [
'content' => base64_encode('foo')
];
$service = $this->prepareService(new Api\Repos, json_encode($payload));
$response = $service->getRepositoryFileContent('foo', 'bar', 'foo.baz');

$this->assertEquals('foo', $response);
}

public function testResponseContentMissingOnGetRepositoryFileContent()
{
$payload = [];
$service = $this->prepareService(new Api\Repos, json_encode($payload));
$response = $service->getRepositoryFileContent('foo', 'bar', 'baz');

$this->assertNull($response);
}

public function testCanRetrieveRepositoryFileMetadata()
{
$payload = [
'name' => 'foo',
'url' => 'http://foo.com'
];

$service = $this->prepareService(new Api\Repos, json_encode($payload));
$metadata = $service->getRepositoryFileMetadata('foo', 'bar', 'baz');

$this->assertInstanceOf('stdClass', $metadata);
$this->assertEquals($payload, (array)$metadata);
}

public function testCanRetrieveAuthenticatedUserRepositories()
{
$payload = [
['name' => 'foo'],
['name' => 'bar'],
['name' => 'baz']
];

$service = $this->prepareService(new Api\CurrentUser, json_encode($payload));

$repositories = $service->getAuthenticatedUserRepositories();
$this->assertInstanceOf('EdpGithub\Collection\RepositoryCollection', $repositories);
Copy link
Member

Choose a reason for hiding this comment

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

Can leverage the class keyword here, too. Helps a lot with finding usages in PhpStorm, too.


$count = 0;
foreach ($repositories as $repository) {
$this->assertEquals(current($payload), (array)$repository);
next($payload);
++$count;
}

$this->assertEquals(count($payload), $count);
}
}
Loading