Browse Source

Fix output buffers not being closed when the view triggers an exception.

When evaluating a template or caching a block, the view itself opens a
buffer, and the templates/blocks that it evaluates might open buffers
too. In case an exception happens, the opened buffers will not be
closed, causing risky tests with PHPUnit, and possible further output to
be swallowed unless the test itself closes/flushes the remaining
buffers.
ndm2 5 years ago
parent
commit
ec43ae7ad0

+ 25 - 2
src/View/View.php

@@ -34,6 +34,7 @@ use Cake\View\Exception\MissingTemplateException;
 use InvalidArgumentException;
 use LogicException;
 use RuntimeException;
+use Throwable;
 
 /**
  * View, the V in the MVC triad. View interacts with Helpers and view variables passed
@@ -687,8 +688,20 @@ class View implements EventDispatcherInterface
         if ($result) {
             return $result;
         }
+
+        $bufferLevel = ob_get_level();
         ob_start();
-        $block();
+
+        try {
+            $block();
+        } catch (Throwable $exception) {
+            while (ob_get_level() > $bufferLevel) {
+                ob_end_clean();
+            }
+
+            throw $exception;
+        }
+
         $result = ob_get_clean();
 
         Cache::write($options['key'], $result, $options['config']);
@@ -1155,9 +1168,19 @@ class View implements EventDispatcherInterface
     protected function _evaluate(string $templateFile, array $dataForView): string
     {
         extract($dataForView);
+
+        $bufferLevel = ob_get_level();
         ob_start();
 
-        include func_get_arg(0);
+        try {
+            include func_get_arg(0);
+        } catch (Throwable $exception) {
+            while (ob_get_level() > $bufferLevel) {
+                ob_end_clean();
+            }
+
+            throw $exception;
+        }
 
         return ob_get_clean();
     }

+ 61 - 4
tests/TestCase/View/ViewTest.php

@@ -1635,7 +1635,6 @@ TEXT;
             $this->View->render('extend_self', false);
             $this->fail('No exception');
         } catch (\LogicException $e) {
-            ob_end_clean();
             $this->assertStringContainsString('cannot have templates extend themselves', $e->getMessage());
         }
     }
@@ -1651,7 +1650,6 @@ TEXT;
             $this->View->render('extend_loop', false);
             $this->fail('No exception');
         } catch (\LogicException $e) {
-            ob_end_clean();
             $this->assertStringContainsString('cannot have templates extend in a loop', $e->getMessage());
         }
     }
@@ -1704,8 +1702,6 @@ TEXT;
             $this->View->render('extend_missing_element', false);
             $this->fail('No exception');
         } catch (\LogicException $e) {
-            ob_end_clean();
-            ob_end_clean();
             $this->assertStringContainsString('element', $e->getMessage());
         }
     }
@@ -1745,6 +1741,67 @@ TEXT;
     }
 
     /**
+     * Tests that the buffers that are opened when evaluating a template
+     * are being closed in case an exception happens.
+     *
+     * @return void
+     */
+    public function testBuffersOpenedDuringTemplateEvaluationAreBeingClosed()
+    {
+        $bufferLevel = ob_get_level();
+
+        $e = null;
+        try {
+            $this->View->element('exception_with_open_buffers');
+        } catch (\Exception $e) {
+        }
+
+        $this->assertNotNull($e);
+        $this->assertEquals('Exception with open buffers', $e->getMessage());
+        $this->assertEquals($bufferLevel, ob_get_level());
+    }
+
+    /**
+     * Tests that the buffers that are opened during block caching are
+     * being closed in case an exception happens.
+     *
+     * @return void
+     */
+    public function testBuffersOpenedDuringBlockCachingAreBeingClosed()
+    {
+        Cache::drop('test_view');
+        Cache::setConfig('test_view', [
+            'engine' => 'File',
+            'duration' => '+1 day',
+            'path' => CACHE . 'views/',
+            'prefix' => '',
+        ]);
+        Cache::clear('test_view');
+
+        $bufferLevel = ob_get_level();
+
+        $e = null;
+        try {
+            $this->View->cache(function () {
+                ob_start();
+
+                throw new \Exception('Exception with open buffers');
+            }, [
+                'key' => __FUNCTION__,
+                'config' => 'test_view',
+            ]);
+        } catch (\Exception $e) {
+        }
+
+        Cache::clear('test_view');
+        Cache::drop('test_view');
+
+        $this->assertNotNull($e);
+        $this->assertEquals('Exception with open buffers', $e->getMessage());
+        $this->assertEquals($bufferLevel, ob_get_level());
+    }
+
+    /**
      * Test memory leaks that existed in _paths at one point.
      *
      * @return void

+ 3 - 0
tests/test_app/templates/element/exception_with_open_buffers.php

@@ -0,0 +1,3 @@
+<?php
+$this->start('non closing block');
+throw new \Exception('Exception with open buffers');