-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Changes from 1 commit
7721ba4
639c8ea
16dd82b
ec9ffeb
2ee2361
800ef55
e4cbf01
5d96ec6
804fcec
e3c9c29
82f0541
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,7 +65,10 @@ public function load(array $configs, ContainerBuilder $container) | |
$loader->load('templating_twig.xml'); | ||
$loader->load('collectors.xml'); | ||
$loader->load('guard.xml'); | ||
$loader->load('security_debug.xml'); | ||
|
||
if ($container->getParameter('kernel.debug')) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should keep the check with |
||
$loader->load('security_debug.xml'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it OK to load the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we check There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @stof you're right, then
We can create a new parameter something like $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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
|
||
if (!class_exists('Symfony\Component\ExpressionLanguage\ExpressionLanguage')) { | ||
$container->removeDefinition('security.expression_language'); | ||
|
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.
@javiereguiluz the issue still occurs if
profiler
service is not defined, sodata_collector.security
require ofdebug.security.access.decision_manager
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.
Could we solve this via config?
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.
Or maybe just load the collector in debug?
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.
@HeahDude apparently we can't do that. See #18934 (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.
Ok, I see another solution, now that the
security.access.decision_manager
is properly decorated, just pass it to the collector instead of thedebug.security.access.decision_manager
, this line should ensure everything is fine, right?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.
👍 Let's try that! Thanks.