Added custom layout support in NotFoundDelegate #511
Conversation
There was a problem hiding this comment.
Disclaimer: I don't use zend-view myself, so I have no idea how its layout system works.
Shouldn't this be added somehow to zend-expressive-zendviewrenderer, as it's specific for zend-view only? No idea how that woud work though. If it isn't possible add a zendviewrenderer specific NotFoundDelegate in that package?
src/Delegate/NotFoundDelegate.php
Outdated
| $response = $this->responsePrototype->withStatus(StatusCodeInterface::STATUS_NOT_FOUND); | ||
| $response->getBody()->write( | ||
| $this->renderer->render($this->template, ['request' => $request]) | ||
| $this->renderer->render($this->template, $templateData) |
There was a problem hiding this comment.
Why add a new variable?
$this->renderer->render($this->template, [
'request' => $request,
'layout' => $this->layout,
]);There was a problem hiding this comment.
I will rework this point to make the parameter optional - or do you have a better idea? Another option would be that in the factory other optional parameters can be passed to the renderer no matter what...
| @@ -58,7 +67,7 @@ public function __construct( | |||
| */ | |||
| public function process(ServerRequestInterface $request) | |||
| { | |||
| if (! $this->renderer) { | |||
| if (!$this->renderer) { | |||
There was a problem hiding this comment.
Revert this change please. CS is now failing:
There must be a single space after a NOT operator; 0 found
| @@ -78,7 +87,7 @@ private function generatePlainTextResponse(ServerRequestInterface $request) | |||
| ->write(sprintf( | |||
| 'Cannot %s %s', | |||
| $request->getMethod(), | |||
| (string) $request->getUri() | |||
| (string)$request->getUri() | |||
There was a problem hiding this comment.
Revert this change please. We don;t have currently sniff to check it, so all seems to be fine, but for consistency we should have here single space.
| $renderer->render(NotFoundDelegate::TEMPLATE_DEFAULT, [ | ||
| 'request' => $request, | ||
| 'layout' => NotFoundDelegate::LAYOUT_DEFAULT | ||
| ])->willReturn('CONTENT'); |
There was a problem hiding this comment.
The last line should have one less indent:
$renderer->render(NotFoundDelegate::TEMPLATE_DEFAULT, [
'request' => $request,
'layout' => NotFoundDelegate::LAYOUT_DEFAULT
])->willReturn('CONTENT');| ->willReturn('CONTENT'); | ||
| $renderer->render(NotFoundDelegate::TEMPLATE_DEFAULT, [ | ||
| 'request' => $request, | ||
| 'layout' => NotFoundDelegate::LAYOUT_DEFAULT |
There was a problem hiding this comment.
Please add trailing comma after last element in the array
There was a problem hiding this comment.
One minor change necessary to make this backwards compatible; I'll make that change, and the other CS changes, on merge.
Thanks, @mano87
| @@ -16,6 +16,7 @@ | |||
| class NotFoundDelegate implements DelegateInterface | |||
| { | |||
| const TEMPLATE_DEFAULT = 'error::404'; | |||
| const LAYOUT_DEFAULT = 'layout::error'; | |||
There was a problem hiding this comment.
The main problem I have with this is that we do not at this time define a layout::error template in the skeleton application, only a layout::default. As such, I think that should likely be the default in this situation, to allow for backwards compatibility with existing applications.
Added custom layout support in NotFoundDelegate
|
Thanks, @mano87 |
This is a better implementation for chaning the layout of error pages. See also the issue: #360
I hope my pull request will be accepted :-)