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

Change Formatter interface to allow returning complete PSR 7 response #50

Merged
merged 3 commits into from
Jul 2, 2020
Merged

Change Formatter interface to allow returning complete PSR 7 response #50

merged 3 commits into from
Jul 2, 2020

Conversation

hiqsol
Copy link
Contributor

@hiqsol hiqsol commented Jun 24, 2020

Continuing #49

Not finished: not all tests fixed.
Let me know if it is ok to you and I'll finish.

Pay attention to ContentTypeMiddleware::NOT_ACCEPTABLE constant and getNotAcceptableFormatter().
It allows redefine formatter for not acceptable case in the formatters map.

@hiqsol hiqsol marked this pull request as draft June 24, 2020 17:58
@hiqsol hiqsol force-pushed the allow-raw-formatter branch from dca9c89 to 8ae16b7 Compare June 25, 2020 12:54
@hiqsol hiqsol changed the title Allow raw formatter returning complete PSR 7 response Change Formatter interface to allow returning complete PSR 7 response Jun 25, 2020
@hiqsol hiqsol force-pushed the allow-raw-formatter branch from 9f2cecb to a14fcec Compare June 25, 2020 13:17
@hiqsol
Copy link
Contributor Author

hiqsol commented Jun 25, 2020

Fixed all tests and code style.
Build fails at mutation tests - don't know how to fix it.

@hiqsol
Copy link
Contributor Author

hiqsol commented Jun 25, 2020

AFAICS all the mutations found by infection are covered by tests :-/

@hiqsol
Copy link
Contributor Author

hiqsol commented Jul 1, 2020

@lcobucci please take a look

@lcobucci
Copy link
Owner

lcobucci commented Jul 2, 2020

@hiqsol sorry, about the delay... way too busy with paid work.

lcobucci
lcobucci requested changes Jul 2, 2020
View reviewed changes
Copy link
Owner

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

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

Since you had some urgency here I'll handle this 👍

@lcobucci lcobucci added this to the 2.3.0 milestone Jul 2, 2020
lcobucci
lcobucci reviewed Jul 2, 2020
View reviewed changes
src/ContentTypeMiddleware.php Outdated
@@ -66,9 +67,9 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface
}

$contentType = $this->extractContentType($response->getHeaderLine('Content-Type'));
$formatter = $this->formatters[$contentType] ?? null;
$formatter = $this->formatters[$contentType] ?? $this->getNotAcceptableFormatter();
Copy link
Owner

Choose a reason for hiding this comment

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

I'm modifying this slightly to ensure that we always have the formatter when instantiating this, which removes a method call from this bit.

It's a very small performance gain for normal PHP usage but for handling things with async php is a nice thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I've chosen the laziest variant exactly for this small gain because this object is just not needed in normal (non error) cases.
But it is such a tiny difference that it is just unimportant

Copy link
Owner

Choose a reason for hiding this comment

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

Exactly, which is why I'm thinking if it makes sense to actually cache this instance... I mean, in a ideal scenario it will hardly reach that point.

@lcobucci lcobucci force-pushed the allow-raw-formatter branch from b2410dd to 7440587 Compare July 2, 2020 15:36
@lcobucci lcobucci marked this pull request as ready for review July 2, 2020 15:37
@lcobucci lcobucci force-pushed the allow-raw-formatter branch from 7440587 to b738629 Compare July 2, 2020 15:37
@lcobucci lcobucci force-pushed the allow-raw-formatter branch from b738629 to 4faacd1 Compare July 2, 2020 15:38
@lcobucci
Copy link
Owner

lcobucci commented Jul 2, 2020

@hiqsol Alright, I've reorganised your commit and now everything should be passing 👍

@hiqsol @lcobucci
Make formatters rely on PSR-7 responses
90b7f26 
Providing more flexibility to users and simplifying the API.
@lcobucci lcobucci force-pushed the allow-raw-formatter branch from 4faacd1 to 90b7f26 Compare July 2, 2020 15:42
@lcobucci lcobucci self-assigned this Jul 2, 2020
@lcobucci lcobucci added BC-break This pull request breaks backwards compatibility Improvement Improvement of existing feature labels Jul 2, 2020
@lcobucci
Copy link
Owner

lcobucci commented Jul 2, 2020

After merging it I'll just handle #48 and release v3.0.
Thanks a lot for your help here!

@lcobucci lcobucci merged commit 1c1e0b2 into lcobucci:master Jul 2, 2020
@lcobucci
Copy link
Owner

lcobucci commented Jul 2, 2020

@hiqsol released https://github.com/lcobucci/content-negotiation-middleware/releases/tag/3.0.0 🚢

@hiqsol
Copy link
Contributor Author

hiqsol commented Jul 3, 2020

Cool! Thanks! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcobucci lcobucci lcobucci approved these changes

Assignees

@lcobucci lcobucci

Labels
BC-break This pull request breaks backwards compatibility Improvement Improvement of existing feature
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@hiqsol @lcobucci