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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TwigBundle] Added priority to twig extensions #24777

Closed
wants to merge 12 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@Brunty
Contributor

Brunty commented Oct 31, 2017

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR Don't merge before the docs

Added priority to twig extensions in the TwigEnvironmentPass to control the order in which they're registered, similar to the TwigLoaderPass

Though, unsure on what priority to use as a default, and will PR docs when finalised.

foreach ($container->findTaggedServiceIds('twig.extension', true) as $id => $attributes) {
$priority = isset($attributes[0]['priority']) ? $attributes[0]['priority'] : 0;
$prioritizedLoaders[$priority][] = $id;
}

This comment has been minimized.

@iltar

iltar Nov 1, 2017

Contributor

Could this be replaced by using the PriorityTaggedServiceTrait?

This comment has been minimized.

@Brunty

Brunty Nov 1, 2017

Contributor

@iltar done :)

@fabpot

This comment has been minimized.

Member

fabpot commented Nov 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

This comment has been minimized.

Contributor

Brunty commented Nov 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.

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Nov 1, 2017

foreach ($container->findTaggedServiceIds('twig.extension', true) as $id => $attributes) {
$definition->addMethodCall('addExtension', array(new Reference($id)));
foreach ($this->findAndSortTaggedServices('twig.extension', $container) as $extension) {
$definition->addMethodCall('addExtension', array(new Reference($extension)));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 1, 2017

Member

$extension is already a reference so that this code could be simplified

@ro0NL

ro0NL approved these changes Nov 18, 2017

needs a CHANGELOG entry i guess.

class TwigEnvironmentPassTest extends TestCase
{
/**
* @var \PHPUnit_Framework_MockObject_MockObject

This comment has been minimized.

@ro0NL

ro0NL Nov 18, 2017

Contributor

for clarity i prefer /** @var Type */ thus inline. But not a blocker :) and im not sure we do that elsewhere anyway.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 29, 2017

Member

AFAIK, we don't do much "inline" on properties.

@Brunty

This comment has been minimized.

Contributor

Brunty commented Nov 18, 2017

@ro0NL will add a changelog entry now, what version should it go under?

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Nov 18, 2017

4.0.0 but there's a 4.4.0 entry already, i think it's a typo :)

@Brunty

This comment has been minimized.

Contributor

Brunty commented Nov 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

This comment has been minimized.

Contributor

ro0NL commented Nov 18, 2017

think so. affects master only anyway.

@Brunty

This comment has been minimized.

Contributor

Brunty commented Nov 18, 2017

@ro0NL - added changelog entry for you

-----
* removed `ContainerAwareRuntimeLoader`
* added priority to twig extensions

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 29, 2017

Member

should be added in a new entry for 4.1.0

This comment has been minimized.

@Brunty
4.4.0
4.1.0
-----
* added priority to twig extensions

This comment has been minimized.

@xabbuh

xabbuh Jan 1, 2018

Member

Twig

This comment has been minimized.

@Brunty

Brunty Jan 2, 2018

Contributor

Changed

protected function setUp()
{
$this->builder = $this->getMockBuilder('Symfony\Component\DependencyInjection\ContainerBuilder')->setMethods(array('hasDefinition', 'findTaggedServiceIds', 'setAlias', 'getDefinition'))->getMock();

This comment has been minimized.

@xabbuh

xabbuh Jan 1, 2018

Member

I would not mock the ContainerBuilder class

This comment has been minimized.

@Brunty

This comment has been minimized.

@xabbuh

xabbuh Jan 2, 2018

Member

please do so :)

This comment has been minimized.

@Brunty

Brunty Jan 2, 2018

Contributor

@xabbuh done :)

@@ -1,6 +1,10 @@
CHANGELOG
=========
4.1.0
-----
* added priority to Twig extensions

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 16, 2018

Member

missing blank line before

This comment has been minimized.

@Brunty

Brunty Jan 16, 2018

Contributor

Added :)

*/
private $pass;
protected function setUp()

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 16, 2018

Member

we don't need this method IMHO, better remove it and the corresponding properties

This comment has been minimized.

@Brunty

Brunty Jan 16, 2018

Contributor

Done :)

Brunty added some commits Oct 31, 2017

Brunty added some commits Jan 2, 2018

[TwigBundle] Remove TwigEngironmentPassTest setUp method
Moves the properties on the test class to variables inside the single test method it has.
@xabbuh

xabbuh approved these changes Jan 17, 2018

@fabpot

fabpot approved these changes Jan 17, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Jan 17, 2018

Thank you @Brunty.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment