403 and 404 pages call error_log cluttering server error logs

This is a debug_backtrace() from each of the Access_Subscriber line 34 uncaught exceptions. I haven't parsed through it to see if it tells us anything useful (since the format_debug() version certainly didn't).

http://drupaltestbot-dev-mysql.osuosl.test/uncaughtexception.log

EDIT: The above link requires OSUOSL access.

The chain of execution is this: AccessSubscriber.php throws a 403, handleException catches it, dispatches a KernelEvents::EXCEPTION event and causes the ExceptionListener to fire which calls error_log.

Possible solutions include overriding handleException in our kernel not to dispatch the EXCEPTION event. I do not see a point really.

Handing it over to Crell for guidance.

And this is major if not critical because it causes our testbot disks to fill up. Ridiculous...

When investigating this, please also consider the NotFoundHttpException chain, which I assume is a similar chain of execution.

Not dispatching an EXCEPTION event is a non-starter. But perhaps an alternate ExceptionListener that only calls error_log() for really exceptional exceptions?

Tell me how to change the even subscribers and I will.

Crell's short recipe: Subclass ExceptionListener, stick it in the Core\EventSubscriber namespace, and update the code in the DIC registration that registers that listener.

He also approved my battle plan to copy the whole ExceptionListener class and wrap the error_log in an if (!httpexception)

Actually, why copy instead of subclass and override?

Because the whole class is one method :P (almost)

Eh, OK, I suppose that's a reason.

The bot is finicky today ;) what's a syntax error or two in the critical patch in the critical path, meh.

Ah ha, now I see why this is happening, and why Symfony sites don't melt servers. :-) The assumption is that you'd always have a logger object, so that code path doesn't get called. A Symfony site would. We do not yet. Related: #1289536: Switch Watchdog to a PSR-3 logging framework

Still, we shouldn't hold up on that. Maybe drop a @todo noting why we have to do it so we can back out later. And removing the (c) Fabien Potencier block and such. :-)

If we're going to bother introducing our own class for this - why not leave out the $logger property altogether? Copying the entire class and leaving it exactly as is bar this one little change seems crazy when half of that code will never run.

That makes even more sense. If we add a logger, we can drop this class and go back to the Symfony one, logger and all.

Took the axe to it.

I think it makes sense to remove the logger property from the class entirely. And so if we're going that far it seems reasonable to "adopt" the class as our own, i.e. with Drupal coding standards instead of Symfony's.

+++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionListener.php
@@ -0,0 +1,87 @@
+class ExceptionListener implements EventSubscriberInterface
+{

Missed the coding standards for classes and functions. :-)

+++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionListener.php
@@ -0,0 +1,87 @@
+    // Do not put a line in the server logs for every HTTP error.
+    if (!$exception instanceof HttpExceptionInterface) {

I rechecked, and the Symfony logger is still logging for HTTP 500 errors. I don't know if we want to do that or not. We need to reroll for formatting anyway, so we can add that here if we want, or not.

+++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionListener.php
@@ -0,0 +1,87 @@
+      // Re-throw the exception as this is a catch-all
+      return;

I know this is in Symfony, but it looks wrong. We're not rethrowing an exception, we're swallowing it. Rightly or wrongly, the comment doesn't match the code. (That said, it is what Symfony is doing, so I'm OK with leaving it as is and opening up an upstream issue to fix the comment at least.)

Otherwise this looks good.

Missed the coding standards for classes and functions. :-)

Fixed

the Symfony logger is still logging for HTTP 500 errors. I don't know if we want to do that or not.

I think that makes sense, added.

the comment doesn't match the code

Actually both that comment and the one above it were bothering me, so I got rid of the second one seeing as it's incorrect and I don't think we really need a comment there at all, and I changed the previous one to make grammatical sense.

Yay for fewer gigabyte-sized log files.

Confirmed that this does indeed prevent the uncaught AccessDeniedHttpException from hitting the testbot apache log on at least one of the tests which was causing it earlier.

+++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionListener.phpundefined
@@ -0,0 +1,83 @@
+/**
+ * ExceptionListener.

I was going to mark needs work for this, only to discover that this is what passes for PHPDoc in the Symfony project. :\ Egads.

Brainstormed with chx and jthorson on IRC and came up with "Override of Symfony EventListener class to kill 403 and 404 from server logs." Fixed on commit.

Committed and pushed to 8.x. Yay fewer hard drive fill-ups! :)