Skip to content

[FrameworkBundle] enable ErrorHandler in prod #12062

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[FrameworkBundle] enable ErrorHandler in prod #12062

wants to merge 1 commit into from

Conversation

nicolas-grekas
Copy link
Member

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #11053, #8281
License MIT
Doc PR -

This is a first proposal for enabling error handling (mainly logging) in prod envs.
I'd like to make it for 2.6 but need help to make the best decisions.
@Koc worked on the subject but issue #8281 remains.

This misses configuration options for the ErrorHandler and for php ini settings related to error handling:
I'd like to allow apps to define any log channel per log level,
and I'd also like to be able to ini_set('error_log', ...) through symfony configuration system, with app/logs/php.log as default.

WIP state, but early feedback welcomed. We have only a few days to make it.

https://github.com/symfony/symfony/pull/12062/files?w=1

@Koc
Copy link
Contributor

Koc commented Sep 27, 2014

We haven't solved this issue at last 3 years and IMHO it is ok wait 6 month until 2.7 will be released. Last-minute rush can decrease quality of the sollution.

Logging fatal errors would be better with stacktrace, but xdebug is so slow for production. What about thinking to this issue from other side? Is it possible create production-ready extension from this https://wiki.php.net/rfc/engine_exceptions RFC?

@nicolas-grekas
Copy link
Member Author

The rush is relative: we have 2 months after feature freeze to finalize, and we have some experience enabling this feature in existing prod apps.
About stack trace for fatal errors, I wonder if we can add a prod friendly variant of xdebug_get_function_stack() in our shinny new debug extension for 2.7. I'll talk with @jpauli about that.

@Nicofuma
Copy link
Contributor

@Koc The result of the poll was no. So I don't think that's so useful prepare something for it.

@nicolas-grekas nicolas-grekas force-pushed the prod-error-handler branch 3 times, most recently from d303503 to 0fa3ac0 Compare September 29, 2014 15:05
@nicolas-grekas nicolas-grekas changed the title [WIP] [FrameworkBundle] enable ErrorHandler in prod [FrameworkBundle] enable ErrorHandler in prod Sep 29, 2014
@nicolas-grekas
Copy link
Member Author

So this is not WIP anymore:

  • a new debug.error_handler service is the registered PHP error handler, with ErrorHandler::register() as factory
  • ErrorHandler::register() is patched so that it checks if the currently registered error handler is an instance of ErrorHandler - in which case it returns this instance and don't create a new one.
  • DebugHandlersListener now listen to ConsoleEvents and re-injects fatal errors within the $app->renderException code path

When kernel.debug=false, no exception is thrown on error (same as today) but the already configured logger is called now, with screaming switched on for fatal errors (which means silenced fatal errors are logged but not the other silenced ones).
The debug mode is unchanged (throws non silenced errors and logs silenced ones).

@nicolas-grekas
Copy link
Member Author

Merging #12065 is required to make tests pass on this PR.

lyrixx
lyrixx reviewed Sep 29, 2014
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
}

$this->addClassesToCompile(array(
'Symfony\\Component\\Config\\FileLocator',
'Symfony\\Component\\Debug\\ErrorHandler',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing empty line to keep consistency (group by component)

@lyrixx
Copy link
Member

lyrixx commented Sep 29, 2014

It seems good to me.

@nicolas-grekas nicolas-grekas force-pushed the prod-error-handler branch 4 times, most recently from 9d725b1 to 5e43a69 Compare September 29, 2014 18:46
@nicolas-grekas nicolas-grekas force-pushed the prod-error-handler branch 2 times, most recently from f14c2d9 to 513c9a0 Compare September 29, 2014 21:13
@nicolas-grekas
Copy link
Member Author

So, I switched myself in @stof mode.
I reduced the patch to its minimum, added tests and doc block.
I also replaced the dep on the Console component by a calling defined() before using the ConsoleEvents::COMMAND const.

nicolas-grekas
nicolas-grekas reviewed Sep 29, 2014
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
@@ -133,8 +133,6 @@ public function load(array $configs, ContainerBuilder $container)
if ($container->getParameter('kernel.debug')) {
$loader->load('debug.xml');

$definition->replaceArgument(0, array(new Reference('http_kernel', ContainerInterface::NULL_ON_INVALID_REFERENCE), 'terminateWithException'));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is replaced by a dynamic fetch of the kernel in the configured listener service (see below): $event->getKernel()

@mvrhov
Copy link

mvrhov commented Sep 30, 2014

👍 As ATM I'm using a TracyBundle for production debugging. Would gladly switch to Symfony only implementation.

@stof
Copy link
Member

stof commented Sep 30, 2014

looks good to me. 👍

@nicolas-grekas nicolas-grekas mentioned this pull request Sep 30, 2014
Merged
@nicolas-grekas
Copy link
Member Author

Replaced by #12081

@nicolas-grekas nicolas-grekas deleted the prod-error-handler branch October 2, 2014 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
FrameworkBundle
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@nicolas-grekas @Koc @Nicofuma @lyrixx @mvrhov @stof @kingcrunch @romainneutron