Catch exceptions thrown during rendering #2812

Closed
wants to merge 11 commits into
from

Projects

None yet

3 participants

@radnan
radnan commented Oct 19, 2012

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.

@blanchonvincent blanchonvincent commented on an outdated diff Oct 20, 2012
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);
@blanchonvincent
blanchonvincent Oct 20, 2012

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

@radnan
radnan commented Oct 20, 2012

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

@weierophinney weierophinney commented on the diff Oct 30, 2012
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
weierophinney Oct 30, 2012 Zend Framework member

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.

@blanchonvincent
blanchonvincent Oct 30, 2012

👍 with the RenderListener to keep consistency & readable.

@weierophinney
weierophinney Nov 13, 2012 Zend Framework member

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.

@weierophinney weierophinney commented on the diff Oct 30, 2012
library/Zend/Mvc/View/Http/InjectViewModelListener.php
@@ -83,6 +84,9 @@ public function injectViewModel(MvcEvent $e)
return;
}
+ if ($e->getError()) {
+ $model->clearChildren();
+ }
@weierophinney
weierophinney Oct 30, 2012 Zend Framework member

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

@weierophinney weierophinney commented on the diff Oct 30, 2012
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
weierophinney Oct 30, 2012 Zend Framework member

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.

@weierophinney
Member

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

@weierophinney weierophinney added a commit to weierophinney/zendframework that referenced this pull request Nov 13, 2012
@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 added a commit to weierophinney/zendframework that referenced this pull request Nov 13, 2012
@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 added a commit to weierophinney/zendframework that referenced this pull request Nov 13, 2012
@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 Nov 13, 2012
@weierophinney
Member

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

@weierophinney weierophinney added a commit to zendframework/zend-view that referenced this pull request May 15, 2015
@weierophinney weierophinney [zendframework/zendframework#2812][zendframework/zendframework#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 added a commit to zendframework/zend-view that referenced this pull request May 15, 2015
@weierophinney weierophinney [zendframework/zendframework#2812][zendframework/zendframework#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