Issue #2828706 by smustgrave, Chi, saidatom, dagmar, dawehner, mfb, catch, larowlan, longwave, joshua1234511, Berdir, quietone, FeyP, slip, samiullah, johnwebdev, Primsi, ravi.shankar: ExceptionLoggingSubscriber should not log HTTP 4XX errors using PHP logger channel

core/lib/Drupal/Core/EventSubscriber/ExceptionLoggingSubscriber.php

@@ -75,6 +75,22 @@ class ExceptionLoggingSubscriber implements EventSubscriberInterface {
     }
   }
 
+  /**
+   * Log 4xx errors that are not 403 or 404.
+   *
+   * @param \Symfony\Component\HttpKernel\Event\ExceptionEvent $event
+   *   The event to process.
+   */
+  public function onClientError(ExceptionEvent $event) {
+    $exception = $event->getThrowable();
+    $error = Error::decodeException($exception);
+    $error += [
+      'status_code' => $exception->getStatusCode(),
+    ];
+    $this->logger->get('client error')
+      ->log($error['severity_level'], Error::DEFAULT_ERROR_MESSAGE, $error);
+  }
+
   /**
    * Log all exceptions.
    *
@@ -88,10 +104,14 @@ class ExceptionLoggingSubscriber implements EventSubscriberInterface {
 
     // Treat any non-HTTP exception as if it were one, so we log it the same.
     if ($exception instanceof HttpExceptionInterface) {
-      $possible_method = 'on' . $exception->getStatusCode();
+      $status_code = $exception->getStatusCode();
+      $possible_method = 'on' . $status_code;
       if (method_exists($this, $possible_method)) {
         $method = $possible_method;
       }
+      elseif ($status_code >= 400 && $status_code < 500) {
+        $method = 'onClientError';
+      }
     }
 
     $this->$method($event);

core/tests/Drupal/KernelTests/Core/EventSubscriber/ExceptionLoggingSubscriberTest.php

@@ -0,0 +1,81 @@
+<?php
+
+namespace Drupal\KernelTests\Core\EventSubscriber;
+
+use Drupal\Core\DependencyInjection\ContainerBuilder;
+use Drupal\KernelTests\KernelTestBase;
+use Symfony\Component\ErrorHandler\BufferingLogger;
+use Symfony\Component\HttpFoundation\Request;
+
+/**
+ * Tests that HTTP exceptions are logged correctly.
+ *
+ * @group system
+ */
+class ExceptionLoggingSubscriberTest extends KernelTestBase {
+
+  /**
+   * The service name for a logger implementation that collects anything logged.
+   *
+   * @var string
+   */
+  protected $testLogServiceName = 'exception_logging_subscriber_test.logger';
+
+  /**
+   * {@inheritdoc}
+   */
+  protected static $modules = ['system', 'test_page_test'];
+
+  /**
+   * Tests \Drupal\Core\EventSubscriber\ExceptionLoggingSubscriber::onException().
+   */
+  public function testExceptionLogging() {
+    $http_kernel = \Drupal::service('http_kernel');
+
+    $channel_map = [
+      400 => 'client error',
+      401 => 'client error',
+      403 => 'access denied',
+      404 => 'page not found',
+      405 => 'client error',
+      408 => 'client error',
+      // Do not check the 500 status code here because it would be caught by
+      // Drupal\Core\EventSubscriberExceptionTestSiteSubscriber which has lower
+      // priority.
+      501 => 'php',
+      502 => 'php',
+      503 => 'php',
+    ];
+
+    // Ensure that noting is logged.
+    $this->assertEmpty($this->container->get($this->testLogServiceName)->cleanLogs());
+
+    // Temporarily disable error log as the ExceptionLoggingSubscriber logs 5xx
+    // HTTP errors using error_log().
+    $error_log = ini_set('error_log', '/dev/null');
+    foreach ($channel_map as $code => $channel) {
+      $request = Request::create('/test-http-response-exception/' . $code);
+      $http_kernel->handle($request);
+    }
+    ini_set('error_log', $error_log);
+
+    $expected_channels = array_values($channel_map);
+
+    $logs = $this->container->get($this->testLogServiceName)->cleanLogs();
+    foreach ($expected_channels as $key => $expected_channel) {
+      $log_message = $logs[$key][2]['channel'];
+      $this->assertEquals($expected_channel, $log_message);
+    }
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function register(ContainerBuilder $container) {
+    parent::register($container);
+    $container
+      ->register($this->testLogServiceName, BufferingLogger::class)
+      ->addTag('logger');
+  }
+
+}