8000 [TwigBundle] Added priority to twig extensions by Brunty · Pull Request #24777 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[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.

Sign up for GitHub

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

Closed
wants to merge 12 commits into from
Closed

[TwigBundle] Added priority to twig extensions #24777

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9658556
[TwigBundle] Added priority to twig extensions
Brunty Oct 31, 2017
65cc813
[TwigBundle] Refactor to PriorityTaggedServiceTrait
Brunty Nov 1, 2017
0d77f49
[TwigBundle] Refactor extension variable as already a reference
Brunty Nov 1, 2017
54d30b1
[TwigBundle] Remove unused use statement
Brunty Nov 2, 2017
3d5a545
[TwigBundle] Add changelog entry for priority on extensions
Brunty Nov 18, 2017
1cf78d2
[TwigBundle] Move changelog entry to 4.1.0
Brunty Dec 29, 2017
a22d713
[TwigBundle] Fix capitalisation in changelog
Brunty Jan 2, 2018
65f4e89
[TwigBundle] Remove mock container builder in test
Brunty Jan 2, 2018
a00ffad
[TwigBundle] Fix code style errors
Brunty Jan 2, 2018
ff 8000 2aba2
[TwigBundle] Update changelog for newline formatting
Brunty Jan 16, 2018
9e8d1f5
[TwigBundle] Remove TwigEngironmentPassTest setUp method
Brunty Jan 16, 2018
5399083
[TwigBundle] Fix code style in TwigEnvironmentPassTest
Brunty Jan 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[TwigBundle] Added priority to twig extensions
  • Loading branch information
@Brunty
Brunty committed Jan 16, 2018
commit 96585561a9e91606119cfb1e65905ff5fd4540a0
14 changes: 12 additions & 2 deletions src/Symfony/Bundle/TwigBundle/DependencyInjection/Compiler/TwigEnvironmentPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,26 @@ public function process(ContainerBuilder $container)
return;
}

$prioritizedLoaders = array();

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

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 the PriorityTaggedServiceTrait?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iltar done :)


$definition = $container->getDefinition('twig');
krsort($prioritizedLoaders);

// Extensions must always be registered before everything else.
// For instance, global variable definitions must be registered
// afterward. If not, the globals from the extensions will never
// be registered.
$calls = $definition->getMethodCalls();
$definition->setMethodCalls(array());
foreach ($container->findTaggedServiceIds('twig.extension', true) as $id => $attributes) {
$definition->addMethodCall('addExtension', array(new Reference($id)));
foreach ($prioritizedLoaders as $loaders) {
foreach ($loaders as $loader) {
$definition->addMethodCall('addExtension', array(new Reference($loader)));
}
}
$definition->setMethodCalls(array_merge($definition->getMethodCalls(), $calls));
}
Expand Down
72 changes: 72 additions & 0 deletions src/Symfony/Bundle/TwigBundle/Tests/DependencyInjection/Compiler/TwigEnvironmentPassTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Bundle\TwigBundle\Tests\DependencyInjection\Compiler;

use PHPUnit\Framework\TestCase;
use Symfony\Bundle\TwigBundle\DependencyInjection\Compiler\TwigEnvironmentPass;
use Symfony\Component\DependencyInjection\Definition;

class TwigEnvironmentPassTest extends TestCase
{
/**
* @var \PHPUnit_Framework_MockObject_MockObject
Copy link
Contributor
@ro0NL ro0NL Nov 18, 2017 •
edited
Loading

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.

Copy link
Member

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.

*/
private $builder;
/**
* @var Definition
*/
private $definition;
/**
* @var TwigEnvironmentPass
*/
private $pass;

protected function setUp()
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done :)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not mock the ContainerBuilder class

Copy link
Contributor Author

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please do so :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xabbuh done :)

$this->definition = new Definition('twig');
$this->pass = new TwigEnvironmentPass();
}

public function testPassWithTwoExtensionsWithPriority()
{
$serviceIds = array(
'test_extension_1' => array(
array('priority' => 100),
),
'test_extension_2' => array(
array('priority' => 200),
),
);

$this->builder->expects($this->once())
->method('hasDefinition')
->with('twig')
->will($this->returnValue(true));
$this->builder->expects($this->once())
->method('findTaggedServiceIds')
->with('twig.extension')
->will($this->returnValue($serviceIds));
$this->builder->expects($this->once())
->method('getDefinition')
->with('twig')
->will($this->returnValue($this->definition));

$this->pass->process($this->builder);
$calls = $this->definition->getMethodCalls();
$this->assertCount(2, $calls);
$this->assertEquals('addExtension', $calls[0][0]);
$this->assertEquals('addExtension', $calls[1][0]);
$this->assertEquals('test_extension_2', (string) $calls[0][1][0]);
$this->assertEquals('test_extension_1', (string) $calls[1][1][0]);
}
}
0