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

Deferred can be null + add container factories #11

Merged
merged 6 commits into from Jul 2, 2017
Merged

Deferred can be null + add container factories #11

merged 6 commits into from Jul 2, 2017

Conversation

prolic
Copy link
Member

@prolic prolic commented Jun 29, 2017 •
edited

resolves #10

  • added psr container factories

@prolic prolic added the bug label Jun 29, 2017
@prolic prolic requested a review from codeliner June 29, 2017 15:15
@prolic
Copy link
Member Author

prolic commented Jun 29, 2017

/cc @lunetics

@coveralls
Copy link

Coverage Status

Coverage increased (+1.2%) to 96.97% when pulling c0056ee on deferred_can_be_null into b997e56 on master.

@lunetics
Copy link
Member

+1! go from me

@coveralls
Copy link

Coverage Status

Coverage increased (+1.2%) to 96.97% when pulling c0056ee on deferred_can_be_null into b997e56 on master.

@prolic
Copy link
Member Author

prolic commented Jun 30, 2017

@codeliner @basz wanna review?

@prolic prolic changed the title Deferred can be null Deferred can be null + add container factories Jul 1, 2017
@prolic
Copy link
Member Author

prolic commented Jul 1, 2017

ping @codeliner

@coveralls
Copy link

Coverage Status

Coverage increased (+2.2%) to 97.959% when pulling 51918d2 on deferred_can_be_null into b997e56 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+2.2%) to 97.959% when pulling 51918d2 on deferred_can_be_null into b997e56 on master.

codeliner
codeliner requested changes Jul 1, 2017
View reviewed changes
src/HttpAsyncMessageProducer.php Outdated
// we accept only status code 2xx
if ('2' !== substr((string) $response->getStatusCode(), 0, 1)) {
if ($deferred) {
$deferred->reject($response->getReasonPhrase());
Copy link
Member

Choose a reason for hiding this comment

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

We should reject with the full error response so that also body and status code can be inspected because this can contain important information to figure out what went wrong

src/HttpAsyncMessageProducer.php Outdated
if ($deferred) {
$deferred->reject($response->getReasonPhrase());
} else {
$exception = new RuntimeException($response->getReasonPhrase());
Copy link
Member

Choose a reason for hiding this comment

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

same here, maybe a custom exception that takes the response as argument and provides a getter for the error response

@prolic
Copy link
Member Author

prolic commented Jul 2, 2017

@codeliner updated

@coveralls
Copy link

Coverage Status

Coverage increased (+2.3%) to 98.058% when pulling 597d3cc on deferred_can_be_null into b997e56 on master.

@codeliner codeliner merged commit 6915b37 into master Jul 2, 2017
@codeliner codeliner deleted the deferred_can_be_null branch July 2, 2017 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeliner codeliner codeliner approved these changes

Assignees

@codeliner codeliner

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deferred can be null
4 participants
@prolic @coveralls @lunetics @codeliner