ExceptionLoggingSubscriber should not log HTTP 4XX errors using PHP logger channel

Not sure I follow?

@smustgraveI think what @mfb is trying to say is, send the 4xx code as part of the $context array. Because it is hidden behind the generic client error.

Like this?

Yes I think that's the jist of the suggestion.

quietone → credited Berdir → .

quietone → credited samiullah → .

i closed 🐛 Generic logging for 4xx errors Closed: duplicate as a duplicate, moving credit.

quietone → credited slip → .

quietone → credited Primsi → .

quietone → credited ravi.shankar → .

quietone → credited joshua1234511 → .

quietone → credited johnwebdev → .

1. 
   +++ b/core/modules/dblog/tests/src/Functional/DbLogTest.php
   +++ b/core/modules/dblog/tests/src/Functional/DbLogTest.php
   @@ -183,6 +183,41 @@ public function test403LogEventPage() {
   
   @@ -183,6 +183,41 @@ public function test403LogEventPage() {
        $this->assertMatchesRegularExpression($regex, $message[0]->getText());
      }
    
   +  /**
   +   * Tests that 4xx errors, outside 403 and 404, are logged with the exception.
   +   */
   +  public function testClientErrors() {
   +    // Trigger BadRequestException.
   

   I'm not sure we need this if we have the Kernel test - depending on the answer to the question about the kernel test.

2. +++ b/core/tests/Drupal/KernelTests/Core/EventSubscriber/ExceptionLoggingSubscriberTest.php
   @@ -0,0 +1,57 @@
   +
   +    $channel_map = [
   +      403 => 'access denied',
   +      404 => 'page not found',
   +      // 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',
   +    ];
   

   This isn't testing the non-403/404 4xx errors that this issue is about, should it be?

Removed functional test

#67 looks good.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionLoggingSubscriber.php
   @@ -88,10 +104,16 @@ public function onException(ExceptionEvent $event) {
   +      else {
   +        if ($status_code >= 400 && $status_code < 500) {
   +          $method = 'onClientError';
   +        }
   

   shouldn't this be elseif? as much as I hate elseif, its odd to see an if inside an else

2. +++ b/core/modules/system/tests/modules/error_test/error_test.routing.yml
   @@ -40,3 +40,10 @@ error_test.trigger_renderer_exception:
   +error_test.trigger_bad_request_exception:
   +  path: '/error-test/trigger-bad_request-exception'
   +  defaults:
   +    _controller: '\Drupal\error_test\Controller\ErrorTestController::triggerBadRequestException'
   +  requirements:
   +    _access: 'TRUE'
   
   +++ b/core/modules/system/tests/modules/error_test/src/Controller/ErrorTestController.php
   @@ -5,6 +5,7 @@
   +use Symfony\Component\HttpFoundation\Exception\BadRequestException;
   
   @@ -98,4 +99,12 @@ function () {
   +  /**
   +   * Trigger a bad request exception.
   +   */
   +  public function triggerBadRequestException() {
   +    define('SIMPLETEST_COLLECT_ERRORS', FALSE);
   +    throw new BadRequestException();
   +  }
   

   is this used? the test seems to be hitting an existing route

3. +++ b/core/tests/Drupal/KernelTests/Core/EventSubscriber/ExceptionLoggingSubscriberTest.php
   @@ -0,0 +1,61 @@
   +    // Make sure that HTTP exceptions are logged into correct channel.
   +    $this->installSchema('dblog', ['watchdog']);
   

   Let's not use the database to collect the log messages, we can just make the kernel test a logger 💪

   See https://www.previousnext.com.au/blog/making-your-drupal-8-kernel-tests-f... for an example

Or even simpler, use the test logger like in 🐛 \Drupal\language\LanguageNegotiator does not handle PluginNotFoundException and break the site completely Fixed - learnt something new 🔥

Let me work on #69

Resolved the 1st issue from #69 and created PR.

What about #2 and #3 @sourabhjain?

Going to cancel the test run, its a waste of DA resources without addressing the feedback.

Attempted to address all points.

+ 502 => 'php',

I know this issue is specifically about 4xx errors, but i'd like to point that 5xx errors shouldn't be always logged in PHP channel. For example I have a project where Drupal serves as a gateway to another site. So when the upstream site fails Drupal returns 502 exception.
That's part of normal workflow and has nothing to do with PHP.

I think PHP channel should only be used for exceptions that got 500 status code in FinalExceptionSubscriber. That might need a separate issue.

This issue is being reviewed by the kind folks in Slack → , #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

Thanks for working on this, found some more stuff to remove from the test ;).

diff --git a/core/modules/system/tests/modules/error_test/src/Controller/ErrorTestController.php b/core/modules/system/tests/modules/error_test/src/Controller/ErrorTestController.php
index b15bb9b00c..b550b4fda4 100644
--- a/core/modules/system/tests/modules/error_test/src/Controller/ErrorTestController.php
+++ b/core/modules/system/tests/modules/error_test/src/Controller/ErrorTestController.php
@@ -5,6 +5,7 @@
 use Drupal\Core\Controller\ControllerBase;
 use Drupal\Core\Database\Connection;
 use Symfony\Component\DependencyInjection\ContainerInterface;
+use Symfony\Component\HttpFoundation\Exception\BadRequestException;
 
 /**
  * Controller routines for error_test routes.
@@ -98,4 +99,12 @@ function () {
     ];
   }
 
+  /**
+   * Trigger a bad request exception.
+   */
+  public function triggerBadRequestException() {
+    define('SIMPLETEST_COLLECT_ERRORS', FALSE);
+    throw new BadRequestException();
+  }
+
 }

To address #69.2 the latest patch removed the route that wasn't used, but I think this needs to be removed as well. Without the route this is now dead code.

diff --git a/core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php b/core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php
index c85cc3c6e1..969c364890 100644
--- a/core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php
+++ b/core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php
@@ -94,7 +94,7 @@ public function testUncaughtException() {
   public function testUncaughtFatalError() {
     $fatal_error = [
       '%type' => 'TypeError',
-      '@message' => 'Drupal\error_test\Controller\ErrorTestController::Drupal\error_test\Controller\{closure}(): Argument #1 ($test) must be of type array, string given, called in ' . \Drupal::root() . '/core/modules/system/tests/modules/error_test/src/Controller/ErrorTestController.php on line 65',
+      '@message' => 'Drupal\error_test\Controller\ErrorTestController::Drupal\error_test\Controller\{closure}(): Argument #1 ($test) must be of type array, string given, called in ' . \Drupal::root() . '/core/modules/system/tests/modules/error_test/src/Controller/ErrorTestController.php on line 66',
       '%function' => 'Drupal\error_test\Controller\ErrorTestController->Drupal\error_test\Controller\{closure}()',
     ];
     $this->drupalGet('error-test/generate-fatals');

Since we don't use the error_test module in the test at all and we thus don't need to make any modifications to that module anymore, this can most likely also be reverted.

+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp(): void {
+    parent::setUp();
+    // Prepare the logger for collecting the expected critical error.
+    $this->container->get($this->testLogServiceName)->cleanLogs();
+  }

Further down in the test method, we're already calling $this->assertEmpty($this->container->get($this->testLogServiceName)->cleanLogs()); before we start triggering errors. I don't think we need to call this twice. I think asserting that the logger is empty in the test is a good idea, so we should probably just remove the setup method.

+  protected static $modules = ['system', 'test_page_test', 'dblog'];

and

+    // Make sure that HTTP exceptions are logged into correct channel.
+    $this->installSchema('dblog', ['watchdog']);

Given that we now use a test logger, I don't think we need need dblog anymore, do we?

Tagging "Needs followup" for #76.

This came up again in 🐛 AssetControllerBase.php - "The theme must be passed as a query argument" Active .

Opened 🐛 ExceptionLoggingSubscriber should not log HTTP 5XX errors using PHP logger channel Active

Points from #77 do appear to be resolved.

Committed/pushed to 11.x, cherry-picked to 10.2.x and 10.1.x, thanks!

Automatically closed - issue fixed for 2 weeks with no activity.