Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/exception factory #50

Merged
merged 20 commits into from
Oct 10, 2017
Merged
Show file tree
Hide file tree
Changes from 4 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
31 changes: 30 additions & 1 deletion src/Exception/ApiException.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,35 @@

/**
*/
class ApiException extends BunqException
class ApiException extends \Exception
Copy link
Contributor

Choose a reason for hiding this comment

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

Let us please do use Exception; and then Exception with no preceding \ :)

{
/**
* The first item index in an array.
*/
const INDEX_FIRST = 0;

/**
* @var int
*/
private $responseCode;

/**
* ApiException constructor.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment can go :)

*
* @param int $responseCode
*/
public function __construct($message, $args = [])
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add strict types for arguments (PHP 7.0 style) and doc block params. Also, why are we using $args? Is it necessary here? If so, please add a comment explaining this to the constructor header.

{
$this->responseCode = $args[self::INDEX_FIRST];

parent::__construct(vsprintf($message, $args));
}

/**
* @return int
*/
public function getResponseCode()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add PHP 7.0 strict response type (: int)

{
return $this->responseCode;
}
}
9 changes: 9 additions & 0 deletions src/Exception/BadRequestException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?php
namespace bunq\Exception;

/**
*/
class BadRequestException extends ApiException
{

}
52 changes: 52 additions & 0 deletions src/Exception/ExceptionFactory.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
<?php
namespace bunq\Exception;

/**
*/
class ExceptionFactory
{
/**
* HTTP error response codes constants.
*/
const HTTP_RESPONSE_CODE_BAD_REQUEST = 400;
const HTTP_RESPONSE_CODE_UNAUTHORIZED = 401;
const HTTP_RESPONSE_CODE_FORBIDDEN = 403;
const HTTP_RESPONSE_CODE_NOT_FOUND = 404;
const HTTP_RESPONSE_CODE_METHOD_NOT_ALLOWED = 405;
const HTTP_RESPONSE_CODE_TOO_MANY_REQUESTS = 429;
const HTTP_RESPONSE_CODE_INTERNAL_SERVER_ERROR = 500;

/**
* The first item index in an array.
*/
const INDEX_FIRST = 0;

/**
* @param string $message
* @param array $args
*
* @return BadRequestException|ForbiddenException|MethodNotAllowedException|NotFoundException|PleaseContactBunqException|ToManyRequestsException|UnauthorizedException|UnknownApiErrorException
Copy link
Contributor

Choose a reason for hiding this comment

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

Just ApiException would be good :P

*/
public static function createExceptionForResponse($message, $args = [])
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question: do we need args here?

Also please add strict param types and the response type :)

{
switch ($args[self::INDEX_FIRST])
{
case self::HTTP_RESPONSE_CODE_BAD_REQUEST:
return new BadRequestException($message, $args);
case self::HTTP_RESPONSE_CODE_UNAUTHORIZED:
return new UnauthorizedException($message, $args);
case self::HTTP_RESPONSE_CODE_FORBIDDEN:
return new ForbiddenException($message, $args);
case self::HTTP_RESPONSE_CODE_NOT_FOUND:
return new NotFoundException($message, $args);
case self::HTTP_RESPONSE_CODE_METHOD_NOT_ALLOWED:
return new MethodNotAllowedException($message, $args);
case self::HTTP_RESPONSE_CODE_TOO_MANY_REQUESTS:
return new ToManyRequestsException($message, $args);
case self::HTTP_RESPONSE_CODE_INTERNAL_SERVER_ERROR:
return new PleaseContactBunqException($message, $args);
default:
return new UnknownApiErrorException($message, $args);
}
}
}
9 changes: 9 additions & 0 deletions src/Exception/ForbiddenException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?php
namespace bunq\Exception;

/**
*/
class ForbiddenException extends ApiException
{

}
9 changes: 9 additions & 0 deletions src/Exception/MethodNotAllowedException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?php
namespace bunq\Exception;

/**
*/
class MethodNotAllowedException extends ApiException
{

}
9 changes: 9 additions & 0 deletions src/Exception/NotFoundException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?php
namespace bunq\Exception;

/**
*/
class NotFoundException extends ApiException
{

}
9 changes: 9 additions & 0 deletions src/Exception/PleaseContactBunqException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?php
namespace bunq\Exception;

/**
*/
class PleaseContactBunqException extends ApiException
{

}
9 changes: 9 additions & 0 deletions src/Exception/ToManyRequestsException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?php
namespace bunq\Exception;

/**
*/
class ToManyRequestsException extends ApiException
Copy link
Contributor

Choose a reason for hiding this comment

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

One o refuged it seems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OMG this is in all the SDK's 😁 🙈

{

}
9 changes: 9 additions & 0 deletions src/Exception/UnauthorizedException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?php
namespace bunq\Exception;

/**
*/
class UnauthorizedException extends ApiException
{

}
9 changes: 9 additions & 0 deletions src/Exception/UnknownApiErrorException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?php
namespace bunq\Exception;

/**
*/
class UnknownApiErrorException extends ApiException
{

}
5 changes: 3 additions & 2 deletions src/Http/Handler/ResponseHandlerError.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace bunq\Http\Handler;

use bunq\Exception\ApiException;
use bunq\Exception\ExceptionFactory;
use Psr\Http\Message\ResponseInterface;

/**
Expand Down Expand Up @@ -60,15 +61,15 @@ public function execute(ResponseInterface $response)
$errorDescriptions[] = $error[self::FIELD_ERROR_DESCRIPTION];
}

throw new ApiException(
throw ExceptionFactory::createExceptionForResponse(
self::ERROR_FROM_JSON,
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it is not yours, but please make the interface of createExceptionForResponse consistent with that in all the other SDKs if possible :)

[
$response->getStatusCode(),
implode(self::SEPERATOR_ERROR, $errorDescriptions),
]
);
} else {
throw new ApiException(
throw ExceptionFactory::createExceptionForResponse(
self::ERROR_UNKNOWN,
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, please make error handling here consistent with what we have in e.g. Python :)

[
$response->getStatusCode(),
Expand Down
4 changes: 1 addition & 3 deletions tests/BunqSdkTestBase.php
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,7 @@ protected static function ensureApiContextValid()
{
try {
$apiContext = ApiContext::restore(static::FILENAME_CONTEXT_CONFIG);
User::listing($apiContext);
} catch (ApiException $exception) {
$apiContext = self::createApiContext();
$apiContext->ensureSessionActive();
} catch (BunqException $exception) {
$apiContext = self::createApiContext();
}
Expand Down