Skip to content
This repository was archived by the owner on Jan 29, 2020. It is now read-only.

Provide a dedicated service for server request error response generation #562

Conversation

weierophinney
Copy link
Member

@weierophinney weierophinney commented Feb 22, 2018

Provide a dedicated service for the server request error response generator

Instead of re-using the existing ErrorResponseGenerator, this patch does the following:

  • Separates functionality for generating the error response into a trait, Zend\Expressive\Response\ErrorResponseGeneratorTrait. The method prepareTemplatedResponse() now expects the error, the response, and an array of data to provide to the template.
  • Updates Zend\Expressive\Middleware\ErrorResponseGenerator to use the new trait.
  • Creates a new class, Zend\Expressive\Response\ServerRequestErrorResponseGenerator, which expects a response factory, and optionally the debug flag, renderer, and template to render to. It composes the trait, and generates a response on-the-fly to pass to the trait methods.

The RequestHandlerRunnerFactory has been updated to use this new service, and the ServerRequestErrorResponseGeneratorFactory was updated to generate an instance of the new class.

These changes mean the constant SERVER_REQUEST_ERROR_RESPONSE_GENERATOR is no longer needed, and was thus removed.

Copy link
Member

@michalbundyra michalbundyra left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I had a hope we can avoid using constants at all...

@weierophinney weierophinney force-pushed the hotfix/use-constants-not-virtual-services branch from 8acb272 to ecbe016 Compare February 22, 2018 16:25
@weierophinney weierophinney changed the title Use the SERVER_REQUEST_ERROR_RESPONSE_GENERATOR constant Provide a dedicated service for server request error response generation Feb 22, 2018
@weierophinney
Copy link
Member Author

@webimpress Please re-review; this removes the constant, and provides a dedicated service.

public function __invoke(Throwable $e)
{
$response = ($this->responseFactory)()
->withStatus(Utils::getStatusCode($e, $response));
Copy link
Member

Choose a reason for hiding this comment

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

$response is not yet defined so can't be used 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.

Good catch - and demonstrates I need to fully test this class. I'll push more commits momentarily.

@weierophinney weierophinney force-pushed the hotfix/use-constants-not-virtual-services branch from 7099eaf to 85b832e Compare February 22, 2018 17:39
…erator

Instead of re-using the existing `ErrorResponseGenerator`, this patch
does the following:

- Separates functionality for generating the error response into a
  trait, `Zend\Expressive\Response\ErrorResponseGeneratorTrait`. The
  method `prepareTemplatedResponse()` now expects the error, the
  response, and an array of data to provide to the template.
- Updates `Zend\Expressive\Middleware\ErrorResponseGenerator` to use the
  new trait.
- Creates a new class, `Zend\Expressive\Response\ServerRequestErrorResponseGenerator`,
  which expects a response factory, and optionally the debug flag,
  renderer, and template to render to. It composes the trait, and
  generates a response on-the-fly to pass to the trait methods.

The `RequestHandlerRunnerFactory` has been updated to use this new
service, and the `ServerRequestErrorResponseGeneratorFactory` was
updated to generate an instance of the new class.

These changes mean the constant SERVER_REQUEST_ERROR_RESPONSE_GENERATOR
is no longer needed, and was thus removed.
Corrects an issue in the default message when a stack trace is produced
as well.
Updates the methods of the `ErrorResponseGeneratorTrait` to accept the
renderer and debug flag as arguments instead of relying on internal
state; this ensures that they are of the type expected.

Updated both implementations to pass the new arguments.
@weierophinney weierophinney force-pushed the hotfix/use-constants-not-virtual-services branch from 85b832e to 457bc1f Compare February 22, 2018 20:47
@weierophinney weierophinney merged commit 457bc1f into zendframework:release-3.0.0 Feb 22, 2018
@weierophinney weierophinney deleted the hotfix/use-constants-not-virtual-services branch February 22, 2018 20:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants