From d7a186f5ca088c13d3b10ba5e8d07d61cc73200b Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Sun, 27 Apr 2014 09:44:26 +0000 Subject: [PATCH] [Debug] less intrusive work around for https://bugs.php.net/54275 --- src/Symfony/Component/Debug/CHANGELOG.md | 2 - src/Symfony/Component/Debug/ErrorHandler.php | 150 ++++++++++++++---- .../Debug/Exception/ContextErrorException.php | 19 ++- .../Debug/Exception/FatalErrorException.php | 2 +- .../Debug/Exception/HandledErrorException.php | 86 ---------- .../Debug/Tests/DebugClassLoaderTest.php | 37 ++--- .../Debug/Tests/ErrorHandlerTest.php | 55 +++---- .../EventListener/ErrorsLoggerListener.php | 3 +- .../FatalErrorExceptionsListener.php | 4 +- .../Fragment/InlineFragmentRenderer.php | 12 +- .../Component/HttpKernel/HttpKernel.php | 5 - 11 files changed, 176 insertions(+), 199 deletions(-) delete mode 100644 src/Symfony/Component/Debug/Exception/HandledErrorException.php diff --git a/src/Symfony/Component/Debug/CHANGELOG.md b/src/Symfony/Component/Debug/CHANGELOG.md index 3823fc8783..b128efaaa8 100644 --- a/src/Symfony/Component/Debug/CHANGELOG.md +++ b/src/Symfony/Component/Debug/CHANGELOG.md @@ -4,11 +4,9 @@ CHANGELOG 2.5.0 ----- -* added HandledErrorException * added ErrorHandler::setFatalErrorExceptionHandler() * added UndefinedMethodFatalErrorHandler * deprecated ExceptionHandlerInterface -* deprecated ContextErrorException * deprecated DummyException 2.4.0 diff --git a/src/Symfony/Component/Debug/ErrorHandler.php b/src/Symfony/Component/Debug/ErrorHandler.php index 46296f45e7..e1bad044ba 100644 --- a/src/Symfony/Component/Debug/ErrorHandler.php +++ b/src/Symfony/Component/Debug/ErrorHandler.php @@ -13,9 +13,8 @@ namespace Symfony\Component\Debug; use Psr\Log\LogLevel; use Psr\Log\LoggerInterface; +use Symfony\Component\Debug\Exception\ContextErrorException; use Symfony\Component\Debug\Exception\FatalErrorException; -use Symfony\Component\Debug\Exception\DummyException; -use Symfony\Component\Debug\Exception\HandledErrorException; use Symfony\Component\Debug\FatalErrorHandler\UndefinedFunctionFatalErrorHandler; use Symfony\Component\Debug\FatalErrorHandler\UndefinedMethodFatalErrorHandler; use Symfony\Component\Debug\FatalErrorHandler\ClassNotFoundFatalErrorHandler; @@ -54,6 +53,8 @@ class ErrorHandler private $displayErrors; + private $caughtOutput = 0; + /** * @var LoggerInterface[] Loggers for channels */ @@ -129,7 +130,7 @@ class ErrorHandler } /** - * @throws HandledErrorException When error_reporting returns error + * @throws ContextErrorException When error_reporting returns error */ public function handle($level, $message, $file = 'unknown', $line = 0, $context = array()) { @@ -159,36 +160,24 @@ class ErrorHandler } if ($this->displayErrors && error_reporting() & $level && $this->level & $level) { - // Exceptions thrown from error handlers are sometimes not caught by the exception - // handler, so we invoke it directly (https://bugs.php.net/bug.php?id=54275) - $exceptionHandler = set_exception_handler('var_dump'); - restore_exception_handler(); + if (self::$stackedErrorLevels) { + self::$stackedErrors[] = func_get_args(); - if ($exceptionHandler) { - if (self::$stackedErrorLevels) { - self::$stackedErrors[] = func_get_args(); - - return true; - } - - $exception = sprintf('%s: %s in %s line %d', isset($this->levels[$level]) ? $this->levels[$level] : $level, $message, $file, $line); - $exception = new HandledErrorException($exception, 0, $level, $file, $line, $context); - $exception->handleWith($exceptionHandler); - - // we must stop the PHP script execution, as the exception has - // already been dealt with, so, let's throw an exception that - // will be caught by a dummy exception handler - set_exception_handler(function (\Exception $e) use ($exceptionHandler) { - if (!$e instanceof HandledErrorException && !$e instanceof DummyException) { - // happens if our handled exception is caught by a - // catch-all from user code, in which case, let the - // current handler handle this "new" exception - call_user_func($exceptionHandler, $e); - } - }); - - throw $exception; + return true; } + + $exception = sprintf('%s: %s in %s line %d', isset($this->levels[$level]) ? $this->levels[$level] : $level, $message, $file, $line); + $exception = new ContextErrorException($exception, 0, $level, $file, $line, $context); + + if (PHP_VERSION_ID <= 50407 && (PHP_VERSION_ID >= 50400 || PHP_VERSION_ID <= 50317)) { + // Exceptions thrown from error handlers are sometimes not caught by the exception + // handler and shutdown handlers are bypassed before 5.4.8/5.3.18. + // We temporarily re-enable display_errors to prevent any blank page related to this bug. + + $exception->errorHandlerCanary = new ErrorHandlerCanary(); + } + + throw $exception; } if (isset(self::$loggers['scream']) && !(error_reporting() & $level)) { @@ -323,23 +312,114 @@ class ErrorHandler private function handleFatalError($exceptionHandler, array $error) { + // Let PHP handle any further error + set_error_handler('var_dump', 0); + ini_set('display_errors', 1); + $level = isset($this->levels[$error['type']]) ? $this->levels[$error['type']] : $error['type']; $message = sprintf('%s: %s in %s line %d', $level, $error['message'], $error['file'], $error['line']); $exception = new FatalErrorException($message, 0, $error['type'], $error['file'], $error['line'], 3); foreach ($this->getFatalErrorHandlers() as $handler) { - if ($ex = $handler->handleError($error, $exception)) { - $exception = $ex; + if ($e = $handler->handleError($error, $exception)) { + $exception = $e; break; } } + // To be as fail-safe as possible, the FatalErrorException is first handled + // by the exception handler, then by the fatal error handler. The latter takes + // precedence and any output from the former is cancelled, if and only if + // nothing bad happens in this handling path. + + $caughtOutput = 0; + if ($exceptionHandler) { - $exception->handleWith($exceptionHandler); + $this->caughtOutput = false; + ob_start(array($this, 'catchOutput')); + try { + call_user_func($exceptionHandler, $exception); + } catch (\Exception $e) { + // Ignore this exception, we have to deal with the fatal error + } + if (false === $this->caughtOutput) { + ob_end_clean(); + } + if (isset($this->caughtOutput[0])) { + ob_start(array($this, 'cleanOutput')); + echo $this->caughtOutput; + $caughtOutput = ob_get_length(); + } + $this->caughtOutput = 0; } if (self::$fatalHandler) { - call_user_func(self::$fatalHandler, $exception); + try { + call_user_func(self::$fatalHandler, $exception); + + if ($caughtOutput) { + $this->caughtOutput = $caughtOutput; + } + } catch (\Exception $e) { + if (!$caughtOutput) { + // Neither the exception nor the fatal handler succeeded. + // Let PHP handle that now. + throw $exception; + } + } + } + } + + /** + * @internal + */ + public function catchOutput($buffer) + { + $this->caughtOutput = $buffer; + + return ''; + } + + /** + * @internal + */ + public function cleanOutput($buffer) + { + if ($this->caughtOutput) { + // use substr_replace() instead of substr() for mbstring overloading resistance + $cleanBuffer = substr_replace($buffer, '', 0, $this->caughtOutput); + if (isset($cleanBuffer[0])) { + $buffer = $cleanBuffer; + } + } + + return $buffer; + } +} + +/** + * Private class used to work around https://bugs.php.net/54275 + * + * @author Nicolas Grekas + * + * @internal + */ +class ErrorHandlerCanary +{ + private static $displayErrors = null; + + public function __construct() + { + if (null === self::$displayErrors) { + self::$displayErrors = ini_set('display_errors', 1); + } + } + + public function __destruct() + { + if (null !== self::$displayErrors) { + ini_set('display_errors', self::$displayErrors); + self::$displayErrors = null; } } } diff --git a/src/Symfony/Component/Debug/Exception/ContextErrorException.php b/src/Symfony/Component/Debug/Exception/ContextErrorException.php index d6a9eaf3da..54f0198f1b 100644 --- a/src/Symfony/Component/Debug/Exception/ContextErrorException.php +++ b/src/Symfony/Component/Debug/Exception/ContextErrorException.php @@ -15,9 +15,22 @@ namespace Symfony\Component\Debug\Exception; * Error Exception with Variable Context. * * @author Christian Sciberras - * - * @deprecated since version 2.5, to be removed in 3.0. */ -class ContextErrorException extends HandledErrorException +class ContextErrorException extends \ErrorException { + private $context = array(); + + public function __construct($message, $code, $severity, $filename, $lineno, $context = array()) + { + parent::__construct($message, $code, $severity, $filename, $lineno); + $this->context = $context; + } + + /** + * @return array Array of variables that existed when the exception occurred + */ + public function getContext() + { + return $this->context; + } } diff --git a/src/Symfony/Component/Debug/Exception/FatalErrorException.php b/src/Symfony/Component/Debug/Exception/FatalErrorException.php index 47375a3700..4e29495f30 100644 --- a/src/Symfony/Component/Debug/Exception/FatalErrorException.php +++ b/src/Symfony/Component/Debug/Exception/FatalErrorException.php @@ -18,7 +18,7 @@ namespace Symfony\Component\Debug\Exception; * @author Konstanton Myakshin * @author Nicolas Grekas */ -class FatalErrorException extends HandledErrorException +class FatalErrorException extends \ErrorException { public function __construct($message, $code, $severity, $filename, $lineno, $traceOffset = null) { diff --git a/src/Symfony/Component/Debug/Exception/HandledErrorException.php b/src/Symfony/Component/Debug/Exception/HandledErrorException.php deleted file mode 100644 index 83219d1196..0000000000 --- a/src/Symfony/Component/Debug/Exception/HandledErrorException.php +++ /dev/null @@ -1,86 +0,0 @@ - - * - * For the full copyright and license information, please view the LICENSE - * file that was distributed with this source code. - */ - -namespace Symfony\Component\Debug\Exception; - -/** - * @author Nicolas Grekas - */ -class HandledErrorException extends \ErrorException -{ - private $handlerOutput = false; - private $context = array(); - - public function __construct($message, $code, $severity, $filename, $lineno, $context = array()) - { - parent::__construct($message, $code, $severity, $filename, $lineno); - $this->context = $context; - } - - /** - * @return array Array of variables that existed when the exception occurred - */ - public function getContext() - { - return $this->context; - } - - public function handleWith($exceptionHandler) - { - $this->handlerOutput = false; - ob_start(array($this, 'catchOutput')); - call_user_func($exceptionHandler, $this); - if (false === $this->handlerOutput) { - ob_end_clean(); - } - ob_start(array(__CLASS__, 'flushOutput')); - echo $this->handlerOutput; - $this->handlerOutput = ob_get_length(); - } - - /** - * @internal - */ - public function catchOutput($buffer) - { - $this->handlerOutput = $buffer; - - return ''; - } - - /** - * @internal - */ - public static function flushOutput($buffer) - { - return $buffer; - } - - public function cleanOutput() - { - $status = ob_get_status(); - - if (isset($status['name']) && __CLASS__.'::flushOutput' === $status['name']) { - if ($this->handlerOutput) { - // use substr_replace() instead of substr() for mbstring overloading resistance - echo substr_replace(ob_get_clean(), '', 0, $this->handlerOutput); - } else { - ob_end_flush(); - } - } - } - - public function __destruct() - { - $this->handlerOutput = 0; - $this->cleanOutput(); - } -} diff --git a/src/Symfony/Component/Debug/Tests/DebugClassLoaderTest.php b/src/Symfony/Component/Debug/Tests/DebugClassLoaderTest.php index d50c7adc75..a002e22d0b 100644 --- a/src/Symfony/Component/Debug/Tests/DebugClassLoaderTest.php +++ b/src/Symfony/Component/Debug/Tests/DebugClassLoaderTest.php @@ -13,6 +13,7 @@ namespace Symfony\Component\Debug\Tests; use Symfony\Component\Debug\DebugClassLoader; use Symfony\Component\Debug\ErrorHandler; +use Symfony\Component\Debug\Exception\ContextErrorException; class DebugClassLoaderTest extends \PHPUnit_Framework_TestCase { @@ -77,52 +78,40 @@ class DebugClassLoaderTest extends \PHPUnit_Framework_TestCase $this->assertStringMatchesFormat('%aParse error%a', $output); } - /** - * @expectedException \Symfony\Component\Debug\Exception\HandledErrorException - */ public function testStacking() { - // the HandledErrorException must not be loaded to test the workaround + // the ContextErrorException must not be loaded to test the workaround // for https://bugs.php.net/65322. - if (class_exists('Symfony\Component\Debug\Exception\HandledErrorException', false)) { - $this->markTestSkipped('The HandledErrorException class is already loaded.'); + if (class_exists('Symfony\Component\Debug\Exception\ContextErrorException', false)) { + $this->markTestSkipped('The ContextErrorException class is already loaded.'); } - $exceptionHandler = $this->getMock('Symfony\Component\Debug\ExceptionHandler', array('handle')); - set_exception_handler(array($exceptionHandler, 'handle')); - - $that = $this; - $exceptionCheck = function ($exception) use ($that) { - $that->assertInstanceOf('Symfony\Component\Debug\Exception\HandledErrorException', $exception); - $that->assertEquals(E_STRICT, $exception->getSeverity()); - $that->assertStringStartsWith(__FILE__, $exception->getFile()); - $that->assertRegexp('/^Runtime Notice: Declaration/', $exception->getMessage()); - }; - - $exceptionHandler->expects($this->once()) - ->method('handle') - ->will($this->returnCallback($exceptionCheck)); ErrorHandler::register(); try { // Trigger autoloading + E_STRICT at compile time // which in turn triggers $errorHandler->handle() - // that again triggers autoloading for HandledErrorException. + // that again triggers autoloading for ContextErrorException. // Error stacking works around the bug above and everything is fine. eval(' namespace '.__NAMESPACE__.'; class ChildTestingStacking extends TestingStacking { function foo($bar) {} } '); + $this->fail('ContextErrorException expected'); + } catch (ContextErrorException $exception) { + // if an exception is thrown, the test passed + restore_error_handler(); + restore_exception_handler(); + $this->assertEquals(E_STRICT, $exception->getSeverity()); + $this->assertStringStartsWith(__FILE__, $exception->getFile()); + $this->assertRegexp('/^Runtime Notice: Declaration/', $exception->getMessage()); } catch (\Exception $e) { restore_error_handler(); restore_exception_handler(); throw $e; } - - restore_error_handler(); - restore_exception_handler(); } /** diff --git a/src/Symfony/Component/Debug/Tests/ErrorHandlerTest.php b/src/Symfony/Component/Debug/Tests/ErrorHandlerTest.php index 7786c14ef1..1bb16712e8 100644 --- a/src/Symfony/Component/Debug/Tests/ErrorHandlerTest.php +++ b/src/Symfony/Component/Debug/Tests/ErrorHandlerTest.php @@ -12,7 +12,7 @@ namespace Symfony\Component\Debug\Tests; use Symfony\Component\Debug\ErrorHandler; -use Symfony\Component\Debug\Exception\HandledErrorException; +use Symfony\Component\Debug\Exception\ContextErrorException; /** * ErrorHandlerTest @@ -46,42 +46,33 @@ class ErrorHandlerTest extends \PHPUnit_Framework_TestCase public function testNotice() { - $exceptionHandler = $this->getMock('Symfony\Component\Debug\ExceptionHandler', array('handle')); - set_exception_handler(array($exceptionHandler, 'handle')); - - $that = $this; - $exceptionCheck = function ($exception) use ($that) { - $that->assertInstanceOf('Symfony\Component\Debug\Exception\HandledErrorException', $exception); - $that->assertEquals(E_NOTICE, $exception->getSeverity()); - $that->assertEquals(__FILE__, $exception->getFile()); - $that->assertRegexp('/^Notice: Undefined variable: (foo|bar)/', $exception->getMessage()); - $that->assertArrayHasKey('foobar', $exception->getContext()); - - $trace = $exception->getTrace(); - $that->assertEquals(__FILE__, $trace[0]['file']); - $that->assertEquals('Symfony\Component\Debug\ErrorHandler', $trace[0]['class']); - $that->assertEquals('handle', $trace[0]['function']); - $that->assertEquals('->', $trace[0]['type']); - - $that->assertEquals(__FILE__, $trace[1]['file']); - $that->assertEquals(__CLASS__, $trace[1]['class']); - $that->assertEquals('triggerNotice', $trace[1]['function']); - $that->assertEquals('::', $trace[1]['type']); - - $that->assertEquals(__CLASS__, $trace[2]['class']); - $that->assertEquals('testNotice', $trace[2]['function']); - $that->assertEquals('->', $trace[2]['type']); - }; - - $exceptionHandler->expects($this->once()) - ->method('handle') - ->will($this->returnCallback($exceptionCheck)); ErrorHandler::register(); try { self::triggerNotice($this); - } catch (HandledErrorException $e) { + $this->fail('ContextErrorException expected'); + } catch (ContextErrorException $exception) { // if an exception is thrown, the test passed + restore_error_handler(); + $this->assertEquals(E_NOTICE, $exception->getSeverity()); + $this->assertEquals(__FILE__, $exception->getFile()); + $this->assertRegexp('/^Notice: Undefined variable: (foo|bar)/', $exception->getMessage()); + $this->assertArrayHasKey('foobar', $exception->getContext()); + + $trace = $exception->getTrace(); + $this->assertEquals(__FILE__, $trace[0]['file']); + $this->assertEquals('Symfony\Component\Debug\ErrorHandler', $trace[0]['class']); + $this->assertEquals('handle', $trace[0]['function']); + $this->assertEquals('->', $trace[0]['type']); + + $this->assertEquals(__FILE__, $trace[1]['file']); + $this->assertEquals(__CLASS__, $trace[1]['class']); + $this->assertEquals('triggerNotice', $trace[1]['function']); + $this->assertEquals('::', $trace[1]['type']); + + $this->assertEquals(__CLASS__, $trace[2]['class']); + $this->assertEquals('testNotice', $trace[2]['function']); + $this->assertEquals('->', $trace[2]['type']); } catch (\Exception $e) { restore_error_handler(); diff --git a/src/Symfony/Component/HttpKernel/EventListener/ErrorsLoggerListener.php b/src/Symfony/Component/HttpKernel/EventListener/ErrorsLoggerListener.php index ec6b7f9328..025b6475a4 100644 --- a/src/Symfony/Component/HttpKernel/EventListener/ErrorsLoggerListener.php +++ b/src/Symfony/Component/HttpKernel/EventListener/ErrorsLoggerListener.php @@ -38,11 +38,12 @@ class ErrorsLoggerListener implements EventSubscriberInterface { if (null !== $this->logger) { ErrorHandler::setLogger($this->logger, $this->channel); + $this->logger = null; } } public static function getSubscribedEvents() { - return array(KernelEvents::REQUEST => 'injectLogger'); + return array(KernelEvents::REQUEST => array('injectLogger', 2048)); } } diff --git a/src/Symfony/Component/HttpKernel/EventListener/FatalErrorExceptionsListener.php b/src/Symfony/Component/HttpKernel/EventListener/FatalErrorExceptionsListener.php index 39ccfaa0cc..0677682810 100644 --- a/src/Symfony/Component/HttpKernel/EventListener/FatalErrorExceptionsListener.php +++ b/src/Symfony/Component/HttpKernel/EventListener/FatalErrorExceptionsListener.php @@ -35,11 +35,13 @@ class FatalErrorExceptionsListener implements EventSubscriberInterface { if ($this->handler) { ErrorHandler::setFatalErrorExceptionHandler($this->handler); + $this->handler = null; } } public static function getSubscribedEvents() { - return array(KernelEvents::REQUEST => 'injectHandler'); + // Don't register early as e.g. the Router is generally required by the handler + return array(KernelEvents::REQUEST => array('injectHandler', 8)); } } diff --git a/src/Symfony/Component/HttpKernel/Fragment/InlineFragmentRenderer.php b/src/Symfony/Component/HttpKernel/Fragment/InlineFragmentRenderer.php index 33866e94ea..a6ab82ea28 100644 --- a/src/Symfony/Component/HttpKernel/Fragment/InlineFragmentRenderer.php +++ b/src/Symfony/Component/HttpKernel/Fragment/InlineFragmentRenderer.php @@ -18,7 +18,6 @@ use Symfony\Component\HttpKernel\Controller\ControllerReference; use Symfony\Component\HttpKernel\KernelEvents; use Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent; use Symfony\Component\EventDispatcher\EventDispatcherInterface; -use Symfony\Component\Debug\Exception\HandledErrorException; /** * Implements the inline rendering strategy where the Request is rendered by the current HTTP kernel. @@ -87,15 +86,10 @@ class InlineFragmentRenderer extends RoutableFragmentRenderer } catch (\Exception $e) { // we dispatch the exception event to trigger the logging // the response that comes back is simply ignored - if (isset($options['ignore_errors']) && $options['ignore_errors']) { - if ($e instanceof HandledErrorException) { - $e->cleanOutput(); - } - if ($this->dispatcher) { - $event = new GetResponseForExceptionEvent($this->kernel, $request, HttpKernelInterface::SUB_REQUEST, $e); + if (isset($options['ignore_errors']) && $options['ignore_errors'] && $this->dispatcher) { + $event = new GetResponseForExceptionEvent($this->kernel, $request, HttpKernelInterface::SUB_REQUEST, $e); - $this->dispatcher->dispatch(KernelEvents::EXCEPTION, $event); - } + $this->dispatcher->dispatch(KernelEvents::EXCEPTION, $event); } // let's clean up the output buffers that were created by the sub-request diff --git a/src/Symfony/Component/HttpKernel/HttpKernel.php b/src/Symfony/Component/HttpKernel/HttpKernel.php index 46e8a1fe12..c3556972e9 100644 --- a/src/Symfony/Component/HttpKernel/HttpKernel.php +++ b/src/Symfony/Component/HttpKernel/HttpKernel.php @@ -25,7 +25,6 @@ use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\RequestStack; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\EventDispatcher\EventDispatcherInterface; -use Symfony\Component\Debug\Exception\HandledErrorException; use Symfony\Component\Debug\Exception\FatalErrorException; /** @@ -72,9 +71,6 @@ class HttpKernel implements HttpKernelInterface, TerminableInterface throw $e; } - if ($e instanceof HandledErrorException) { - $e->cleanOutput(); - } return $this->handleException($e, $request, $type); } @@ -102,7 +98,6 @@ class HttpKernel implements HttpKernelInterface, TerminableInterface $response->sendContent(); $this->terminate($request, $response); - $exception->cleanOutput(); } /**