Skip to content
This repository has been archived by the owner on Nov 26, 2017. It is now read-only.

Add extra docs to Github package; light refactoring; bug fixes. #295

Merged
merged 3 commits into from
Dec 5, 2013
Merged
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
133 changes: 133 additions & 0 deletions src/Joomla/Github/AbstractGithubObject.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
<?php
/**
* Part of the Joomla Framework Github Package
*
* @copyright Copyright (C) 2005 - 2013 Open Source Matters, Inc. All rights reserved.
* @license GNU General Public License version 2 or later; see LICENSE
*/

namespace Joomla\Github;

use Joomla\Http\Response;
use Joomla\Uri\Uri;
use Joomla\Registry\Registry;

/**
* GitHub API object class for the Joomla Framework.
*
* @since 1.0
*/
abstract class AbstractGithubObject
{
/**
* @var Registry Options for the GitHub object.
* @since 1.0
*/
protected $options;

/**
* @var Http The HTTP client object to use in sending HTTP requests.
* @since 1.0
*/
protected $client;

/**
* @var string
* @since 1.0
*/
protected $package = '';

/**
* Constructor.
*
* @param Registry $options GitHub options object.
* @param Http $client The HTTP client object.
*
* @since 1.0
*/
public function __construct(Registry $options = null, Http $client = null)
{
$this->options = isset($options) ? $options : new Registry;
$this->client = isset($client) ? $client : new Http($this->options);

$this->package = get_class($this);
$this->package = substr($this->package, strrpos($this->package, '\\') + 1);
}

/**
* Method to build and return a full request URL for the request. This method will
* add appropriate pagination details if necessary and also prepend the API url
* to have a complete URL for the request.
*
* @param string $path URL to inflect
* @param integer $page Page to request
* @param integer $limit Number of results to return per page
*
* @return string The request URL.
*
* @since 1.0
*/
protected function fetchUrl($path, $page = 0, $limit = 0)
{
// Get a new Uri object fousing the api url and given path.
$uri = new Uri($this->options->get('api.url') . $path);

if ($this->options->get('gh.token', false))
{
// Use oAuth authentication - @todo set in request header ?
$uri->setVar('access_token', $this->options->get('gh.token'));
}
else
{
// Use basic authentication
if ($this->options->get('api.username', false))
{
$uri->setUser($this->options->get('api.username'));
}

if ($this->options->get('api.password', false))
{
$uri->setPass($this->options->get('api.password'));
}
}

// If we have a defined page number add it to the JUri object.
if ($page > 0)
{
$uri->setVar('page', (int) $page);
}

// If we have a defined items per page add it to the JUri object.
if ($limit > 0)
{
$uri->setVar('per_page', (int) $limit);
}

return (string) $uri;
}

/**
* Process the response and decode it.
*
* @param Response $response The response.
* @param integer $expectedCode The expected "good" code.
*
* @return mixed
*
* @since 1.0
* @throws \DomainException
*/
protected function processResponse(Response $response, $expectedCode = 200)
{
// Validate the response code.
if ($response->code != $expectedCode)
{
// Decode the error response and throw an exception.
$error = json_decode($response->body);
$message = isset($error->message) ? $error->message : 'Invalid response received from GitHub.';
throw new \DomainException($message, $response->code);
}

return json_decode($response->body);
}
}
67 changes: 67 additions & 0 deletions src/Joomla/Github/AbstractPackage.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
<?php
/**
* Part of the Joomla Framework Github Package
*
* @copyright Copyright (C) 2005 - 2013 Open Source Matters, Inc. All rights reserved.
* @license GNU General Public License version 2 or later; see LICENSE
*/

namespace Joomla\Github;

use Joomla\Registry\Registry;

/**
* GitHub API package class for the Joomla Framework.
*
* @since 1.0
*/
abstract class AbstractPackage extends AbstractGithubObject
{
/**
* Constructor.
*
* @param Registry $options GitHub options object.
* @param Http $client The HTTP client object.
*
* @since 1.0
*/
public function __construct(Registry $options = null, Http $client = null)
{
parent::__construct($options, $client);

$this->package = get_class($this);
$this->package = substr($this->package, strrpos($this->package, '\\') + 1);
}

/**
* Magic method to lazily create API objects
*
* @param string $name Name of property to retrieve
*
* @since 1.0
* @throws \InvalidArgumentException
*
* @return AbstractPackage GitHub API package object.
*/
public function __get($name)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to use normal getters because it looks like you access a mutable (public) property.
I would rather see a normal factory to create the top level objects and getters for the sublevel ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a simple renaming of the file. The idea is that the package API is available just using magic properties, sort of like mixins. The Docblocks help the IDE work out what should be returned during chaining.

Copy link
Contributor

Choose a reason for hiding this comment

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

It can be handy but the problem is that you can "navigate" inside a package like this :

<?php
$github->issues->issues->issues....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But does that cause an actual problem?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, but it shouldn't be possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm mentionning it as I see it. It is the same "issue" with Input.
Maybe using the Github class as factory to create the objects

<?php

$issues = $github->getIssues();
$issueComments = $github->getIssueComments();

I think, ideally it should be structured this way

<?php

$issueManager = new IssueManager($options, $client);

// Finding an issue
$issue = $issueManager->findById(8);

// Creating an issue
$issue = new Issue();
$issue->setTitle('hello');
$issueManager->save($issue);

Each issue is a plain object with setters and getters and the manager acting as Data access object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My goal was simply to rename the file to agree with the naming standards. This isn't new work.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have created an issue #301

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

no problem :)

