From 01c51e28419bcaf617791d896eee9cb06e5bedd5 Mon Sep 17 00:00:00 2001 From: Michael Tibben Date: Mon, 18 Dec 2017 09:19:39 +1100 Subject: [PATCH 01/13] Allow the log level to be overriden based on the http status code --- .../EventListener/ExceptionListener.php | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php b/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php index 45e2fdaadf57d..a137e4360705e 100644 --- a/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php +++ b/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php @@ -12,6 +12,7 @@ namespace Symfony\Component\HttpKernel\EventListener; use Psr\Log\LoggerInterface; +use Psr\Log\LogLevel; use Symfony\Component\Debug\Exception\FlattenException; use Symfony\Component\EventDispatcher\EventDispatcherInterface; use Symfony\Component\HttpFoundation\Request; @@ -32,6 +33,7 @@ class ExceptionListener implements EventSubscriberInterface { protected $controller; protected $logger; + public static $httpStatusCodeLogLevel = []; public function __construct($controller, LoggerInterface $logger = null) { @@ -103,11 +105,19 @@ public static function getSubscribedEvents() protected function logException(\Exception $exception, $message) { if (null !== $this->logger) { - if (!$exception instanceof HttpExceptionInterface || $exception->getStatusCode() >= 500) { - $this->logger->critical($message, array('exception' => $exception)); - } else { - $this->logger->error($message, array('exception' => $exception)); + $logLevel = LogLevel::ERROR; + if ($exception instanceof HttpExceptionInterface) { + $statusCode = $exception->getStatusCode(); + if (isset(static::$httpStatusCodeLogLevel[$statusCode])) { + $logLevel = static::$httpStatusCodeLogLevel[$statusCode]; + } else if ($statusCode >= 500) { + $logLevel = LogLevel::CRITICAL; + } else if ($statusCode >= 400) { + $logLevel = LogLevel::WARNING; + } } + + $this->logger->log($logLevel, $message, array('exception' => $exception)); } } From 8a7772848098d8a38dddfa6dcc7929dde62d4db4 Mon Sep 17 00:00:00 2001 From: Michael Tibben Date: Tue, 19 Dec 2017 10:44:41 +1100 Subject: [PATCH 02/13] Make httpStatusCodeLogLevel a class property --- .../HttpKernel/EventListener/ExceptionListener.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php b/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php index a137e4360705e..2f7f285b25d19 100644 --- a/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php +++ b/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php @@ -33,7 +33,7 @@ class ExceptionListener implements EventSubscriberInterface { protected $controller; protected $logger; - public static $httpStatusCodeLogLevel = []; + protected $httpStatusCodeLogLevel = []; public function __construct($controller, LoggerInterface $logger = null) { @@ -108,8 +108,8 @@ protected function logException(\Exception $exception, $message) $logLevel = LogLevel::ERROR; if ($exception instanceof HttpExceptionInterface) { $statusCode = $exception->getStatusCode(); - if (isset(static::$httpStatusCodeLogLevel[$statusCode])) { - $logLevel = static::$httpStatusCodeLogLevel[$statusCode]; + if (isset($this->httpStatusCodeLogLevel[$statusCode])) { + $logLevel = $this->httpStatusCodeLogLevel[$statusCode]; } else if ($statusCode >= 500) { $logLevel = LogLevel::CRITICAL; } else if ($statusCode >= 400) { From 5ef1878d5a0cda42a93bc69abcf8b8d0923b4c4c Mon Sep 17 00:00:00 2001 From: Michael Tibben Date: Tue, 19 Dec 2017 10:48:37 +1100 Subject: [PATCH 03/13] Separate log level logic to its own method --- .../EventListener/ExceptionListener.php | 29 ++++++++++--------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php b/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php index 2f7f285b25d19..9985131091a9b 100644 --- a/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php +++ b/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php @@ -96,6 +96,21 @@ public static function getSubscribedEvents() ); } + protected function getExceptionLogLevel(\Exception $exception) + { + $logLevel = LogLevel::CRITICAL; + if ($exception instanceof HttpExceptionInterface) { + $statusCode = $exception->getStatusCode(); + if (isset($this->httpStatusCodeLogLevel[$statusCode])) { + $logLevel = $this->httpStatusCodeLogLevel[$statusCode]; + } else if ($statusCode < 500) { + $logLevel = LogLevel::ERROR; + } + } + + return $logLevel; + } + /** * Logs an exception. * @@ -105,19 +120,7 @@ public static function getSubscribedEvents() protected function logException(\Exception $exception, $message) { if (null !== $this->logger) { - $logLevel = LogLevel::ERROR; - if ($exception instanceof HttpExceptionInterface) { - $statusCode = $exception->getStatusCode(); - if (isset($this->httpStatusCodeLogLevel[$statusCode])) { - $logLevel = $this->httpStatusCodeLogLevel[$statusCode]; - } else if ($statusCode >= 500) { - $logLevel = LogLevel::CRITICAL; - } else if ($statusCode >= 400) { - $logLevel = LogLevel::WARNING; - } - } - - $this->logger->log($logLevel, $message, array('exception' => $exception)); + $this->logger->log($this->getExceptionLogLevel($exception), $message, array('exception' => $exception)); } } From 06bb76aa5b329b444f28b8043788854bb6782f2e Mon Sep 17 00:00:00 2001 From: Michael Tibben Date: Tue, 19 Dec 2017 10:56:29 +1100 Subject: [PATCH 04/13] Fix code style --- .../Component/HttpKernel/EventListener/ExceptionListener.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php b/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php index 9985131091a9b..84d04fddae9ca 100644 --- a/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php +++ b/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php @@ -33,7 +33,7 @@ class ExceptionListener implements EventSubscriberInterface { protected $controller; protected $logger; - protected $httpStatusCodeLogLevel = []; + protected $httpStatusCodeLogLevel = array(); public function __construct($controller, LoggerInterface $logger = null) { @@ -103,7 +103,7 @@ protected function getExceptionLogLevel(\Exception $exception) $statusCode = $exception->getStatusCode(); if (isset($this->httpStatusCodeLogLevel[$statusCode])) { $logLevel = $this->httpStatusCodeLogLevel[$statusCode]; - } else if ($statusCode < 500) { + } elseif ($statusCode < 500) { $logLevel = LogLevel::ERROR; } } From 367e68e640128c5d76d14ee2f436e46d9b44684d Mon Sep 17 00:00:00 2001 From: Michael Tibben Date: Tue, 19 Dec 2017 20:05:30 +1100 Subject: [PATCH 05/13] Add param for setting log levels in DI --- src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml | 1 + .../Component/HttpKernel/EventListener/ExceptionListener.php | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml b/src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml index 50e439b39ec64..3e14e6c263617 100644 --- a/src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml +++ b/src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml @@ -128,6 +128,7 @@ %twig.exception_listener.controller% + %twig.exception_listener.http_status_codes_log_level% diff --git a/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php b/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php index 84d04fddae9ca..e3dbfbdab8108 100644 --- a/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php +++ b/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php @@ -35,10 +35,13 @@ class ExceptionListener implements EventSubscriberInterface protected $logger; protected $httpStatusCodeLogLevel = array(); - public function __construct($controller, LoggerInterface $logger = null) + public function __construct($controller, LoggerInterface $logger = null, $httpStatusCodeLogLevel = array()) { $this->controller = $controller; $this->logger = $logger; + if ($httpStatusCodeLogLevel && is_array($httpStatusCodeLogLevel)) { + $this->httpStatusCodeLogLevel = $httpStatusCodeLogLevel; + } } public function logKernelException(GetResponseForExceptionEvent $event) From 2288c5b27f5a35b8c41a045f88d27c20598de2b7 Mon Sep 17 00:00:00 2001 From: Michael Tibben Date: Wed, 27 Dec 2017 15:22:08 +1100 Subject: [PATCH 06/13] Fixes from PR feedback --- .../Bundle/TwigBundle/Resources/config/twig.xml | 2 +- .../HttpKernel/EventListener/ExceptionListener.php | 10 ++++------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml b/src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml index 3e14e6c263617..f76c0e69516eb 100644 --- a/src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml +++ b/src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml @@ -128,7 +128,7 @@ %twig.exception_listener.controller% - %twig.exception_listener.http_status_codes_log_level% + %framework.http_exception.log_levels% diff --git a/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php b/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php index e3dbfbdab8108..dfc83bdf6f36d 100644 --- a/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php +++ b/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php @@ -33,15 +33,13 @@ class ExceptionListener implements EventSubscriberInterface { protected $controller; protected $logger; - protected $httpStatusCodeLogLevel = array(); + protected $httpStatusCodeLogLevel; - public function __construct($controller, LoggerInterface $logger = null, $httpStatusCodeLogLevel = array()) + public function __construct($controller, LoggerInterface $logger = null, array $httpStatusCodeLogLevel = array()) { $this->controller = $controller; $this->logger = $logger; - if ($httpStatusCodeLogLevel && is_array($httpStatusCodeLogLevel)) { - $this->httpStatusCodeLogLevel = $httpStatusCodeLogLevel; - } + $this->httpStatusCodeLogLevel = $httpStatusCodeLogLevel; } public function logKernelException(GetResponseForExceptionEvent $event) @@ -99,7 +97,7 @@ public static function getSubscribedEvents() ); } - protected function getExceptionLogLevel(\Exception $exception) + protected function getExceptionLogLevel(\Exception $exception): string { $logLevel = LogLevel::CRITICAL; if ($exception instanceof HttpExceptionInterface) { From c1d0dea35f9f2085a941de014f4f67baef2c66d0 Mon Sep 17 00:00:00 2001 From: Michael Tibben Date: Wed, 27 Dec 2017 15:25:46 +1100 Subject: [PATCH 07/13] Log http 4xx as warning --- .../Component/HttpKernel/EventListener/ExceptionListener.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php b/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php index dfc83bdf6f36d..dac036504e841 100644 --- a/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php +++ b/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php @@ -104,8 +104,8 @@ protected function getExceptionLogLevel(\Exception $exception): string $statusCode = $exception->getStatusCode(); if (isset($this->httpStatusCodeLogLevel[$statusCode])) { $logLevel = $this->httpStatusCodeLogLevel[$statusCode]; - } elseif ($statusCode < 500) { - $logLevel = LogLevel::ERROR; + } elseif (400 <= $statusCode && $statusCode < 500) { + $logLevel = LogLevel::WARNING; } } From dc246e0b829e4a013f070a8244280aa425062c8c Mon Sep 17 00:00:00 2001 From: Michael Tibben Date: Wed, 27 Dec 2017 16:11:51 +1100 Subject: [PATCH 08/13] Add tests for log level overrides --- .../EventListener/ExceptionListenerTest.php | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/src/Symfony/Component/HttpKernel/Tests/EventListener/ExceptionListenerTest.php b/src/Symfony/Component/HttpKernel/Tests/EventListener/ExceptionListenerTest.php index f745e5b3e1116..778efc5bb3c16 100644 --- a/src/Symfony/Component/HttpKernel/Tests/EventListener/ExceptionListenerTest.php +++ b/src/Symfony/Component/HttpKernel/Tests/EventListener/ExceptionListenerTest.php @@ -22,6 +22,7 @@ use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Tests\Logger; +use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; /** * ExceptionListenerTest. @@ -155,6 +156,44 @@ public function testCSPHeaderIsRemoved() $this->assertFalse($response->headers->has('content-security-policy'), 'CSP header has been removed'); $this->assertFalse($dispatcher->hasListeners(KernelEvents::RESPONSE), 'CSP removal listener has been removed'); } + + public function testHttp4xxLogLevel() + { + $logger = new TestLogger(); + $l = new ExceptionListener('foo', $logger); + $exception = new \Exception('foo'); + $event = new GetResponseForExceptionEvent(new TestKernelThatThrowsHttp4xxException(), Request::create('/'), HttpKernelInterface::MASTER_REQUEST, $exception); + try { + $l->logKernelException($event); + $l->onKernelException($event); + $this->fail('NotFoundHttpException expected'); + } catch (NotFoundHttpException $e) { + $this->assertSame('4xx', $e->getMessage()); + $this->assertSame('foo', $e->getPrevious()->getMessage()); + } + + $this->assertEquals(1, $logger->countErrors()); + $this->assertCount(1, $logger->getLogs('warning')); + } + + public function testHttpLogLevelOverride() + { + $logger = new TestLogger(); + $l = new ExceptionListener('foo', $logger, array(404 => 'notice')); + $exception = new \Exception('foo'); + $event = new GetResponseForExceptionEvent(new TestKernelThatThrowsHttp4xxException(), Request::create('/'), HttpKernelInterface::MASTER_REQUEST, $exception); + try { + $l->logKernelException($event); + $l->onKernelException($event); + $this->fail('NotFoundHttpException expected'); + } catch (NotFoundHttpException $e) { + $this->assertSame('4xx', $e->getMessage()); + $this->assertSame('foo', $e->getPrevious()->getMessage()); + } + + $this->assertEquals(1, $logger->countErrors()); + $this->assertCount(1, $logger->getLogs('notice')); + } } class TestLogger extends Logger implements DebugLoggerInterface @@ -180,3 +219,11 @@ public function handle(Request $request, $type = self::MASTER_REQUEST, $catch = throw new \RuntimeException('bar'); } } + +class TestKernelThatThrowsHttp4xxException implements HttpKernelInterface +{ + public function handle(Request $request, $type = self::MASTER_REQUEST, $catch = true) + { + throw new NotFoundHttpException('4xx'); + } +} From 25fd9e42570895fa6d54b9486013baf776dbbd75 Mon Sep 17 00:00:00 2001 From: Michael Tibben Date: Wed, 27 Dec 2017 18:29:12 +1100 Subject: [PATCH 09/13] Add to twig bundle configuration --- .../DependencyInjection/Configuration.php | 41 +++++++++++++++++++ .../DependencyInjection/TwigExtension.php | 1 + .../TwigBundle/Resources/config/twig.xml | 2 +- 3 files changed, 43 insertions(+), 1 deletion(-) diff --git a/src/Symfony/Bundle/TwigBundle/DependencyInjection/Configuration.php b/src/Symfony/Bundle/TwigBundle/DependencyInjection/Configuration.php index f4488c0d5ccd4..d3e7bef00a424 100644 --- a/src/Symfony/Bundle/TwigBundle/DependencyInjection/Configuration.php +++ b/src/Symfony/Bundle/TwigBundle/DependencyInjection/Configuration.php @@ -14,6 +14,7 @@ use Symfony\Component\Config\Definition\Builder\ArrayNodeDefinition; use Symfony\Component\Config\Definition\Builder\TreeBuilder; use Symfony\Component\Config\Definition\ConfigurationInterface; +use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException; /** * TwigExtension configuration structure. @@ -42,6 +43,7 @@ public function getConfigTreeBuilder() $this->addGlobalsSection($rootNode); $this->addTwigOptions($rootNode); $this->addTwigFormatOptions($rootNode); + $this->addHttpExceptionLogLevels($rootNode); return $treeBuilder; } @@ -194,4 +196,43 @@ private function addTwigFormatOptions(ArrayNodeDefinition $rootNode) ->end() ; } + + private function addHttpExceptionLogLevels(ArrayNodeDefinition $rootNode) + { + $rootNode + ->children() + ->arrayNode('http_exception_log_levels') + ->info('The override log levels for http exceptions') + ->example(array('403' => 'NOTICE', '404' => 'INFO')) + ->useAttributeAsKey('http_exception_log_levels') + ->prototype('variable')->end() + ->validate() + ->always(function ($v) { + $map = array(); + foreach ($v as $status => $level) { + if (!(is_int($status) && 100 <= $status && $status <= 599)) { + throw new InvalidConfigurationException(sprintf( + 'The configured status code "%s" in twig.http_exception_log_levels is not a valid http status code.', + $status + )); + } + + $levelConstant = 'Psr\Log\LogLevel::'.$level; + if (!defined($levelConstant)) { + throw new InvalidConfigurationException(sprintf( + 'The configured log level "%s" in twig.http_exception_log_levels is invalid as it is not defined in Psr\\Log\\LogLevel.', + $level + )); + } + + $map[$status] = constant($levelConstant); + } + + return $map; + }) + ->end() + ->end() + ->end() + ; + } } diff --git a/src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php b/src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php index b44dd2481c631..dec453852cbf7 100644 --- a/src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php +++ b/src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php @@ -78,6 +78,7 @@ public function load(array $configs, ContainerBuilder $container) $config = $this->processConfiguration($configuration, $configs); $container->setParameter('twig.exception_listener.controller', $config['exception_controller']); + $container->setParameter('twig.exception_listener.http_log_levels', $config['http_exception_log_levels']); $container->setParameter('twig.form.resources', $config['form_themes']); $container->setParameter('twig.default_path', $config['default_path']); diff --git a/src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml b/src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml index f76c0e69516eb..65cb61afe0496 100644 --- a/src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml +++ b/src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml @@ -128,7 +128,7 @@ %twig.exception_listener.controller% - %framework.http_exception.log_levels% + %twig.exception_listener.http_log_levels% From d0948d72d21e0208072e39275deca2de39d179a5 Mon Sep 17 00:00:00 2001 From: Michael Tibben Date: Wed, 27 Dec 2017 22:52:42 +1100 Subject: [PATCH 10/13] De-yoda conditionals --- .../Bundle/TwigBundle/DependencyInjection/Configuration.php | 2 +- .../Component/HttpKernel/EventListener/ExceptionListener.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Symfony/Bundle/TwigBundle/DependencyInjection/Configuration.php b/src/Symfony/Bundle/TwigBundle/DependencyInjection/Configuration.php index d3e7bef00a424..a17f26e80e383 100644 --- a/src/Symfony/Bundle/TwigBundle/DependencyInjection/Configuration.php +++ b/src/Symfony/Bundle/TwigBundle/DependencyInjection/Configuration.php @@ -210,7 +210,7 @@ private function addHttpExceptionLogLevels(ArrayNodeDefinition $rootNode) ->always(function ($v) { $map = array(); foreach ($v as $status => $level) { - if (!(is_int($status) && 100 <= $status && $status <= 599)) { + if (!(is_int($status) && $status >= 100 && $status <= 599)) { throw new InvalidConfigurationException(sprintf( 'The configured status code "%s" in twig.http_exception_log_levels is not a valid http status code.', $status diff --git a/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php b/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php index dac036504e841..ae82339a4d91c 100644 --- a/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php +++ b/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php @@ -104,7 +104,7 @@ protected function getExceptionLogLevel(\Exception $exception): string $statusCode = $exception->getStatusCode(); if (isset($this->httpStatusCodeLogLevel[$statusCode])) { $logLevel = $this->httpStatusCodeLogLevel[$statusCode]; - } elseif (400 <= $statusCode && $statusCode < 500) { + } elseif ($statusCode >= 400 && $statusCode < 500) { $logLevel = LogLevel::WARNING; } } From 15b15a5abe1045ad2cc1407bce9f288e28061228 Mon Sep 17 00:00:00 2001 From: Michael Tibben Date: Fri, 5 Jan 2018 15:01:47 +1100 Subject: [PATCH 11/13] Split into two listeners --- .../DependencyInjection/Configuration.php | 40 +++++++ .../FrameworkExtension.php | 2 + .../Resources/config/services.xml | 8 ++ .../DependencyInjection/Configuration.php | 41 ------- .../DependencyInjection/TwigExtension.php | 1 - .../TwigBundle/Resources/config/twig.xml | 3 +- .../EventListener/ExceptionListener.php | 30 ++--- .../LoggingExceptionListener.php | 65 +++++++++++ .../RenderControllerExceptionListener.php | 106 ++++++++++++++++++ 9 files changed, 232 insertions(+), 64 deletions(-) create mode 100644 src/Symfony/Component/HttpKernel/EventListener/LoggingExceptionListener.php create mode 100644 src/Symfony/Component/HttpKernel/EventListener/RenderControllerExceptionListener.php diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php index 53abbb23fa316..3fbcf9c598140 100644 --- a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php +++ b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php @@ -101,6 +101,7 @@ public function getConfigTreeBuilder() $this->addPhpErrorsSection($rootNode); $this->addWebLinkSection($rootNode); $this->addLockSection($rootNode); + $this->addHttpExceptionLogLevels($rootNode); return $treeBuilder; } @@ -902,4 +903,43 @@ private function addWebLinkSection(ArrayNodeDefinition $rootNode) ->end() ; } + + private function addHttpExceptionLogLevels(ArrayNodeDefinition $rootNode) + { + $rootNode + ->children() + ->arrayNode('http_exception_log_levels') + ->info('The override log levels for http exceptions') + ->example(array('403' => 'NOTICE', '404' => 'INFO')) + ->useAttributeAsKey('http_exception_log_levels') + ->prototype('variable')->end() + ->validate() + ->always(function ($v) { + $map = array(); + foreach ($v as $status => $level) { + if (!(is_int($status) && $status >= 100 && $status <= 599)) { + throw new InvalidConfigurationException(sprintf( + 'The configured status code "%s" in framework.http_exception_log_levels is not a valid http status code.', + $status + )); + } + + $levelConstant = 'Psr\Log\LogLevel::'.$level; + if (!defined($levelConstant)) { + throw new InvalidConfigurationException(sprintf( + 'The configured log level "%s" in framework.http_exception_log_levels is invalid as it is not defined in Psr\\Log\\LogLevel.', + $level + )); + } + + $map[$status] = constant($levelConstant); + } + + return $map; + }) + ->end() + ->end() + ->end() + ; + } } diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php index be81b91003c4d..ae45ac74c79cc 100644 --- a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php +++ b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php @@ -165,6 +165,8 @@ public function load(array $configs, ContainerBuilder $container) $container->setParameter('kernel.trusted_hosts', $config['trusted_hosts']); $container->setParameter('kernel.default_locale', $config['default_locale']); + $container->setParameter('framework.exception_listener.http_log_levels', $config['http_exception_log_levels']); + if (!$container->hasParameter('debug.file_link_format')) { if (!$container->hasParameter('templating.helper.code.file_link_format')) { $links = array( diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/services.xml b/src/Symfony/Bundle/FrameworkBundle/Resources/config/services.xml index 2c0072d85d9a1..4d706be51578d 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Resources/config/services.xml +++ b/src/Symfony/Bundle/FrameworkBundle/Resources/config/services.xml @@ -71,5 +71,13 @@ + + + + + + %framework.exception_listener.http_log_levels% + + diff --git a/src/Symfony/Bundle/TwigBundle/DependencyInjection/Configuration.php b/src/Symfony/Bundle/TwigBundle/DependencyInjection/Configuration.php index a17f26e80e383..f4488c0d5ccd4 100644 --- a/src/Symfony/Bundle/TwigBundle/DependencyInjection/Configuration.php +++ b/src/Symfony/Bundle/TwigBundle/DependencyInjection/Configuration.php @@ -14,7 +14,6 @@ use Symfony\Component\Config\Definition\Builder\ArrayNodeDefinition; use Symfony\Component\Config\Definition\Builder\TreeBuilder; use Symfony\Component\Config\Definition\ConfigurationInterface; -use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException; /** * TwigExtension configuration structure. @@ -43,7 +42,6 @@ public function getConfigTreeBuilder() $this->addGlobalsSection($rootNode); $this->addTwigOptions($rootNode); $this->addTwigFormatOptions($rootNode); - $this->addHttpExceptionLogLevels($rootNode); return $treeBuilder; } @@ -196,43 +194,4 @@ private function addTwigFormatOptions(ArrayNodeDefinition $rootNode) ->end() ; } - - private function addHttpExceptionLogLevels(ArrayNodeDefinition $rootNode) - { - $rootNode - ->children() - ->arrayNode('http_exception_log_levels') - ->info('The override log levels for http exceptions') - ->example(array('403' => 'NOTICE', '404' => 'INFO')) - ->useAttributeAsKey('http_exception_log_levels') - ->prototype('variable')->end() - ->validate() - ->always(function ($v) { - $map = array(); - foreach ($v as $status => $level) { - if (!(is_int($status) && $status >= 100 && $status <= 599)) { - throw new InvalidConfigurationException(sprintf( - 'The configured status code "%s" in twig.http_exception_log_levels is not a valid http status code.', - $status - )); - } - - $levelConstant = 'Psr\Log\LogLevel::'.$level; - if (!defined($levelConstant)) { - throw new InvalidConfigurationException(sprintf( - 'The configured log level "%s" in twig.http_exception_log_levels is invalid as it is not defined in Psr\\Log\\LogLevel.', - $level - )); - } - - $map[$status] = constant($levelConstant); - } - - return $map; - }) - ->end() - ->end() - ->end() - ; - } } diff --git a/src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php b/src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php index dec453852cbf7..b44dd2481c631 100644 --- a/src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php +++ b/src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php @@ -78,7 +78,6 @@ public function load(array $configs, ContainerBuilder $container) $config = $this->processConfiguration($configuration, $configs); $container->setParameter('twig.exception_listener.controller', $config['exception_controller']); - $container->setParameter('twig.exception_listener.http_log_levels', $config['http_exception_log_levels']); $container->setParameter('twig.form.resources', $config['form_themes']); $container->setParameter('twig.default_path', $config['default_path']); diff --git a/src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml b/src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml index 65cb61afe0496..db9eaf26852f3 100644 --- a/src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml +++ b/src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml @@ -123,12 +123,11 @@ - + %twig.exception_listener.controller% - %twig.exception_listener.http_log_levels% diff --git a/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php b/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php index ae82339a4d91c..0c1a38cff8944 100644 --- a/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php +++ b/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php @@ -12,7 +12,6 @@ namespace Symfony\Component\HttpKernel\EventListener; use Psr\Log\LoggerInterface; -use Psr\Log\LogLevel; use Symfony\Component\Debug\Exception\FlattenException; use Symfony\Component\EventDispatcher\EventDispatcherInterface; use Symfony\Component\HttpFoundation\Request; @@ -24,22 +23,24 @@ use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface; use Symfony\Component\EventDispatcher\EventSubscriberInterface; +@trigger_error(sprintf('The "%s" class is deprecated since Symfony 4.1 and will be removed in 5.0, use "%s" or "%s" instead.', ExceptionListener::class, RenderControllerExceptionListener::class, LoggingExceptionListener::class), E_USER_DEPRECATED); + /** * ExceptionListener. * * @author Fabien Potencier + * + * @deprecated since Symfony 4.1, use RenderControllerExceptionListener or LoggingExceptionListener instead */ class ExceptionListener implements EventSubscriberInterface { protected $controller; protected $logger; - protected $httpStatusCodeLogLevel; - public function __construct($controller, LoggerInterface $logger = null, array $httpStatusCodeLogLevel = array()) + public function __construct($controller, LoggerInterface $logger = null) { $this->controller = $controller; $this->logger = $logger; - $this->httpStatusCodeLogLevel = $httpStatusCodeLogLevel; } public function logKernelException(GetResponseForExceptionEvent $event) @@ -97,21 +98,6 @@ public static function getSubscribedEvents() ); } - protected function getExceptionLogLevel(\Exception $exception): string - { - $logLevel = LogLevel::CRITICAL; - if ($exception instanceof HttpExceptionInterface) { - $statusCode = $exception->getStatusCode(); - if (isset($this->httpStatusCodeLogLevel[$statusCode])) { - $logLevel = $this->httpStatusCodeLogLevel[$statusCode]; - } elseif ($statusCode >= 400 && $statusCode < 500) { - $logLevel = LogLevel::WARNING; - } - } - - return $logLevel; - } - /** * Logs an exception. * @@ -121,7 +107,11 @@ protected function getExceptionLogLevel(\Exception $exception): string protected function logException(\Exception $exception, $message) { if (null !== $this->logger) { - $this->logger->log($this->getExceptionLogLevel($exception), $message, array('exception' => $exception)); + if (!$exception instanceof HttpExceptionInterface || $exception->getStatusCode() >= 500) { + $this->logger->critical($message, array('exception' => $exception)); + } else { + $this->logger->error($message, array('exception' => $exception)); + } } } diff --git a/src/Symfony/Component/HttpKernel/EventListener/LoggingExceptionListener.php b/src/Symfony/Component/HttpKernel/EventListener/LoggingExceptionListener.php new file mode 100644 index 0000000000000..47d09799c628f --- /dev/null +++ b/src/Symfony/Component/HttpKernel/EventListener/LoggingExceptionListener.php @@ -0,0 +1,65 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\HttpKernel\EventListener; + +use Psr\Log\LoggerInterface; +use Psr\Log\LogLevel; +use Psr\Log\NullLogger; +use Symfony\Component\EventDispatcher\EventSubscriberInterface; +use Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent; +use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface; +use Symfony\Component\HttpKernel\KernelEvents; + +class LoggingExceptionListener implements EventSubscriberInterface +{ + protected $logger; + protected $httpStatusCodeLogLevel; + + public function __construct(LoggerInterface $logger = null, array $httpStatusCodeLogLevel = array()) + { + $this->logger = $logger ?: new NullLogger(); + $this->httpStatusCodeLogLevel = $httpStatusCodeLogLevel; + } + + public function logKernelException(GetResponseForExceptionEvent $event) + { + $exception = $event->getException(); + $level = $this->getExceptionLogLevel($exception); + $message = sprintf('Uncaught PHP Exception %s: "%s" at %s line %s', get_class($exception), $exception->getMessage(), $exception->getFile(), $exception->getLine()); + + $this->logger->log($level, $message, array('exception' => $exception)); + } + + public static function getSubscribedEvents(): array + { + return array( + KernelEvents::EXCEPTION => array( + array('logKernelException', 2048), + ), + ); + } + + protected function getExceptionLogLevel(\Exception $exception): string + { + $logLevel = LogLevel::CRITICAL; + if ($exception instanceof HttpExceptionInterface) { + $statusCode = $exception->getStatusCode(); + if (isset($this->httpStatusCodeLogLevel[$statusCode])) { + $logLevel = $this->httpStatusCodeLogLevel[$statusCode]; + } elseif ($statusCode >= 400 && $statusCode < 500) { + $logLevel = LogLevel::WARNING; + } + } + + return $logLevel; + } +} diff --git a/src/Symfony/Component/HttpKernel/EventListener/RenderControllerExceptionListener.php b/src/Symfony/Component/HttpKernel/EventListener/RenderControllerExceptionListener.php new file mode 100644 index 0000000000000..29a5a89b7e0e1 --- /dev/null +++ b/src/Symfony/Component/HttpKernel/EventListener/RenderControllerExceptionListener.php @@ -0,0 +1,106 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\HttpKernel\EventListener; + +use Psr\Log\LoggerInterface; +use Psr\Log\NullLogger; +use Symfony\Component\Debug\Exception\FlattenException; +use Symfony\Component\EventDispatcher\EventDispatcherInterface; +use Symfony\Component\EventDispatcher\EventSubscriberInterface; +use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpKernel\Event\FilterResponseEvent; +use Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent; +use Symfony\Component\HttpKernel\HttpKernelInterface; +use Symfony\Component\HttpKernel\KernelEvents; +use Symfony\Component\HttpKernel\Log\DebugLoggerInterface; + +/** + * RenderControllerExceptionListener renders an exception using the given controller. + */ +class RenderControllerExceptionListener implements EventSubscriberInterface +{ + protected $controller; + protected $logger; + + public function __construct($controller, LoggerInterface $logger = null) + { + $this->controller = $controller; + $this->logger = $logger ?: new NullLogger(); + } + + public static function getSubscribedEvents() + { + return array( + KernelEvents::EXCEPTION => array( + array('onKernelException', -128), + ), + ); + } + + public function onKernelException(GetResponseForExceptionEvent $event, string $eventName, EventDispatcherInterface $eventDispatcher) + { + $exception = $event->getException(); + $request = $this->duplicateRequest($exception, $event->getRequest()); + $response = $this->handleRequest($event->getKernel(), $request, $exception); + $this->addListenerToRemoveContentSecurityPolicyHeader($eventDispatcher); + $event->setResponse($response); + } + + private function duplicateRequest(\Exception $exception, Request $request): Request + { + $attributes = array( + '_controller' => $this->controller, + 'exception' => FlattenException::create($exception), + 'logger' => $this->logger instanceof DebugLoggerInterface ? $this->logger : null, + ); + $request = $request->duplicate(null, null, $attributes); + $request->setMethod('GET'); + + return $request; + } + + private function handleRequest($kernel, $request, $exception) + { + try { + return $kernel->handle($request, HttpKernelInterface::SUB_REQUEST, false); + } catch (\Exception $e) { + $this->logger->critical(sprintf('Exception thrown when handling an exception (%s: %s at %s line %s)', get_class($e), $e->getMessage(), $e->getFile(), $e->getLine()), array('exception' => $e)); + throw $this->pushOriginalExceptionIntoNewException($exception, $e); + } + } + + private function addListenerToRemoveContentSecurityPolicyHeader(EventDispatcherInterface $eventDispatcher) + { + $cspRemovalListener = function (FilterResponseEvent $event) use (&$cspRemovalListener, $eventDispatcher) { + $event->getResponse()->headers->remove('Content-Security-Policy'); + $eventDispatcher->removeListener(KernelEvents::RESPONSE, $cspRemovalListener); + }; + $eventDispatcher->addListener(KernelEvents::RESPONSE, $cspRemovalListener, -128); + } + + private function pushOriginalExceptionIntoNewException(\Exception $originalException, \Exception $newException): \Exception + { + $wrapper = $newException; + + while ($prev = $wrapper->getPrevious()) { + if ($originalException === $wrapper = $prev) { + return $newException; + } + } + + $prev = new \ReflectionProperty('Exception', 'previous'); + $prev->setAccessible(true); + $prev->setValue($wrapper, $originalException); + + return $newException; + } +} From 9236f62a0c950672be796174a05e9c2ae3c7f53a Mon Sep 17 00:00:00 2001 From: Michael Tibben Date: Fri, 5 Jan 2018 23:36:35 +1100 Subject: [PATCH 12/13] Revert "[HttpKernel] Decouple exception logging from rendering" This reverts commit a203d31838b9fde2f7ebe9f6e08a98a9e7fe3872. --- src/Symfony/Component/HttpKernel/CHANGELOG.md | 5 ----- .../EventListener/ExceptionListener.php | 15 ++++----------- .../HttpKernel/EventListener/ProfilerListener.php | 2 +- .../Tests/EventListener/ExceptionListenerTest.php | 6 ------ 4 files changed, 5 insertions(+), 23 deletions(-) diff --git a/src/Symfony/Component/HttpKernel/CHANGELOG.md b/src/Symfony/Component/HttpKernel/CHANGELOG.md index a8ec514ecc920..419e783ca43a0 100644 --- a/src/Symfony/Component/HttpKernel/CHANGELOG.md +++ b/src/Symfony/Component/HttpKernel/CHANGELOG.md @@ -1,11 +1,6 @@ CHANGELOG ========= -4.1.0 ------ - - * `ExceptionListener` now logs and collects exceptions at priority `2048` (previously logged at `-128` and collected at `0`) - 4.0.0 ----- diff --git a/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php b/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php index 0c1a38cff8944..efe4139b6cf8a 100644 --- a/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php +++ b/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php @@ -43,19 +43,15 @@ public function __construct($controller, LoggerInterface $logger = null) $this->logger = $logger; } - public function logKernelException(GetResponseForExceptionEvent $event) + public function onKernelException(GetResponseForExceptionEvent $event) { $exception = $event->getException(); $request = $event->getRequest(); + $eventDispatcher = func_num_args() > 2 ? func_get_arg(2) : null; $this->logException($exception, sprintf('Uncaught PHP Exception %s: "%s" at %s line %s', get_class($exception), $exception->getMessage(), $exception->getFile(), $exception->getLine())); - } - public function onKernelException(GetResponseForExceptionEvent $event) - { - $exception = $event->getException(); - $request = $this->duplicateRequest($exception, $event->getRequest()); - $eventDispatcher = func_num_args() > 2 ? func_get_arg(2) : null; + $request = $this->duplicateRequest($exception, $request); try { $response = $event->getKernel()->handle($request, HttpKernelInterface::SUB_REQUEST, false); @@ -91,10 +87,7 @@ public function onKernelException(GetResponseForExceptionEvent $event) public static function getSubscribedEvents() { return array( - KernelEvents::EXCEPTION => array( - array('logKernelException', 2048), - array('onKernelException', -128), - ), + KernelEvents::EXCEPTION => array('onKernelException', -128), ); } diff --git a/src/Symfony/Component/HttpKernel/EventListener/ProfilerListener.php b/src/Symfony/Component/HttpKernel/EventListener/ProfilerListener.php index 9cc554db72ab0..52e06e1b35513 100644 --- a/src/Symfony/Component/HttpKernel/EventListener/ProfilerListener.php +++ b/src/Symfony/Component/HttpKernel/EventListener/ProfilerListener.php @@ -121,7 +121,7 @@ public static function getSubscribedEvents() { return array( KernelEvents::RESPONSE => array('onKernelResponse', -100), - KernelEvents::EXCEPTION => array('onKernelException', 2048), + KernelEvents::EXCEPTION => 'onKernelException', KernelEvents::TERMINATE => array('onKernelTerminate', -1024), ); } diff --git a/src/Symfony/Component/HttpKernel/Tests/EventListener/ExceptionListenerTest.php b/src/Symfony/Component/HttpKernel/Tests/EventListener/ExceptionListenerTest.php index 778efc5bb3c16..02db732c10b88 100644 --- a/src/Symfony/Component/HttpKernel/Tests/EventListener/ExceptionListenerTest.php +++ b/src/Symfony/Component/HttpKernel/Tests/EventListener/ExceptionListenerTest.php @@ -55,13 +55,11 @@ public function testHandleWithoutLogger($event, $event2) $this->iniSet('error_log', file_exists('/dev/null') ? '/dev/null' : 'nul'); $l = new ExceptionListener('foo'); - $l->logKernelException($event); $l->onKernelException($event); $this->assertEquals(new Response('foo'), $event->getResponse()); try { - $l->logKernelException($event2); $l->onKernelException($event2); $this->fail('RuntimeException expected'); } catch (\RuntimeException $e) { @@ -78,13 +76,11 @@ public function testHandleWithLogger($event, $event2) $logger = new TestLogger(); $l = new ExceptionListener('foo', $logger); - $l->logKernelException($event); $l->onKernelException($event); $this->assertEquals(new Response('foo'), $event->getResponse()); try { - $l->logKernelException($event2); $l->onKernelException($event2); $this->fail('RuntimeException expected'); } catch (\RuntimeException $e) { @@ -164,7 +160,6 @@ public function testHttp4xxLogLevel() $exception = new \Exception('foo'); $event = new GetResponseForExceptionEvent(new TestKernelThatThrowsHttp4xxException(), Request::create('/'), HttpKernelInterface::MASTER_REQUEST, $exception); try { - $l->logKernelException($event); $l->onKernelException($event); $this->fail('NotFoundHttpException expected'); } catch (NotFoundHttpException $e) { @@ -183,7 +178,6 @@ public function testHttpLogLevelOverride() $exception = new \Exception('foo'); $event = new GetResponseForExceptionEvent(new TestKernelThatThrowsHttp4xxException(), Request::create('/'), HttpKernelInterface::MASTER_REQUEST, $exception); try { - $l->logKernelException($event); $l->onKernelException($event); $this->fail('NotFoundHttpException expected'); } catch (NotFoundHttpException $e) { From 1f08bc08ee2f275a2793cc81c39def9bd55b9206 Mon Sep 17 00:00:00 2001 From: Michael Tibben Date: Mon, 8 Jan 2018 12:42:04 +1100 Subject: [PATCH 13/13] Restore ExceptionListener as exception rendering listener, add BC mode --- .../Resources/config/services.xml | 1 + .../DependencyInjection/ConfigurationTest.php | 1 + .../TwigBundle/Resources/config/twig.xml | 3 +- .../EventListener/ExceptionListener.php | 33 ++++- .../LoggingExceptionListener.php | 15 +- .../RenderControllerExceptionListener.php | 106 ------------- .../EventListener/ExceptionListenerTest.php | 140 +++++++++++------- .../LoggingExceptionListenerTest.php | 133 +++++++++++++++++ .../EventListener/RouterListenerTest.php | 2 +- 9 files changed, 265 insertions(+), 169 deletions(-) delete mode 100644 src/Symfony/Component/HttpKernel/EventListener/RenderControllerExceptionListener.php create mode 100644 src/Symfony/Component/HttpKernel/Tests/EventListener/LoggingExceptionListenerTest.php diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/services.xml b/src/Symfony/Bundle/FrameworkBundle/Resources/config/services.xml index 4d706be51578d..57c592a52e4da 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Resources/config/services.xml +++ b/src/Symfony/Bundle/FrameworkBundle/Resources/config/services.xml @@ -77,6 +77,7 @@ %framework.exception_listener.http_log_levels% + diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php index 84921d9737d60..ec613c84944a8 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php +++ b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php @@ -249,6 +249,7 @@ class_exists(SemaphoreStore::class) && SemaphoreStore::isSupported() ? 'semaphor ), ), ), + 'http_exception_log_levels' => array(), ); } } diff --git a/src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml b/src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml index db9eaf26852f3..287f940e02677 100644 --- a/src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml +++ b/src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml @@ -123,11 +123,12 @@ - + %twig.exception_listener.controller% + false diff --git a/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php b/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php index efe4139b6cf8a..ceb0abcbf45f7 100644 --- a/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php +++ b/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php @@ -23,24 +23,22 @@ use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface; use Symfony\Component\EventDispatcher\EventSubscriberInterface; -@trigger_error(sprintf('The "%s" class is deprecated since Symfony 4.1 and will be removed in 5.0, use "%s" or "%s" instead.', ExceptionListener::class, RenderControllerExceptionListener::class, LoggingExceptionListener::class), E_USER_DEPRECATED); - /** * ExceptionListener. * * @author Fabien Potencier - * - * @deprecated since Symfony 4.1, use RenderControllerExceptionListener or LoggingExceptionListener instead */ class ExceptionListener implements EventSubscriberInterface { protected $controller; protected $logger; + private $isBCExceptionLoggingEnabled; - public function __construct($controller, LoggerInterface $logger = null) + public function __construct($controller, LoggerInterface $logger = null/*, bool $isBCExceptionLoggingEnabled = true*/) { $this->controller = $controller; $this->logger = $logger; + $this->isBCExceptionLoggingEnabled = (func_num_args() >= 3) ? (bool) func_get_arg(2) : true; } public function onKernelException(GetResponseForExceptionEvent $event) @@ -49,14 +47,21 @@ public function onKernelException(GetResponseForExceptionEvent $event) $request = $event->getRequest(); $eventDispatcher = func_num_args() > 2 ? func_get_arg(2) : null; - $this->logException($exception, sprintf('Uncaught PHP Exception %s: "%s" at %s line %s', get_class($exception), $exception->getMessage(), $exception->getFile(), $exception->getLine())); + if ($this->isBCExceptionLoggingEnabled) { + $this->logException($exception, sprintf('Uncaught PHP Exception %s: "%s" at %s line %s', get_class($exception), $exception->getMessage(), $exception->getFile(), $exception->getLine())); + } $request = $this->duplicateRequest($exception, $request); try { $response = $event->getKernel()->handle($request, HttpKernelInterface::SUB_REQUEST, false); } catch (\Exception $e) { - $this->logException($e, sprintf('Exception thrown when handling an exception (%s: %s at %s line %s)', get_class($e), $e->getMessage(), $e->getFile(), $e->getLine())); + $message = sprintf('Exception thrown when handling an exception (%s: %s at %s line %s)', get_class($e), $e->getMessage(), $e->getFile(), $e->getLine()); + if ($this->isBCExceptionLoggingEnabled) { + $this->logException($e, $message); + } elseif (null !== $this->logger) { + $this->logger->critical($message, array('exception' => $e)); + } $wrapper = $e; @@ -96,9 +101,13 @@ public static function getSubscribedEvents() * * @param \Exception $exception The \Exception instance * @param string $message The error message to log + * + * @deprecated since Symfony 4.1, to be removed in 5.0. Use LoggingExceptionListener instead to log exceptions. */ protected function logException(\Exception $exception, $message) { + @trigger_error(sprintf('The %s() method is deprecated since Symfony 4.1 and will be removed in 5.0. Use %s instead.', __METHOD__, LoggingExceptionListener::class), E_USER_DEPRECATED); + if (null !== $this->logger) { if (!$exception instanceof HttpExceptionInterface || $exception->getStatusCode() >= 500) { $this->logger->critical($message, array('exception' => $exception)); @@ -128,4 +137,14 @@ protected function duplicateRequest(\Exception $exception, Request $request) return $request; } + + /** + * @deprecated since Symfony 4.1, to be removed in 5.0. + * + * @internal + */ + public function isLoggingExceptions(): bool + { + return $this->isBCExceptionLoggingEnabled; + } } diff --git a/src/Symfony/Component/HttpKernel/EventListener/LoggingExceptionListener.php b/src/Symfony/Component/HttpKernel/EventListener/LoggingExceptionListener.php index 47d09799c628f..fcf68d8d1b22e 100644 --- a/src/Symfony/Component/HttpKernel/EventListener/LoggingExceptionListener.php +++ b/src/Symfony/Component/HttpKernel/EventListener/LoggingExceptionListener.php @@ -23,15 +23,28 @@ class LoggingExceptionListener implements EventSubscriberInterface { protected $logger; protected $httpStatusCodeLogLevel; + private $isDisabled; - public function __construct(LoggerInterface $logger = null, array $httpStatusCodeLogLevel = array()) + public function __construct(LoggerInterface $logger = null, array $httpStatusCodeLogLevel = array()/*, ExceptionListener $exceptionListener = null */) { $this->logger = $logger ?: new NullLogger(); $this->httpStatusCodeLogLevel = $httpStatusCodeLogLevel; + + if (func_num_args() >= 3) { + $exceptionListener = func_get_arg(2); + $this->isDisabled = ($exceptionListener instanceof ExceptionListener) ? $exceptionListener->isLoggingExceptions() : false; + if ($this->isDisabled) { + $this->logger->debug('Logging disabled for backwards compatibility, ExceptionListener is already logging exceptions.'); + } + } } public function logKernelException(GetResponseForExceptionEvent $event) { + if ($this->isDisabled) { + return; + } + $exception = $event->getException(); $level = $this->getExceptionLogLevel($exception); $message = sprintf('Uncaught PHP Exception %s: "%s" at %s line %s', get_class($exception), $exception->getMessage(), $exception->getFile(), $exception->getLine()); diff --git a/src/Symfony/Component/HttpKernel/EventListener/RenderControllerExceptionListener.php b/src/Symfony/Component/HttpKernel/EventListener/RenderControllerExceptionListener.php deleted file mode 100644 index 29a5a89b7e0e1..0000000000000 --- a/src/Symfony/Component/HttpKernel/EventListener/RenderControllerExceptionListener.php +++ /dev/null @@ -1,106 +0,0 @@ - - * - * For the full copyright and license information, please view the LICENSE - * file that was distributed with this source code. - */ - -namespace Symfony\Component\HttpKernel\EventListener; - -use Psr\Log\LoggerInterface; -use Psr\Log\NullLogger; -use Symfony\Component\Debug\Exception\FlattenException; -use Symfony\Component\EventDispatcher\EventDispatcherInterface; -use Symfony\Component\EventDispatcher\EventSubscriberInterface; -use Symfony\Component\HttpFoundation\Request; -use Symfony\Component\HttpKernel\Event\FilterResponseEvent; -use Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent; -use Symfony\Component\HttpKernel\HttpKernelInterface; -use Symfony\Component\HttpKernel\KernelEvents; -use Symfony\Component\HttpKernel\Log\DebugLoggerInterface; - -/** - * RenderControllerExceptionListener renders an exception using the given controller. - */ -class RenderControllerExceptionListener implements EventSubscriberInterface -{ - protected $controller; - protected $logger; - - public function __construct($controller, LoggerInterface $logger = null) - { - $this->controller = $controller; - $this->logger = $logger ?: new NullLogger(); - } - - public static function getSubscribedEvents() - { - return array( - KernelEvents::EXCEPTION => array( - array('onKernelException', -128), - ), - ); - } - - public function onKernelException(GetResponseForExceptionEvent $event, string $eventName, EventDispatcherInterface $eventDispatcher) - { - $exception = $event->getException(); - $request = $this->duplicateRequest($exception, $event->getRequest()); - $response = $this->handleRequest($event->getKernel(), $request, $exception); - $this->addListenerToRemoveContentSecurityPolicyHeader($eventDispatcher); - $event->setResponse($response); - } - - private function duplicateRequest(\Exception $exception, Request $request): Request - { - $attributes = array( - '_controller' => $this->controller, - 'exception' => FlattenException::create($exception), - 'logger' => $this->logger instanceof DebugLoggerInterface ? $this->logger : null, - ); - $request = $request->duplicate(null, null, $attributes); - $request->setMethod('GET'); - - return $request; - } - - private function handleRequest($kernel, $request, $exception) - { - try { - return $kernel->handle($request, HttpKernelInterface::SUB_REQUEST, false); - } catch (\Exception $e) { - $this->logger->critical(sprintf('Exception thrown when handling an exception (%s: %s at %s line %s)', get_class($e), $e->getMessage(), $e->getFile(), $e->getLine()), array('exception' => $e)); - throw $this->pushOriginalExceptionIntoNewException($exception, $e); - } - } - - private function addListenerToRemoveContentSecurityPolicyHeader(EventDispatcherInterface $eventDispatcher) - { - $cspRemovalListener = function (FilterResponseEvent $event) use (&$cspRemovalListener, $eventDispatcher) { - $event->getResponse()->headers->remove('Content-Security-Policy'); - $eventDispatcher->removeListener(KernelEvents::RESPONSE, $cspRemovalListener); - }; - $eventDispatcher->addListener(KernelEvents::RESPONSE, $cspRemovalListener, -128); - } - - private function pushOriginalExceptionIntoNewException(\Exception $originalException, \Exception $newException): \Exception - { - $wrapper = $newException; - - while ($prev = $wrapper->getPrevious()) { - if ($originalException === $wrapper = $prev) { - return $newException; - } - } - - $prev = new \ReflectionProperty('Exception', 'previous'); - $prev->setAccessible(true); - $prev->setValue($wrapper, $originalException); - - return $newException; - } -} diff --git a/src/Symfony/Component/HttpKernel/Tests/EventListener/ExceptionListenerTest.php b/src/Symfony/Component/HttpKernel/Tests/EventListener/ExceptionListenerTest.php index 02db732c10b88..8315f9744be40 100644 --- a/src/Symfony/Component/HttpKernel/Tests/EventListener/ExceptionListenerTest.php +++ b/src/Symfony/Component/HttpKernel/Tests/EventListener/ExceptionListenerTest.php @@ -22,7 +22,6 @@ use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Tests\Logger; -use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; /** * ExceptionListenerTest. @@ -33,10 +32,26 @@ */ class ExceptionListenerTest extends TestCase { + public function provider() + { + if (!class_exists('Symfony\Component\HttpFoundation\Request')) { + return array(array(null, null)); + } + + $request = new Request(); + $exception = new \Exception('foo'); + $event = new GetResponseForExceptionEvent(new TestKernel(), $request, HttpKernelInterface::MASTER_REQUEST, $exception); + $event2 = new GetResponseForExceptionEvent(new TestKernelThatThrowsException(), $request, HttpKernelInterface::MASTER_REQUEST, $exception); + + return array( + array($event, $event2), + ); + } + public function testConstruct() { $logger = new TestLogger(); - $l = new ExceptionListener('foo', $logger); + $l = new ExceptionListener('foo', $logger, false); $_logger = new \ReflectionProperty(get_class($l), 'logger'); $_logger->setAccessible(true); @@ -51,10 +66,25 @@ public function testConstruct() * @dataProvider provider */ public function testHandleWithoutLogger($event, $event2) + { + $this->doTestHandleWithoutLogger($event, $event2); + } + + /** + * @dataProvider provider + * @group legacy + * @expectedDeprecation The Symfony\Component\HttpKernel\EventListener\ExceptionListener::logException() method is deprecated since Symfony 4.1 and will be removed in 5.0. Use Symfony\Component\HttpKernel\EventListener\LoggingExceptionListener instead. + */ + public function testBcHandleWithoutLogger($event, $event2) + { + $this->doTestHandleWithoutLogger($event, $event2, true); + } + + public function doTestHandleWithoutLogger($event, $event2, $bc = false) { $this->iniSet('error_log', file_exists('/dev/null') ? '/dev/null' : 'nul'); - $l = new ExceptionListener('foo'); + $l = new ExceptionListener('foo', null, $bc); $l->onKernelException($event); $this->assertEquals(new Response('foo'), $event->getResponse()); @@ -72,6 +102,23 @@ public function testHandleWithoutLogger($event, $event2) * @dataProvider provider */ public function testHandleWithLogger($event, $event2) + { + $logger = new TestLogger(); + $l = new ExceptionListener('foo', $logger, false); + $l->onKernelException($event); + + $this->assertEquals(new Response('foo'), $event->getResponse()); + foreach ($logger->getLogs() as $logs) { + $this->assertCount(0, $logs); + } + } + + /** + * @dataProvider provider + * @group legacy + * @expectedDeprecation The Symfony\Component\HttpKernel\EventListener\ExceptionListener::logException() method is deprecated since Symfony 4.1 and will be removed in 5.0. Use Symfony\Component\HttpKernel\EventListener\LoggingExceptionListener instead. + */ + public function testBcHandleWithLogger($event, $event2) { $logger = new TestLogger(); @@ -92,25 +139,23 @@ public function testHandleWithLogger($event, $event2) $this->assertCount(3, $logger->getLogs('critical')); } - public function provider() + public function testSubRequestFormat() { - if (!class_exists('Symfony\Component\HttpFoundation\Request')) { - return array(array(null, null)); - } - - $request = new Request(); - $exception = new \Exception('foo'); - $event = new GetResponseForExceptionEvent(new TestKernel(), $request, HttpKernelInterface::MASTER_REQUEST, $exception); - $event2 = new GetResponseForExceptionEvent(new TestKernelThatThrowsException(), $request, HttpKernelInterface::MASTER_REQUEST, $exception); + $this->doTestSubRequestFormat(); + } - return array( - array($event, $event2), - ); + /** + * @group legacy + * @expectedDeprecation The Symfony\Component\HttpKernel\EventListener\ExceptionListener::logException() method is deprecated since Symfony 4.1 and will be removed in 5.0. Use Symfony\Component\HttpKernel\EventListener\LoggingExceptionListener instead. + */ + public function testBcSubRequestFormat() + { + $this->doTestSubRequestFormat(true); } - public function testSubRequestFormat() + public function doTestSubRequestFormat($bc = false) { - $listener = new ExceptionListener('foo', $this->getMockBuilder('Psr\Log\LoggerInterface')->getMock()); + $listener = new ExceptionListener('foo', $this->getMockBuilder('Psr\Log\LoggerInterface')->getMock(), $bc); $kernel = $this->getMockBuilder('Symfony\Component\HttpKernel\HttpKernelInterface')->getMock(); $kernel->expects($this->once())->method('handle')->will($this->returnCallback(function (Request $request) { @@ -128,6 +173,20 @@ public function testSubRequestFormat() } public function testCSPHeaderIsRemoved() + { + $this->doTestCSPHeaderIsRemoved(); + } + + /** + * @group legacy + * @expectedDeprecation The Symfony\Component\HttpKernel\EventListener\ExceptionListener::logException() method is deprecated since Symfony 4.1 and will be removed in 5.0. Use Symfony\Component\HttpKernel\EventListener\LoggingExceptionListener instead. + */ + public function testBcCSPHeaderIsRemoved() + { + $this->doTestCSPHeaderIsRemoved(true); + } + + public function doTestCSPHeaderIsRemoved($bc = false) { $dispatcher = new EventDispatcher(); $kernel = $this->getMockBuilder('Symfony\Component\HttpKernel\HttpKernelInterface')->getMock(); @@ -135,7 +194,7 @@ public function testCSPHeaderIsRemoved() return new Response($request->getRequestFormat()); })); - $listener = new ExceptionListener('foo', $this->getMockBuilder('Psr\Log\LoggerInterface')->getMock()); + $listener = new ExceptionListener('foo', $this->getMockBuilder('Psr\Log\LoggerInterface')->getMock(), $bc); $dispatcher->addSubscriber($listener); @@ -153,40 +212,23 @@ public function testCSPHeaderIsRemoved() $this->assertFalse($dispatcher->hasListeners(KernelEvents::RESPONSE), 'CSP removal listener has been removed'); } - public function testHttp4xxLogLevel() + /** + * @dataProvider provider + */ + public function testHandleWithLoggerWithoutBcWithSecondException($event, $event2) { $logger = new TestLogger(); - $l = new ExceptionListener('foo', $logger); - $exception = new \Exception('foo'); - $event = new GetResponseForExceptionEvent(new TestKernelThatThrowsHttp4xxException(), Request::create('/'), HttpKernelInterface::MASTER_REQUEST, $exception); - try { - $l->onKernelException($event); - $this->fail('NotFoundHttpException expected'); - } catch (NotFoundHttpException $e) { - $this->assertSame('4xx', $e->getMessage()); - $this->assertSame('foo', $e->getPrevious()->getMessage()); - } + $l = new ExceptionListener('foo', $logger, false); - $this->assertEquals(1, $logger->countErrors()); - $this->assertCount(1, $logger->getLogs('warning')); - } - - public function testHttpLogLevelOverride() - { - $logger = new TestLogger(); - $l = new ExceptionListener('foo', $logger, array(404 => 'notice')); - $exception = new \Exception('foo'); - $event = new GetResponseForExceptionEvent(new TestKernelThatThrowsHttp4xxException(), Request::create('/'), HttpKernelInterface::MASTER_REQUEST, $exception); try { - $l->onKernelException($event); - $this->fail('NotFoundHttpException expected'); - } catch (NotFoundHttpException $e) { - $this->assertSame('4xx', $e->getMessage()); + $l->onKernelException($event2); + $this->fail('RuntimeException expected'); + } catch (\RuntimeException $e) { + $this->assertSame('bar', $e->getMessage()); $this->assertSame('foo', $e->getPrevious()->getMessage()); } - $this->assertEquals(1, $logger->countErrors()); - $this->assertCount(1, $logger->getLogs('notice')); + $this->assertCount(1, $logger->getLogs('critical')); } } @@ -213,11 +255,3 @@ public function handle(Request $request, $type = self::MASTER_REQUEST, $catch = throw new \RuntimeException('bar'); } } - -class TestKernelThatThrowsHttp4xxException implements HttpKernelInterface -{ - public function handle(Request $request, $type = self::MASTER_REQUEST, $catch = true) - { - throw new NotFoundHttpException('4xx'); - } -} diff --git a/src/Symfony/Component/HttpKernel/Tests/EventListener/LoggingExceptionListenerTest.php b/src/Symfony/Component/HttpKernel/Tests/EventListener/LoggingExceptionListenerTest.php new file mode 100644 index 0000000000000..eb322f98c7014 --- /dev/null +++ b/src/Symfony/Component/HttpKernel/Tests/EventListener/LoggingExceptionListenerTest.php @@ -0,0 +1,133 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\HttpKernel\Tests\EventListener; + +use PHPUnit\Framework\TestCase; +use Psr\Log\NullLogger; +use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpFoundation\Response; +use Symfony\Component\HttpKernel\EventListener\ExceptionListener; +use Symfony\Component\HttpKernel\EventListener\LoggingExceptionListener; +use Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent; +use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; +use Symfony\Component\HttpKernel\HttpKernelInterface; +use Symfony\Component\HttpKernel\Tests\Logger as TestLogger; + +/** + * LoggingExceptionListenerTest. + * + * @group time-sensitive + */ +class LoggingExceptionListenerTest extends TestCase +{ + public function provider() + { + if (!class_exists('Symfony\Component\HttpFoundation\Request')) { + return array(array(null, null)); + } + + $request = new Request(); + $exception = new \Exception('foo'); + $event = new GetResponseForExceptionEvent(new LoggingExceptionListenerTestKernel(), $request, HttpKernelInterface::MASTER_REQUEST, $exception); + + $exception2 = new NotFoundHttpException('foo'); + $event2 = new GetResponseForExceptionEvent(new LoggingExceptionListenerTestKernel(), Request::create('/'), HttpKernelInterface::MASTER_REQUEST, $exception2); + + return array( + array($event, $event2), + ); + } + + public function testConstruct() + { + $logger = new TestLogger(); + $l = new LoggingExceptionListener($logger); + + $_logger = new \ReflectionProperty(get_class($l), 'logger'); + $_logger->setAccessible(true); + + $this->assertSame($logger, $_logger->getValue($l)); + } + + /** + * @dataProvider provider + */ + public function testHandleWithoutLogger($event, $event2) + { + $l = new LoggingExceptionListener(); + $l->logKernelException($event); + $l->logKernelException($event2); + + // happy path when the logger is missing, assert no exceptions thrown + $this->assertTrue(true); + } + + /** + * @dataProvider provider + */ + public function testHandleWithLogger($event, $event2) + { + $logger = new TestLogger(); + $l = new LoggingExceptionListener($logger); + $l->logKernelException($event); + $l->logKernelException($event2); + + $this->assertCount(1, $logger->getLogs('warning')); + $this->assertCount(1, $logger->getLogs('critical')); + } + + /** + * @dataProvider provider + */ + public function testHttpLogLevelOverride($event, $event2) + { + $logger = new TestLogger(); + $l = new LoggingExceptionListener($logger, array(404 => 'notice')); + $l->logKernelException($event); + $l->logKernelException($event2); + + $this->assertCount(1, $logger->getLogs('notice')); + $this->assertCount(1, $logger->getLogs('critical')); + } + + /** + * @dataProvider provider + */ + public function testBackwardsCompatibilityWithExceptionListener($event, $event2) + { + $customExceptionListener = new TestExtendedExceptionListener(null, new NullLogger()); + + $logger = new TestLogger(); + $l = new LoggingExceptionListener($logger, array(), $customExceptionListener); + $logger->clear(); + $l->logKernelException($event); + foreach ($logger->getLogs() as $logs) { + $this->assertCount(0, $logs); + } + } +} + +class LoggingExceptionListenerTestKernel implements HttpKernelInterface +{ + public function handle(Request $request, $type = self::MASTER_REQUEST, $catch = true) + { + return new Response('foo'); + } +} + +class TestExtendedExceptionListener extends ExceptionListener +{ + protected function logException(\Exception $exception, $message) + { + // no-op + } +} diff --git a/src/Symfony/Component/HttpKernel/Tests/EventListener/RouterListenerTest.php b/src/Symfony/Component/HttpKernel/Tests/EventListener/RouterListenerTest.php index 2668ede8e8e4d..832be298ff857 100644 --- a/src/Symfony/Component/HttpKernel/Tests/EventListener/RouterListenerTest.php +++ b/src/Symfony/Component/HttpKernel/Tests/EventListener/RouterListenerTest.php @@ -172,7 +172,7 @@ public function testWithBadRequest() $dispatcher->addSubscriber(new RouterListener($requestMatcher, $requestStack, new RequestContext())); $dispatcher->addSubscriber(new ExceptionListener(function () { return new Response('Exception handled', 400); - })); + }, null, false)); $kernel = new HttpKernel($dispatcher, new ControllerResolver(), $requestStack, new ArgumentResolver());