[SecurityBundle] fix allow_if expression service compilation to support custom functions #24309
Conversation
|
@chalasr did you have a chance yet to look at this? Feedback welcome 😊 |
|
Can't we just move the serialisation of the expression language to a compiler pass that is processed after the |
|
@xabbuh this seems to be working indeed 😊 👍 I will update the PR later today |
| foreach ($container->findTaggedServiceIds('security.expression_language_provider', true) as $id => $attributes) { | ||
| $definition->addMethodCall('addExpressionLanguageProvider', array(new Reference($id))); | ||
| $definition->addMethodCall('registerProvider', array(new Reference($id))); |
There was a problem hiding this comment.
To me this makes more sense than to "take a detour" via the ExpressionVoter to extend the ExpressionLanguage service.
Should not break BC?
|
@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 😢 |
|
Removing |
|
You mean doing that only for 2.7 and 2.8? Seems a bit wrong though to put that into |
|
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. |
|
@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.
I don't really like this tag name. Can't we just use something like security.expression?
There was a problem hiding this comment.
yeah why not 😊 Done 👍
| return; | ||
| } | ||
|
|
||
| $expressionLanguage = $container->get('security.expression_language'); |
There was a problem hiding this comment.
No this should actually get the instantiated service 😉
There was a problem hiding this comment.
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.
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 an ArrayAdapter:
On 3.3+ we could use the system cache pool for it?
And then we could add a CacheWarmer for it.
Or do you have another idea?
There was a problem hiding this comment.
Indeed, I had something like that in mind.
There was a problem hiding this comment.
I will have a look and maybe also prepare some benchmarks to see the impact of the change
| $expressionLanguage = $container->get('security.expression_language'); | ||
|
|
||
| foreach ($container->findTaggedServiceIds('security.expression') as $id => $attributes) { | ||
| $defition = $container->getDefinition($id); |
|
will work on a different solution within a separate PR |
I would like to discuss possibilities to fix #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.