Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Bridge][Twig] Decouple the SecurityExtension from its runtime#24692
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
| $this->securityChecker =$securityChecker; | ||
| } | ||
| publicfunctionisGranted($role,$object =null,$field =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, it has to be deprecated first.
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.
Even though it's never called directly only through the twig function?
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.
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.
Anyone extending this class (e.g. to replace the security extension by a custom one relying on these methods) would be broken. IMO it's not acceptable.
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 can fix BC break if it really is considered a BC break. I don't want that to be a blocker.
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.
IMHO this is acceptable, for the reason stated: i.e.812fbb4 … namely, this is moving what always should have been to where it, um, should have been … calling it a BC break versus history is just bikeshedding
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.
Well, it was a BC break, we just accepted it (and merged as a feature, not a bugfix).
There is a lot of classes in the codebase that should not be used as extension points, and we don't change their public API from minor to minor, per semver and our BC promise.
I would flag it as@final (same for other extensions) and stop injecting the authorization checker in the extension service for 4.1, then remove its methods in 5.0. At least the class would remain as is for 4.x.
Let's see what others think
| $this->securityChecker =$securityChecker; | ||
| } | ||
| publicfunctionisGranted($role,$object =null,$field =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.
needs a docblock with@deprecated
CarsonF commentedJul 10, 2018
Due to Silex being EoL I'm closing this. |
Uh oh!
There was an error while loading.Please reload this page.
This indirectly fixes two bugs in Silex regarding Security and Twig.
The first is if
twigis loaded beforeboot()some services/parameters are not defined.twigasks forsecurity.authorization_checkersecurity.authorization_checkerasks forsecurity.authentication_managersecurity.authentication_managerasks forsecurity.authentication_providerssecurity.authentication_providersare not completely defined untilsecurity.firewall_mapis loaded (here).I fixed this locally by wrapping the
security.authorization_checkerservice and loading thesecurity.firewall_mapbefore hand:This technically fixed the problem, but it wouldn't be needed if the service wasn't loaded until
boot()where the firewall is loaded by default.The second problem is a circular dependency. I was implementing an AccessDeniedHandler that rendered a template.
security.firewall_mapasks for AccessDeniedHandler servicetwigtwigasks forsecurity.firewall_map....:boom:And again this wouldn't be a problem if
twigdidn't need theSecurityRuntimeuntil it'sis_grantedis called.I don't see this as afeature since it only modifies private services. And Ido see this as abugfix even though it's for Silex and not Symfony. But I know it's a gray area. Twig Runtimes seem to be the way we are heading so I don't think this is a step in the wrong direction either.
I have a PR ready for Silex depending on the direction of this issue. Here's thediff.