{
$class = '\\Joomla\\Github\\Package\\' . $this->package . '\\' . ucfirst($name);

if (false == class_exists($class))
{
throw new \InvalidArgumentException(
sprintf(
'Argument %1$s produced an invalid class name: %2$s in package %3$s',
$name, $class, $this->package
)
);
}

if (false == isset($this->$name))
{
$this->$name = new $class($this->options, $this->client);
}

return $this->$name;
}
}
120 changes: 3 additions & 117 deletions src/Joomla/Github/GithubObject.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,126 +8,12 @@

namespace Joomla\Github;

use Joomla\Http\Response;
use Joomla\Uri\Uri;
use Joomla\Registry\Registry;

/**
* GitHub API object class for the Joomla Framework.
*
* @since 1.0
* @deprecated Use AbstractGithubObject instead.
* @since 1.0
*/
abstract class GithubObject
abstract class GithubObject extends AbstractGithubObject
{
/**
* @var Registry Options for the GitHub object.
* @since 1.0
*/
protected $options;

/**
* @var Http The HTTP client object to use in sending HTTP requests.
* @since 1.0
*/
protected $client;

/**
* @var string The package in which the object belongs.
* @since 1.0
*/
protected $package = '';

/**
* Constructor.
*
* @param Registry $options GitHub options object.
* @param Http $client The HTTP client object.
*
* @since 1.0
*/
public function __construct(Registry $options = null, Http $client = null)
{
$this->options = isset($options) ? $options : new Registry;
$this->client = isset($client) ? $client : new Http($this->options);

$this->package = get_class($this);
$this->package = substr($this->package, strrpos($this->package, '\\') + 1);
}

/**
* Method to build and return a full request URL for the request. This method will
* add appropriate pagination details if necessary and also prepend the API url
* to have a complete URL for the request.
*
* @param string $path URL to inflect
* @param integer $page Page to request
* @param integer $limit Number of results to return per page
*
* @return string The request URL.
*
* @since 1.0
*/
protected function fetchUrl($path, $page = 0, $limit = 0)
{
// Get a new Uri object fousing the api url and given path.
$uri = new Uri($this->options->get('api.url') . $path);

if ($this->options->get('gh.token', false))
{
// Use oAuth authentication - @todo set in request header ?
$uri->setVar('access_token', $this->options->get('gh.token'));
}
else
{
// Use basic authentication
if ($this->options->get('api.username', false))
{
$uri->setUser($this->options->get('api.username'));
}

if ($this->options->get('api.password', false))
{
$uri->setPass($this->options->get('api.password'));
}
}

// If we have a defined page number add it to the JUri object.
if ($page > 0)
{
$uri->setVar('page', (int) $page);
}

// If we have a defined items per page add it to the JUri object.
if ($limit > 0)
{
$uri->setVar('per_page', (int) $limit);
}

return (string) $uri;
}

/**
* Process the response and decode it.
*
* @param Response $response The response.
* @param integer $expectedCode The expected "good" code.
*
* @return mixed
*
* @since 1.0
* @throws \DomainException
*/
protected function processResponse(Response $response, $expectedCode = 200)
{
// Validate the response code.
if ($response->code != $expectedCode)
{
// Decode the error response and throw an exception.
$error = json_decode($response->body);
$message = isset($error->message) ? $error->message : 'Invalid response received from GitHub.';
throw new \DomainException($message, $response->code);
}

return json_decode($response->body);
}
}
54 changes: 3 additions & 51 deletions src/Joomla/Github/Package.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,60 +8,12 @@

namespace Joomla\Github;

use Joomla\Registry\Registry;

/**
* GitHub API package class for the Joomla Framework.
*
* @since 1.0
* @deprecated Use AbstractPackage instead.
* @since 1.0
*/
abstract class Package extends GithubObject
abstract class Package extends AbstractPackage
{
/**
* Constructor.
*
* @param Registry $options GitHub options object.
* @param Http $client The HTTP client object.
*
* @since 1.0
*/
public function __construct(Registry $options = null, Http $client = null)
{
parent::__construct($options, $client);

$this->package = get_class($this);
$this->package = substr($this->package, strrpos($this->package, '\\') + 1);
}

/**
* Magic method to lazily create API objects
*
* @param string $name Name of property to retrieve
*
* @since 1.0
* @throws \InvalidArgumentException
*
* @return Package GitHub API package object.
*/
public function __get($name)
{
$class = '\\Joomla\\Github\\Package\\' . $this->package . '\\' . ucfirst($name);

if (false == class_exists($class))
{
throw new \InvalidArgumentException(
sprintf(
'Argument %1$s produced an invalid class name: %2$s in package %3$s',
$name, $class, $this->package
)
);
}

if (false == isset($this->$name))
{
$this->$name = new $class($this->options, $this->client);
}

return $this->$name;
}
}
Loading