Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[TwigBundle] Added priority to twig extensions#24777
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
| foreach ($container->findTaggedServiceIds('twig.extension',true)as$id =>$attributes) { | ||
| $priority =isset($attributes[0]['priority']) ?$attributes[0]['priority'] :0; | ||
| $prioritizedLoaders[$priority][] =$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.
Could this be replaced by using thePriorityTaggedServiceTrait?
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.
@iltar done :)
fabpot commentedNov 1, 2017
Can you explain the use case for having priorities on extensions? I do understand the need for loaders, but I fail to see why we would need them for extensions. |
Brunty commentedNov 1, 2017
@fabpot I was recently working on something and needed to overload a filter - I was lucky in that where I was registering it, it was after the previous one being registered (so as per docs here:https://twig.symfony.com/doc/2.x/advanced.html#overloading) but figured that might not always be the case. |
| foreach ($container->findTaggedServiceIds('twig.extension',true)as$id =>$attributes) { | ||
| $definition->addMethodCall('addExtension',array(newReference($id))); | ||
| foreach ($this->findAndSortTaggedServices('twig.extension',$container)as$extension) { | ||
| $definition->addMethodCall('addExtension',array(newReference($extension))); |
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.
$extension is already a reference so that this code could be simplified
ro0NL left a comment
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 CHANGELOG entry i guess.
| class TwigEnvironmentPassTestextends TestCase | ||
| { | ||
| /** | ||
| * @var \PHPUnit_Framework_MockObject_MockObject |
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.
for clarity i prefer/** @var Type */ thus inline. But not a blocker :) and im not sure we do that elsewhere anyway.
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.
AFAIK, we don't do much "inline" on properties.
Brunty commentedNov 18, 2017
@ro0NL will add a changelog entry now, what version should it go under? |
ro0NL commentedNov 18, 2017
4.0.0 but there's a 4.4.0 entry already, i think it's a typo :) |
Brunty commentedNov 18, 2017
@ro0NL I'll add a 4.0.0 section, want me to correct the 4.4.0 one to 4.0.0? |
ro0NL commentedNov 18, 2017
think so. affects master only anyway. |
Brunty commentedNov 18, 2017
@ro0NL - added changelog entry for you |
| ----- | ||
| * removed`ContainerAwareRuntimeLoader` | ||
| * added priority to twig extensions |
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.
should be added in a new entry for 4.1.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.
@nicolas-grekas done :)
| 4.4.0 | ||
| 4.1.0 | ||
| ----- | ||
| * added priority to twig extensions |
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.
Twig
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.
Changed
| protectedfunctionsetUp() | ||
| { | ||
| $this->builder =$this->getMockBuilder('Symfony\Component\DependencyInjection\ContainerBuilder')->setMethods(array('hasDefinition','findTaggedServiceIds','setAlias','getDefinition'))->getMock(); |
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 would not mock theContainerBuilder 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 based it on this test:https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/TwigBundle/Tests/DependencyInjection/Compiler/TwigLoaderPassTest.php#L36
I can change it to use the actual builder if you'd like?
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.
please do so :)
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.
@xabbuh done :)
| 4.1.0 | ||
| ----- | ||
| * added priority to Twig extensions |
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.
missing blank line before
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.
Added :)
| */ | ||
| private$pass; | ||
| protectedfunctionsetUp() |
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.
we don't need this method IMHO, better remove it and the corresponding properties
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 :)
Moves the properties on the test class to variables inside the single test method it has.
fabpot commentedJan 17, 2018
Thank you@Brunty. |
Added priority to twig extensions in the
TwigEnvironmentPassto control the order in which they're registered, similar to theTwigLoaderPassThough, unsure on what priority to use as a default, and will PR docs when finalised.