Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpKernel] Fix error logger when stderr is redirected to /dev/null#36855
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM, here are some minor things.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
dunglas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
lgtm
Naktibalda commentedDec 11, 2020
I really don't like this change because php-fpm prepends I made logger work the way it did before by adding this section to config/services.yaml logger:class:Symfony\Component\HttpKernel\Log\Loggerarguments:$output:'php://stderr' |
stof commentedDec 11, 2020
@Naktibalda if your PHP-FPM is configured to preserve stderr, that's a valid configuration (if you still don't want to use a more powerful logger like monolog). Having a |
…el Logger (cyve)This PR was squashed before being merged into the 4.4 branch.Discussion----------[HttpKernel] Remove EOL when using error_log() in HttpKernel Logger| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | yes| New feature? | no| Deprecations? | maybe| Tickets || License | MIT| Doc PR |Hello there !Since PR#36855, when using `Symfony\Component\HttpKernel\Log\Logger` with argument `$output = null`. The logger use the `error_log()` method to log messages. But in the log file, messages are separated by an empty line because the default formatter (`Logger::format()`) adds an extra EOL char at the end of the message.This fix prevents the logger to add an extra EOL char when it uses `error_log()`.Possible BC : if someone use the logger with a custom formatter that already add a EOL char at the end of the message, there will be a empty line between messages in the log file. In my opinion, this is minor enough not to worry about it ;-)Thanks for your review :-)Commits-------69cf83e [HttpKernel] Remove EOL when using error_log() in HttpKernel Logger
…el Logger (cyve)This PR was squashed before being merged into the 4.4 branch.Discussion----------[HttpKernel] Remove EOL when using error_log() in HttpKernel Logger| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | yes| New feature? | no| Deprecations? | maybe| Tickets || License | MIT| Doc PR |Hello there !Since PRsymfony/symfony#36855, when using `Symfony\Component\HttpKernel\Log\Logger` with argument `$output = null`. The logger use the `error_log()` method to log messages. But in the log file, messages are separated by an empty line because the default formatter (`Logger::format()`) adds an extra EOL char at the end of the message.This fix prevents the logger to add an extra EOL char when it uses `error_log()`.Possible BC : if someone use the logger with a custom formatter that already add a EOL char at the end of the message, there will be a empty line between messages in the log file. In my opinion, this is minor enough not to worry about it ;-)Thanks for your review :-)Commits-------69cf83ea1a [HttpKernel] Remove EOL when using error_log() in HttpKernel Logger
Uh oh!
There was an error while loading.Please reload this page.
The HttpKernel Logger is meant to be used as a last resort logging mechanism when no logger has been explicitly configured (Monolog is not a dependency for instance).
For small apps, that can be more than enough.
But under some circumstances, it does not work. When you are using PHP-FPM,
stderris ignored by default (catch_workers_outputisfalse) and so, logs are ignored as well. There is no issue with the official PHP Docker image as the setting has been explicitly set totrue. Not an issue with Symfony CLI as well, as we also change the setting. Not a problem either with the PHP built-in server as it does not use PHP FPM anyway.But, in many other places, where the setting has its default value, logs are lost (as you can imagine, it happened to me). As this feature is meant to be a fallback, I think it should always work, or at least, we need to make everything possible to make it work out of the box; that's why I've considered it a bug and hence a PR on 3.4.
This PR changes the default value for the output to
null, which useserror_log()instead ofstderrto log errors. Why is it better? The output oferror_log()is controllable by theerror_logsPHP ini setting and it is well understood by everyone (the default configuration should always work well); so it should work in most/more cases.The other change (to be discussed) is to also log messages at the
ERRORlevel and not just theCRITICALones./cc@dunglas