Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Catch exceptions thrown during rendering #2812

Closed
wants to merge 11 commits into from

3 participants

@radnan

Proposed fix for #2528. Not sure if this is the best way to go about this, but seems to work for all the cases I've tried so far. Critique very much welcome.

library/Zend/Mvc/Application.php
@@ -323,7 +323,17 @@ protected function completeRequest(MvcEvent $event)
{
$events = $this->getEventManager();
$event->setTarget($this);
- $events->trigger(MvcEvent::EVENT_RENDER, $event);
+ try {
+ ob_start();
+ $events->trigger(MvcEvent::EVENT_RENDER, $event);
+ ob_end_flush();
+ } catch (\Exception $ex) {
+ ob_end_clean();
+ $event->setError($this::ERROR_EXCEPTION)
+ ->setParam('exception', $ex);
+ $events->trigger(MvcEvent::EVENT_DISPATCH_ERROR, $event);

Trigger MvcEvent::EVENT_DISPATCH_ERROR have no sense here for me. Dispatch is finished here :)

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

@blanchonvincent agreed. I've switched it to a new 'render.error' event.

@weierophinney weierophinney commented on the diff
library/Zend/Mvc/Application.php
@@ -323,7 +323,14 @@ protected function completeRequest(MvcEvent $event)
{
$events = $this->getEventManager();
$event->setTarget($this);
- $events->trigger(MvcEvent::EVENT_RENDER, $event);
+ try {
+ $events->trigger(MvcEvent::EVENT_RENDER, $event);
+ } catch (\Exception $ex) {
+ $event->setError($this::ERROR_EXCEPTION)
+ ->setParam('exception', $ex);
+ $events->trigger(MvcEvent::EVENT_RENDER_ERROR, $event);
+ $events->trigger(MvcEvent::EVENT_RENDER, $event);
+ }
@weierophinney Owner

I'm thinking we should do the same as we do with route and dispatch, and create a Zend\Mvc\RenderListener that encapsulates this. It'll be more consistent, allow swapping out internals more easily, and keep the functionality more readable.

:+1: with the RenderListener to keep consistency & readable.

@weierophinney Owner

Actually, on investigating this, we already have a "RenderListener" -- it's the DefaultRenderingStrategy. What we need to do is treat this like we do dispatch and route: push the exception handling into these default listeners (I use the plural, as we have both Console and HTTP variants). I'm tackling this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Mvc/View/Http/InjectViewModelListener.php
@@ -83,6 +84,9 @@ public function injectViewModel(MvcEvent $e)
return;
}
+ if ($e->getError()) {
+ $model->clearChildren();
+ }
@weierophinney Owner

I wonder if we should do this, or provide an alternate layout view model instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/View/Model/ModelInterface.php
@@ -128,6 +128,13 @@ public function getChildren();
public function hasChildren();
/**
+ * Clears out all child models
+ *
+ * @return ModelInterface
+ */
+ public function clearChildren();
@weierophinney Owner

This is a backwards incompatible break. We should add it as a separate interface, and then implement it in the concrete implementations we have; anything that requires its existence would check against it before operating on it.

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

Looking like a nice solution; just need to address a few points.

@weierophinney weierophinney referenced this pull request from a commit in weierophinney/zf2
@weierophinney weierophinney [#2812][#2528] Move exception handling into DefaultRenderingStrategy
- For symmetry with DispatchListener and RouteListener, moved the
  exception handling out of Application and into
  Zend\Mvc\View\Http\DefaultRenderingStrategy.
- Reviewed Zend\Mvc\View\Console\DefaultRenderingStrategy to see if
  exception handling was necessary; it wasn't, but cleaned up a few CS
  issues in the process.
fee0bad
@weierophinney weierophinney referenced this pull request from a commit in weierophinney/zf2
@weierophinney weierophinney [#2812][#2528] Fix newly failing test
- Test now had an expectation that was no longer necessary: that the
  output would continue to be captured and present. Since the
  PhpRenderer now catches exceptions and re-throws, the output buffering
  is handled slightly differently in exceptional circumstances.
bd10918
@weierophinney weierophinney referenced this pull request from a commit in weierophinney/zf2
@weierophinney weierophinney [#2812][#2528] Moved clearChildren into separate interface
- Cannot add methods to existing interfaces; breaks BC
- Created ClearableModelInterface with clearChildren(), and also added
  clearVariables() and clearOptions().
- ViewModel implements ModelInterface and ClearableModelInterface.
- InjectViewModelListener now tests for ClearableModelInterface before
  attempting to clearChildren()
43465cc
This was referenced
@weierophinney

Closed, as #2959 has been merged, containing the patches in this request as well.

@ghost Unknown referenced this pull request from a commit
@weierophinney weierophinney [#2812][#2528] Move exception handling into DefaultRenderingStrategy
- For symmetry with DispatchListener and RouteListener, moved the
  exception handling out of Application and into
  Zend\Mvc\View\Http\DefaultRenderingStrategy.
- Reviewed Zend\Mvc\View\Console\DefaultRenderingStrategy to see if
  exception handling was necessary; it wasn't, but cleaned up a few CS
  issues in the process.
beb9789
@ghost Unknown referenced this pull request from a commit
@weierophinney weierophinney [#2812][#2528] Fix newly failing test
- Test now had an expectation that was no longer necessary: that the
  output would continue to be captured and present. Since the
  PhpRenderer now catches exceptions and re-throws, the output buffering
  is handled slightly differently in exceptional circumstances.
bea1df0
@ghost Unknown referenced this pull request from a commit
@weierophinney weierophinney [#2812][#2528] Moved clearChildren into separate interface
- Cannot add methods to existing interfaces; breaks BC
- Created ClearableModelInterface with clearChildren(), and also added
  clearVariables() and clearOptions().
- ViewModel implements ModelInterface and ClearableModelInterface.
- InjectViewModelListener now tests for ClearableModelInterface before
  attempting to clearChildren()
e45be1d
@weierophinney weierophinney referenced this pull request from a commit in zendframework/zend-view
@weierophinney weierophinney [zendframework/zf2#2812][zendframework/zf2#2528] Fix newly failing test
- Test now had an expectation that was no longer necessary: that the
  output would continue to be captured and present. Since the
  PhpRenderer now catches exceptions and re-throws, the output buffering
  is handled slightly differently in exceptional circumstances.
d6cb976
@weierophinney weierophinney referenced this pull request from a commit in zendframework/zend-view
@weierophinney weierophinney [zendframework/zf2#2812][zendframework/zf2#2528] Moved clearChildren …
…into separate interface

- Cannot add methods to existing interfaces; breaks BC
- Created ClearableModelInterface with clearChildren(), and also added
  clearVariables() and clearOptions().
- ViewModel implements ModelInterface and ClearableModelInterface.
- InjectViewModelListener now tests for ClearableModelInterface before
  attempting to clearChildren()
3717606
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 19, 2012
  1. @radnan

    Catch render exceptions and trigger dispatch error

    radnan authored
    Catch exceptions thrown when EVENT_RENDER is triggered, clean up all output, trigger EVENT_DISPATCH_ERROR, and finally re-trigger EVENT_RENDER
  2. @radnan
  3. @radnan
  4. @radnan

    clear out child models

    radnan authored
  5. @radnan

    Implemented clearChildren()

    radnan authored
Commits on Oct 20, 2012
  1. @radnan

    Opening brace on same-line

    radnan authored
  2. @radnan

    Trigger render_error event

    radnan authored
  3. @radnan
  4. @radnan

    Attach to render.error

    radnan authored
  5. @radnan

    Attach to 'render.error'

    radnan authored
  6. @radnan

    Removed unnecessary cleanup

    radnan authored
This page is out of date. Refresh to see the latest.
View
9 library/Zend/Mvc/Application.php
@@ -323,7 +323,14 @@ protected function completeRequest(MvcEvent $event)
{
$events = $this->getEventManager();
$event->setTarget($this);
- $events->trigger(MvcEvent::EVENT_RENDER, $event);
+ try {
+ $events->trigger(MvcEvent::EVENT_RENDER, $event);
+ } catch (\Exception $ex) {
+ $event->setError($this::ERROR_EXCEPTION)
+ ->setParam('exception', $ex);
+ $events->trigger(MvcEvent::EVENT_RENDER_ERROR, $event);
+ $events->trigger(MvcEvent::EVENT_RENDER, $event);
+ }
@weierophinney Owner

I'm thinking we should do the same as we do with route and dispatch, and create a Zend\Mvc\RenderListener that encapsulates this. It'll be more consistent, allow swapping out internals more easily, and keep the functionality more readable.

:+1: with the RenderListener to keep consistency & readable.

@weierophinney Owner

Actually, on investigating this, we already have a "RenderListener" -- it's the DefaultRenderingStrategy. What we need to do is treat this like we do dispatch and route: push the exception handling into these default listeners (I use the plural, as we have both Console and HTTP variants). I'm tackling this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
$events->trigger(MvcEvent::EVENT_FINISH, $event);
return $event->getResponse();
}
View
1  library/Zend/Mvc/MvcEvent.php
@@ -30,6 +30,7 @@ class MvcEvent extends Event
const EVENT_DISPATCH_ERROR = 'dispatch.error';
const EVENT_FINISH = 'finish';
const EVENT_RENDER = 'render';
+ const EVENT_RENDER_ERROR = 'render.error';
const EVENT_ROUTE = 'route';
/**#@-*/
View
1  library/Zend/Mvc/View/Console/ExceptionStrategy.php
@@ -61,6 +61,7 @@ class ExceptionStrategy implements ListenerAggregateInterface
public function attach(EventManagerInterface $events)
{
$this->listeners[] = $events->attach(MvcEvent::EVENT_DISPATCH_ERROR, array($this, 'prepareExceptionViewModel'));
+ $this->listeners[] = $events->attach(MvcEvent::EVENT_RENDER_ERROR, array($this, 'prepareExceptionViewModel'));
}
/**
View
1  library/Zend/Mvc/View/Console/ViewManager.php
@@ -67,6 +67,7 @@ public function onBootstrap($event)
$events->attach($routeNotFoundStrategy);
$events->attach($exceptionStrategy);
$events->attach(MvcEvent::EVENT_DISPATCH_ERROR, array($injectViewModelListener, 'injectViewModel'), -100);
+ $events->attach(MvcEvent::EVENT_RENDER_ERROR, array($injectViewModelListener, 'injectViewModel'), -100);
$events->attach($mvcRenderingStrategy);
$events->attach($sendResponseListener);
View
1  library/Zend/Mvc/View/Http/ExceptionStrategy.php
@@ -51,6 +51,7 @@ class ExceptionStrategy implements ListenerAggregateInterface
public function attach(EventManagerInterface $events)
{
$this->listeners[] = $events->attach(MvcEvent::EVENT_DISPATCH_ERROR, array($this, 'prepareExceptionViewModel'));
+ $this->listeners[] = $events->attach(MvcEvent::EVENT_RENDER_ERROR, array($this, 'prepareExceptionViewModel'));
}
/**
View
4 library/Zend/Mvc/View/Http/InjectViewModelListener.php
@@ -42,6 +42,7 @@ public function attach(Events $events)
{
$this->listeners[] = $events->attach(MvcEvent::EVENT_DISPATCH, array($this, 'injectViewModel'), -100);
$this->listeners[] = $events->attach(MvcEvent::EVENT_DISPATCH_ERROR, array($this, 'injectViewModel'), -100);
+ $this->listeners[] = $events->attach(MvcEvent::EVENT_RENDER_ERROR, array($this, 'injectViewModel'), -100);
}
/**
@@ -83,6 +84,9 @@ public function injectViewModel(MvcEvent $e)
return;
}
+ if ($e->getError()) {
+ $model->clearChildren();
+ }
@weierophinney Owner

I wonder if we should do this, or provide an alternate layout view model instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
$model->addChild($result);
}
}
View
1  library/Zend/Mvc/View/Http/ViewManager.php
@@ -146,6 +146,7 @@ public function onBootstrap($event)
$events->attach($routeNotFoundStrategy);
$events->attach($exceptionStrategy);
$events->attach(MvcEvent::EVENT_DISPATCH_ERROR, array($injectViewModelListener, 'injectViewModel'), -100);
+ $events->attach(MvcEvent::EVENT_RENDER_ERROR, array($injectViewModelListener, 'injectViewModel'), -100);
$events->attach($mvcRenderingStrategy);
$events->attach($sendResponseListener);
View
7 library/Zend/View/Model/ModelInterface.php
@@ -128,6 +128,13 @@ public function getChildren();
public function hasChildren();
/**
+ * Clears out all child models
+ *
+ * @return ModelInterface
+ */
+ public function clearChildren();
@weierophinney Owner

This is a backwards incompatible break. We should add it as a separate interface, and then implement it in the concrete implementations we have; anything that requires its existence would check against it before operating on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ /**
* Set the name of the variable to capture this model to, if it is a child model
*
* @param string $capture
View
11 library/Zend/View/Model/ViewModel.php
@@ -352,6 +352,17 @@ public function hasChildren()
}
/**
+ * Clears out all child models
+ *
+ * @return ViewModel
+ */
+ public function clearChildren()
+ {
+ $this->children = array();
+ return $this;
+ }
+
+ /**
* Set the name of the variable to capture this model to, if it is a child model
*
* @param string $capture
View
11 library/Zend/View/Renderer/PhpRenderer.php
@@ -457,9 +457,14 @@ public function render($nameOrModel, $values = null)
$this->__template
));
}
- ob_start();
- include $this->__file;
- $this->__content = ob_get_clean();
+ try {
+ ob_start();
+ include $this->__file;
+ $this->__content = ob_get_clean();
+ } catch (\Exception $ex) {
+ ob_end_clean();
+ throw $ex;
+ }
}
$this->setVars(array_pop($this->__varsCache));
Something went wrong with that request. Please try again.