Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[SecurityBundle][Profiler] Give info about called security listeners in profiler#23105
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
| } | ||
| /** | ||
| * @internal |
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.
I think it's OK to not atg this as internal (same below)
493097e to813ce0aCompare| * @paramTraceableFirewallListener|null$firewall | ||
| */ | ||
| publicfunction__construct(TokenStorageInterface$tokenStorage =null,RoleHierarchyInterface$roleHierarchy =null,LogoutUrlGenerator$logoutUrlGenerator =null,AccessDecisionManagerInterface$accessDecisionManager =null,FirewallMapInterface$firewallMap =null) | ||
| publicfunction__construct(TokenStorageInterface$tokenStorage =null,RoleHierarchyInterface$roleHierarchy =null,LogoutUrlGenerator$logoutUrlGenerator =null,AccessDecisionManagerInterface$accessDecisionManager =null,TraceableFirewallListener$firewall =null) |
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.
This is a BC break, right? What about removing the typehint for now with proper deprecation and BC support, and re-add it on 4.0?
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.
Seems like. We could also inject bothFirewallMap andTraceableFirewallListener distinctively, removing the getter added here. WDYT?
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.
Looks good to me to inject both and remove the getter, yes.
| publicfunctiongetInfo() | ||
| { | ||
| $pretty =get_class($this->listener); |
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.
- $pretty+ $class
? (I spotted this comes fromSymfony\Component\EventDispatcher\Debug\WrappedListener, but there isn't the same treatment here. Let's be more explicit). Do you even need it ?
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.
I started suffixing by::handle() for finally removing it, ok for$class
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.
I don't need it :) good catch
48f5e96 to1f36aa8Comparechalasr commentedJun 9, 2017
Comments addressed. Build failure on deps=high is normal because the security component is updated here. |
| <argumenttype="service"id="security.logout_url_generator" /> | ||
| <argumenttype="service"id="security.access.decision_manager" /> | ||
| <argumenttype="service"id="security.firewall.map" /> | ||
| <argumenttype="service"id="security.firewall" /> |
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.
If you don't inject explicitlydebug.security.firewall here, you can't typehintTraceableFirewallListener, but should add aninstanceof check in the collector, otherwise it'll break in non debug mode when theTraceableFirewallListener does not decoratessecurity.firewall (I still don't get why we would have collectors in non-debug mode though...).
IMHO, it's better to keep your collector as you did, but injectdebug.security.firewall withon-invalid="null" here.
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.
updated to injectdebug.security.firewall or null as suggested
b549295 to6065367Comparechalasr commentedJun 11, 2017
This is ready. |
f4be835 toaef1b6fCompare| * | ||
| * @author Robin Chalas <robin.chalas@gmail.com> | ||
| * | ||
| * @final since version 3.4 |
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.
Why are you using the tag here instead of marking the class as being final directly?
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.
To ease maintenance while keeping extensibility: one could add more info per listener and if a day we don't need this method for dumping the object, the breaking change would be free
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.
I don't get it. This class could even be marked as being internal. I would definitely mark it as final.
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.
done
| * | ||
| * @author Robin Chalas <robin.chalas@gmail.com> | ||
| * | ||
| * @final since version 3.4 |
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.
same here
| */ | ||
| class WrappedListenerimplements ListenerInterface | ||
| { | ||
| public$response; |
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.
Why is it public?
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.
It is not anymore
| <argumenttype="service"id="debug.security.access.decision_manager.inner" /> | ||
| </service> | ||
| <serviceid="debug.security.firewall"class="Symfony\Bundle\SecurityBundle\Debug\TraceableFirewallListener"decorates="security.firewall"public="true"> |
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.
this service should not be public. There is no reason to do a `$container->get('debug.security.firewall')
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.
done but that means the bundle needs a conflict for the dispatcher as event subscribers must be public before 3.3
| <argumenttype="service"id="debug.security.access.decision_manager.inner" /> | ||
| </service> | ||
| <serviceid="debug.security.firewall"class="Symfony\Bundle\SecurityBundle\Debug\TraceableFirewallListener"decorates="security.firewall"public="true"> |
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.
Using a decorator is weird here, as it does not perform decoration actually (it never uses the inner service).
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.
fixed
c72a273 to3ff46f1Comparechalasr commentedJun 13, 2017
rebased |
fabpot commentedJun 13, 2017
Thank you@chalasr. |
…rity listeners in profiler (chalasr)This PR was merged into the 3.4 branch.Discussion----------[SecurityBundle][Profiler] Give info about called security listeners in profiler| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#11134| License | MIT| Doc PR | n/aCurrently the profiler gives no info about security listeners (see fixed ticket), this displays each called listener with the time spent at calling it and its response if any.Commits-------369f19f Give info about called security listeners in profiler
Currently the profiler gives no info about security listeners (see fixed ticket), this displays each called listener with the time spent at calling it and its response if any.