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

Decouples AcceptAndContentTypeMiddleware from ResponseManagerInterface #3

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

TiMESPLiNTER
Copy link

@TiMESPLiNTER TiMESPLiNTER commented Feb 18, 2020

This PR decouples AcceptAndContentTypeMiddleware from ResponseManagerInterface and ApiProblem. Therefor it makes it possible to use the AcceptAndContentTypeMiddleware as a standalone class without being dependent on the generic ResponseManagerInterface that serves many other concerns as well beside generating responses for this middleware and one is not forced to react with ApiProblems 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.

$responseManager = new ResponseManager(...);
$middlewareResponseManager = new AcceptAndContentTypeResponseManager($responseManager);

$middleware = new AcceptAndContentTypeMiddleware(
    $acceptNegotiator,
    $contentTypeNegotiator,
    $middlewareResponseManager
);

Copy link
Member

@dominikzogg dominikzogg left a 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?

* @param string $accept
* @param int $status
* @param NormalizerContextInterface|null $context
* @return ResponseInterface
Copy link
Member

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
Copy link
Member

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

TiMESPLiNTER and others added 4 commits February 18, 2020 14:43
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>
@TiMESPLiNTER
Copy link
Author

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. :-)

@dominikzogg
Copy link
Member

dominikzogg commented Feb 19, 2020

@TiMESPLiNTER the size of this package isn't that big, so all-in-one wouldn't be the first time i would choose. I think copy'n'paste of the middleware (project or in a separate library) if its the only thing you need. Probably it is the easier solution.

@healerz any opinion on this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants