-
Notifications
You must be signed in to change notification settings - Fork 2
Decouples AcceptAndContentTypeMiddleware from ResponseManagerInterface #3
base: master
Are you sure you want to change the base?
Decouples AcceptAndContentTypeMiddleware from ResponseManagerInterface #3
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please run composer fix:cs
and composer test
Besides that, this is an opinionated library, which since version 3 is focused on API Problem.
Is there any explanation why i should merge it ones, the change requests would be resolved?
src/Middleware/AcceptAndContentTypeMiddlewareResponseFactoryInterface.php
Outdated
Show resolved
Hide resolved
src/Middleware/AcceptAndContentTypeMiddlewareResponseFactoryInterface.php
Outdated
Show resolved
Hide resolved
* @param string $accept | ||
* @param int $status | ||
* @param NormalizerContextInterface|null $context | ||
* @return ResponseInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useless phpdoc, its already typesafe by design
|
||
public function __construct( | ||
AcceptNegotiatorInterface $acceptNegotiator, | ||
ContentTypeNegotiatorInterface $contentTypeNegotiator, | ||
ResponseManagerInterface $responseManager | ||
AcceptAndContentTypeMiddlewareResponseFactoryInterface $responseFactory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a BC, both arguments should be possible, if the old is given, @trigger_error with user deprecation
Co-Authored-By: Dominik Zogg <dominik.zogg@gmail.com>
Co-Authored-By: Dominik Zogg <dominik.zogg@gmail.com>
…terface.php Co-Authored-By: Dominik Zogg <dominik.zogg@gmail.com>
…terface.php Co-Authored-By: Dominik Zogg <dominik.zogg@gmail.com>
I think it would be great to move it out of this repository as I really want to use only the middleware but not the whole other stuff. I think the middleware really solves a common problem which not necessarily should lead to a generation of an ApiProblem. Of course if you say this is more an all-in-one closed-eco-system package I'll create an own repo and just publish a "fork" of the middleware there. And we can avoid adding additional complexity in this package. :-) |
@TiMESPLiNTER the size of this package isn't that big, so @healerz any opinion on this one? |
5bf135a
to
67409c9
Compare
2db36a8
to
9cc328f
Compare
807fa68
to
0eaa271
Compare
fdc9db1
to
bd04ffd
Compare
9be2d77
to
7d39a1e
Compare
This PR decouples
AcceptAndContentTypeMiddleware
fromResponseManagerInterface
andApiProblem
. Therefor it makes it possible to use theAcceptAndContentTypeMiddleware
as a standalone class without being dependent on the genericResponseManagerInterface
that serves many other concerns as well beside generating responses for this middleware and one is not forced to react withApiProblem
s as response if something's wrong in the middleware.Note: Some tests for the new classes are missing, naming too long as well for the new classes.