Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Fixed some issues of the AccessDecisionManager profiler #18934

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 11 commits into from
Closed
Prev Previous commit
Next Next commit
Load the debug config conditionally
  • Loading branch information
javiereguiluz committed Jun 29, 2016
commit 804fcecbc92599818c88aca122d8622beb313e00
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,10 @@ public function load(array $configs, ContainerBuilder $container)
$loader->load('templating_twig.xml');
$loader->load('collectors.xml');
Copy link
Member

Choose a reason for hiding this comment

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

@javiereguiluz the issue still occurs if profiler service is not defined, so data_collector.security require of debug.security.access.decision_manager

Copy link
Member Author

Choose a reason for hiding this comment

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

Could we solve this via config?

<argument type="service" id="debug.security.access.decision_manager" on-invalid="ignore" />

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe just load the collector in debug?

Copy link
Member Author

Choose a reason for hiding this comment

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

@HeahDude apparently we can't do that. See #18934 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I see another solution, now that the security.access.decision_manager is properly decorated, just pass it to the collector instead of the debug.security.access.decision_manager, this line should ensure everything is fine, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Let's try that! Thanks.

$loader->load('guard.xml');
$loader->load('security_debug.xml');

if ($container->getParameter('kernel.debug')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should keep the check with hasParameter() since some tests are still failing

$loader->load('security_debug.xml');
Copy link
Member

@yceruto yceruto Jun 7, 2016

Choose a reason for hiding this comment

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

@javiereguiluz I tried this PR and the issue still exists :(

https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/SecurityBundle/Resources/config/collectors.xml#L13: The data_collector.security service ("which is always loaded" L66) require debug.security.access.decision_manager, so when $container->getParameter('kernel.debug') is false this service isn't loaded:

php bin/console --env=dev --no-debug

  [Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException]
  The service "profiler" has a dependency on a non-existent service "debug.security.access.decision_manager".

The road should be here https://github.com/symfony/symfony-standard/issues/968#issuecomment-223022665:

@stof: ... It should be based on whether the profiler is enabled instead of being based on the debug mode ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it OK to load the collectors.xml file unconditionally? Is it really used when debug = false ? If this can be changed, the solution would be trivial: move $loader->load('collectors.xml'); under the if ($container->getParameter('kernel.debug')) condition.

Copy link
Member

Choose a reason for hiding this comment

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

well, if you do it, the security panel would not appear when enabling the profiler in no-debug mode, which might be confusing

Copy link
Member

@yceruto yceruto Jun 10, 2016

Choose a reason for hiding this comment

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

Could we check web_profiler.controller.profiler service definition ? it's defined only if WebProfileBundle is enabled. BUT, this would imply that all panels profile should be loaded only if this "WebProfileBundle" is enabled ? Currenly this is not taken into consideration in others collectors.

Copy link
Member

Choose a reason for hiding this comment

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

@yceruto WebProfilerBundle is not related to enabling the profiler (the bundle does not provide the profiler. It is only about providing a visualization UI).
The proper service would be detecting profiler (coming from FrameworkBundle), but this cannot be done in a DI extension, because DI extension cannot see services defined in other extensions (see the previous discussion)

Copy link
Member

Choose a reason for hiding this comment

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

@stof you're right, then

#18934 (comment)

... Only the parameters are copied from actual to tmp container ...

We can create a new parameter something like profiler.collect into FrameworkExtension::registerProfilerConfiguration ?:

$container->setParameter('profiler.collect', $config['collect']);

and use it here ?

if ($container->hasParameter('profiler.collect') && $container->getParameter('profiler.collect')) {
    $loader->load('collectors.xml');
    $loader->load('security_debug.xml');
}

@javiereguiluz WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

@yceruto my guess is that they won't accept to introduce a parameter for this. Let's try to do the compilar pass that @stof suggested in the first place.

}

if (!class_exists('Symfony\Component\ExpressionLanguage\ExpressionLanguage')) {
$container->removeDefinition('security.expression_language');
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.