[TwigBundle] Add loader priority #12174
Conversation
| return 0; | ||
| } | ||
| return ($a[0] > $b[0]) ? -1 : 1; | ||
| }); |
There was a problem hiding this comment.
this can be much faster by using nested arrays instead, as done in other places:
$prioritizedLoaders = array();
foreach ($loaderIds as $id => $tags) {
foreach($tags as $tag) {
$priority = isset($attribute['priority']) ? $attribute['priority'] : 0;
$prioritizedLoaders[$priority][] = $id;
}
}
ksort($prioritizedLoaders);
foreach ($prioritizedLoaders as $loaders) {
foreach ($loaders as $loader) {
$chainLoader->addMethodCall('addLoader', array(new Reference($loader)));
}
}There was a problem hiding this comment.
note that my own logic supports having multiple tags on the same service properly instead of ignoring all tags except the first one defining a priority (even though registering the same loader multiple times does not make much sense)
There was a problem hiding this comment.
is ksort correct here? Higher priority needs to be first.
|
Shouldn't it be better to add the support of the priorities to the ChainLoader? (in this case it's up to the Twig project and you should create a ticket in fabpot/twig) |
|
@Nicofuma putting this in the ChainLoader would mean doing the sorting at runtime, which has an impact on perf. IMO, it is perfectly fine for Twig to assume that you register the loaders in the order which matters for you. |
|
I see the point, I was thinking to something similar to the EventDispatcher where the overhead of the priorities is very low. |
|
Well, the priority at runtime are logical in the dispatcher, as the registration of event listeners is not expected to happen in a central place, even when not using it in a DI context. For instance, for forms, each form type in the hierarchy can add listeners, and they might want to run either before or after the listener added by the parent type in the hierarchy. |
|
Thank you @wizhippo. |
|
@wizhippo Can you submit a PR on symfony/symfony-docs to document this new feature? Thanks. |
This PR was submitted for the master branch but it was merged into the 2.7 branch instead (closes #12174). Discussion ---------- [TwigBundle] Add loader priority | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Add the ability to specify a priority to the tag of `twig.loader` services. eg: ``` <service id="twig.loader.filesystem" class="%twig.loader.filesystem.class%" public="false"> <argument type="service" id="templating.locator" /> <argument type="service" id="templating.name_parser" /> <tag name="twig.loader" priority="100"/> </service> ``` Commits ------- 67dffea Add Twig loader priority
Add the ability to specify a priority to the tag of
twig.loaderservices.eg: