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.

Sign up for GitHub

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

Jump to bottom

Exception handling plugin #4

Merged
merged 2 commits into from Nov 13, 2017
Merged

Exception handling plugin #4

merged 2 commits into from Nov 13, 2017

Conversation

OndraM
Copy link
Member

@OndraM OndraM commented Nov 10, 2017 •
edited

Also part of the internal HTTP layer. Will transfer error responses from Matej API to custom Exceptions.

It will be used internally in the library as another HTTPlug request plugin (http://docs.php-http.org/en/latest/plugins/build-your-own.html).

$pluginClient = new PluginClient(
    $this->getHttpClient(),
    [
        // ... other plugins, like AuthenticationPlugin
        new ExceptionPlugin(),
    ]
);

@OndraM OndraM changed the base branch from master to feature/authentication November 10, 2017 14:45
@OndraM OndraM force-pushed the feature/exceptions branch 2 times, most recently from 8873842 to 2efb101 Compare November 10, 2017 15:26
jirinovak
jirinovak reviewed Nov 12, 2017
View reviewed changes
src/Exception/AuthorizationException.php Outdated
ResponseInterface $response,
\Throwable $previous = null
) {
$responseData = json_decode($response->getBody()->getContents(), false);
Copy link
Member

Choose a reason for hiding this comment

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

false is the default

src/Exception/AuthorizationException.php Outdated
use Psr\Http\Message\ResponseInterface;

/**
* Exception thrown when
Copy link
Member

Choose a reason for hiding this comment

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

Ok, when? :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a question!

Fixed :)

src/Http/Plugin/ExceptionPlugin.php
}

if ($responseCode >= 400 && $responseCode < 600) {
// TODO: use more specific exceptions
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could create issue and link it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is/will be more of this, the whole repo is TODO right now :-D. I will make sure fix all TODOs before the 1.0 version (or do issues where needed), though.

tests/Http/Plugin/ExceptionPluginTest.php Outdated
public function init(): void
{
$this->emptyFunction = function (): void {
};
Copy link
Member

Choose a reason for hiding this comment

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

same line as {?

Copy link
Member Author

Choose a reason for hiding this comment

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

Its (unfortunately, in this case) forbidden by PSR-2 and causes php-cs-fixer to cry.

Opening braces for control structures MUST go on the same line, and closing braces MUST go on the next line after the body.

¯\_(ツ)_/¯

Copy link
Contributor

@foglcz foglcz Nov 13, 2017 •
edited

Choose a reason for hiding this comment

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

Technically, function isn't a control structure (edit: I think 😄)

In php-cs-fixer, they mention allow_single_line_closure (defaults to false). Maybe that'd help?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would sey they are, but this is kinda gray zone. However, nice finding about the allow_single_line_closure setting! I'm for adding this to configuration and us the one-liner. This newline makes it surely less readable for no reason.

tests/Http/Plugin/ExceptionPluginTest.php
'HTTP 400' => [StatusCodeInterface::STATUS_BAD_REQUEST, RequestException::class],
'HTTP 401' => [StatusCodeInterface::STATUS_UNAUTHORIZED, AuthorizationException::class],
'HTTP 404' => [StatusCodeInterface::STATUS_NOT_FOUND, RequestException::class],
'HTTP 500' => [StatusCodeInterface::STATUS_INTERNAL_SERVER_ERROR, RequestException::class],
Copy link
Member

Choose a reason for hiding this comment

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

What about testing < 600 boundary.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no HTTP 600 :). Byt I added the check for (imaginary) HTTP 599 code as well.

@OndraM OndraM changed the base branch from feature/authentication to master November 13, 2017 11:55
@OndraM OndraM merged commit e45e141 into master Nov 13, 2017
@OndraM OndraM deleted the feature/exceptions branch November 13, 2017 12:51
@OndraM OndraM added this to the 0.9 milestone Nov 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@foglcz foglcz foglcz approved these changes

@jirinovak jirinovak jirinovak approved these changes

@kubasimon kubasimon Awaiting requested review from kubasimon

@davidkmenta davidkmenta Awaiting requested review from davidkmenta

@kdosiodjinud kdosiodjinud Awaiting requested review from kdosiodjinud

@MortalFlesh MortalFlesh Awaiting requested review from MortalFlesh

@hokypierce hokypierce Awaiting requested review from hokypierce

@MarketaSebkova MarketaSebkova Awaiting requested review from MarketaSebkova

@florianjiri florianjiri Awaiting requested review from florianjiri

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.9
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@OndraM @foglcz @jirinovak