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] fix allow_if expression service compilation to support custom functions#24309
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
dmaicher commentedOct 2, 2017
@chalasr did you have a chance yet to look at this? Feedback welcome 😊 |
chalasr commentedOct 2, 2017
xabbuh commentedOct 11, 2017
Can't we just move the serialisation of the expression language to a compiler pass that is processed after the |
dmaicher commentedOct 11, 2017
@xabbuh this seems to be working indeed 😊 👍 I will update the PR later today |
| $definition =$container->findDefinition('security.expression_language'); | ||
| foreach ($container->findTaggedServiceIds('security.expression_language_provider',true)as$id =>$attributes) { | ||
| $definition->addMethodCall('addExpressionLanguageProvider',array(newReference($id))); | ||
| $definition->addMethodCall('registerProvider',array(newReference($id))); |
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 me this makes more sense than to "take a detour" via the ExpressionVoter to extend the ExpressionLanguage service.
Should not break BC?
dmaicher commentedOct 11, 2017
@xabbuh before I will fix the tests + add more tests: Is this what you meant? Also I had to change the base to 3.3 as we cannot set priorities for compiler passes before 😢 |
xabbuh commentedOct 11, 2017
Removing |
dmaicher commentedOct 12, 2017
You mean doing that only for 2.7 and 2.8? Seems a bit wrong though to put that into |
xabbuh commentedOct 13, 2017
Alternatively, we can add the new compiler pass to the FrameworkBundle instead. This shouldn't be a big problem as we already have the handling for registering custom provider there. |
dmaicher commentedOct 13, 2017
@xabbuh indeed that seems better. Done 😉 |
| ->setPublic(false) | ||
| ->addArgument($expression) | ||
| ->addArgument(serialize($this->getExpressionLanguage()->parse($expression,array('token','user','object','roles','request','trust_resolver'))->getNodes())) | ||
| ->addTag('security.expression.unparsed') |
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 really like this tag name. Can't we just use something likesecurity.expression?
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.
yeah why not 😊 Done 👍
| return; | ||
| } | ||
| $expressionLanguage =$container->get('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.
findDefinition()
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.
No this should actually get the instantiated 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.
Oh hm, I missed that. To be honest I am not really happy with having compiler passes that force the creation of the actual service during compilation. Can't we solve the problem by letting the serialization happening in a cache warmer where the service is actually available?
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 see your point. I'm also not 100% happy with this solution.
The only other option I see is to use a proper persisting cache by default for the expression language service. Currently it only uses anArrayAdapter:
On 3.3+ we could use the system cache pool for it?
And then we could add aCacheWarmer for it.
Or do you have another idea?
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.
Indeed, I had something like that in mind.
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 will have a look and maybe also prepare some benchmarks to see the impact of the change
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.
| $expressionLanguage =$container->get('security.expression_language'); | ||
| foreach ($container->findTaggedServiceIds('security.expression')as$id =>$attributes) { | ||
| $defition =$container->getDefinition($id); |
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.
typo:$definition
dmaicher commentedNov 8, 2017
will work on a different solution within a separate PR |
Uh oh!
There was an error while loading.Please reload this page.
I would like to discuss possibilities tofix#23208 (also see closed duplicate#24306).
The proposed solution in this PR is to simply not dump the parsed expression but simply the "raw" expressionsQuestion: Will this have a big negative performance impact? As far as I understand theExpressionLanguageshould have a cache anyway for parsed expressions? Or not?Idea: maybe we can add an additionalCacheWarmerthat warms the cache for all those expressions?Or is there a way to perform the compilation into parsed expressions at the very end of the container compilation when we could instantiate the actualExpressionLanguageservice with all its custom extensions?Edit: the new approach is to still dump the parsed expressions but we do it using the "real" expression language service with all custom extensions.