[HttpKernel][DX] Allow the log level of http exceptions to be overridden

[mtibben]

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#9712
License	MIT
Doc PR

This allows the log level to be configured based on the specific HTTP status code.

This is an alternative approach to #23707. The problem with the approach in #23707 (and with the documented approaches to filter 404s) is that it only works with the fingers crossed handler. However the fingers crossed handler introduces unwanted behaviour, adds complexity to the log configuration, and can be confusing for a developer.

This PR takes a more direct approach. The actual problem is that different applications have different requirements as to what level a specific http code should be logged at. So this makes this behaviour directly configurable.

[ro0NL]

i think i'd be in favor or 4xx logged as WARNING, that would save us some regular log filtering :-)

for the framework config; should it be semantic? framework.http_exception.log_levels or so?

[mtibben]

i think i'd be in favor or 4xx logged as WARNING, that would save us some regular log filtering :-)

Done - how does this impact BC?

for the framework config; should it be semantic? framework.http_exception.log_levels or so?

Yes that makes a lot more sense.

[mtibben]

for the framework config; should it be semantic? framework.http_exception.log_levels or so?

I've just implemented the configuration changes, for example it currently looks something like

twig:
    http_exception_log_levels:
        404: INFO
        403: NOTICE

I'll need some guidance on how to make this configurable via the framework bundle rather than the twig bundle @ro0NL.

Actually it seems pretty weird to me that the twig bundle is responsible for logging http exceptions... any reason behind that, or just an accident of history?

[ro0NL]

Actually it seems pretty weird to me that the twig bundle is responsible for logging http exceptions... any reason behind that, or just an accident of history?

Agree, and it's the way it's always has been AFAIK. Note recently we decoupled logging from rendering into different priorities (see #25366), perhaps now we should favor 2 different listeners instead (logging in the framework bundle, rendering in the twig bundle).

cc @fabpot

[mtibben]

perhaps now we should favor 2 different listeners instead (logging in the framework bundle, rendering in the twig bundle)

Looking at this, but might be tricky to do in a BC way

[mtibben]

I've had an initial stab at splitting the listener into two.

• LoggingExceptionListener - Logging
• RenderControllerExceptionListener - Rendering an error controller

I've left the original ExceptionListener in place for BC, although it's no longer used by the framework. Any suggested strategy to deal with BC for old services?

And what's the BC policy for service names? Is it OK to remove old service names?

[mtibben]

And the config looks like

framework:
    http_exception_log_levels:
        404: INFO
        403: NOTICE

[mtibben]

OK this is ready for further feedback @ro0NL @nicolas-grekas @sstok. I'm not sure why the tests on php 7.2 are failing though 😕

[mtibben]

I've rebased on the latest master, any feedback?

[fabpot]

@yceruto Maybe you can help here.

[fabpot]

We've just moved away from master as the main branch to use 5.x instead. Unfortunately, I cannot reopen the PR and change the target branch to 5.x. Can you open a new PR referencing this one to not loose the discussion? Thank you for your understanding and for your help.

[mtibben]

I won't get a chance to revisit this. If anyone else reading this wants to pick this up, feel free!