merged branch fabpot/env-simplifications (PR #904)

This PR was merged into the master branch.

Commits
-------

0a7b37b changed the way extension filters/tests/functions/node visitors/globals/token parsers are registered (they were loaded as late as possible, they are now loaded as early as possible)

Discussion
----------

simplified some code

This PR changes the way extension filters/tests/functions/node visitors/globals/token parsers are registered (they were loaded as late as possible, they are now loaded as early as possible)

It mainly consists of a code cleanup by removing the horrible staging hack we have now.

CHANGELOG

@@ -1,5 +1,6 @@
 * 1.12.0 (2012-XX-XX)
 
+ * changed the way extension filters/tests/functions/node visitors/globals/token parsers are registered (they were loaded as late as possible, they are now loaded as early as possible)
  * added the ability to set default values for macro arguments
  * added support for named arguments for filters, tests, and functions
  * moved filters/functions/tests syntax errors to the parser

doc/api.rst

@@ -317,10 +317,7 @@ Twig comes bundled with the following extensions:
 * *Twig_Extension_Optimizer*: Optimizers the node tree before compilation.
 
 The core, escaper, and optimizer extensions do not need to be added to the
-Twig environment, as they are registered by default. You can disable an
-already registered extension::
-
-    $twig->removeExtension('escaper');
+Twig environment, as they are registered by default.
 
 Built-in Extensions
 -------------------

doc/deprecated.rst

@@ -12,3 +12,9 @@ Token Parsers
 
   * ``Twig_TokenParserBrokerInterface``
   * ``Twig_TokenParserBroker``
+
+Extensions
+----------
+
+* The ability to remove an extension is deprecated and the
+  ``Twig_Environment::removeExtension()`` method will be removed in 2.0.

lib/Twig/Environment.php

@@ -102,24 +102,23 @@ class Twig_Environment
         $this->charset            = $options['charset'];
         $this->baseTemplateClass  = $options['base_template_class'];
         $this->autoReload         = null === $options['auto_reload'] ? $this->debug : (bool) $options['auto_reload'];
-        $this->extensions         = array(
-            'core'      => new Twig_Extension_Core(),
-            'escaper'   => new Twig_Extension_Escaper($options['autoescape']),
-            'optimizer' => new Twig_Extension_Optimizer($options['optimizations']),
-        );
         $this->strictVariables    = (bool) $options['strict_variables'];
         $this->runtimeInitialized = false;
         $this->setCache($options['cache']);
         $this->functionCallbacks = array();
         $this->filterCallbacks = array();
-        $this->staging = array(
-            'functions'     => array(),
-            'filters'       => array(),
-            'tests'         => array(),
-            'token_parsers' => array(),
-            'visitors'      => array(),
-            'globals'       => array(),
-        );
+        $this->parsers = new Twig_TokenParserBroker();
+        $this->filters = array();
+        $this->functions = array();
+        $this->tests = array();
+        $this->globals = array();
+        $this->visitors = array();
+        $this->unaryOperators = array();
+        $this->binaryOperators = array();
+
+        $this->addExtension(new Twig_Extension_Core());
+        $this->addExtension(new Twig_Extension_Escaper($options['autoescape']));
+        $this->addExtension(new Twig_Extension_Optimizer($options['optimizations']));
     }
 
     /**
@@ -629,29 +628,140 @@ class Twig_Environment
      */
     public function addExtension(Twig_ExtensionInterface $extension)
     {
-        $this->extensions[$extension->getName()] = $extension;
-        $this->parsers = null;
-        $this->visitors = null;
-        $this->filters = null;
-        $this->tests = null;
-        $this->functions = null;
-        $this->globals = null;
+        $key = $extension->getName();
+        $this->extensions[$key] = $extension;
+        $this->staging[$key] = array(
+            'filters'       => (array) $extension->getFilters(),
+            'functions'     => (array) $extension->getFunctions(),
+            'tests'         => (array) $extension->getTests(),
+            'globals'       => (array) $extension->getGlobals(),
+            'token_parsers' => (array) $extension->getTokenParsers(),
+            'node_visitors' => (array) $extension->getNodeVisitors(),
+            'operators'     => (array) $extension->getOperators(),
+        );
+
+        // filters
+        foreach ($this->staging[$key]['filters'] as $name => $filter) {
+            $this->addFilter($name, $filter);
+        }
+
+        // functions
+        foreach ($this->staging[$key]['functions'] as $name => $function) {
+            $this->addFunction($name, $function);
+        }
+
+        // tests
+        foreach ($this->staging[$key]['tests'] as $name => $test) {
+            $this->addTest($name, $test);
+        }
+
+        // globals
+        $this->globals = array_merge($this->globals, $this->staging[$key]['globals']);
+
+        // token parsers
+        foreach ($this->staging[$key]['token_parsers'] as $parser) {
+            if ($parser instanceof Twig_TokenParserInterface) {
+                $this->parsers->addTokenParser($parser);
+            } elseif ($parser instanceof Twig_TokenParserBrokerInterface) {
+                $this->parsers->addTokenParserBroker($parser);
+            } else {
+                throw new LogicException('getTokenParsers() must return an array of Twig_TokenParserInterface or Twig_TokenParserBrokerInterface instances');
+            }
+        }
+
+        // node visitors
+        foreach ($this->staging[$key]['node_visitors'] as $visitor) {
+            $this->addNodeVisitor($visitor);
+        }
+
+        // operators
+        if ($operators = $this->staging[$key]['operators']) {
+            if (2 !== count($operators)) {
+                throw new InvalidArgumentException(sprintf('"%s::getOperators()" does not return a valid operators array.', get_class($extension)));
+            }
+
+            $this->unaryOperators = array_merge($this->unaryOperators, $operators[0]);
+            $this->binaryOperators = array_merge($this->binaryOperators, $operators[1]);
+        }
     }
 
     /**
      * Removes an extension by name.
      *
+     * This method is deprecated and you should not use it.
+     *
+     * This method won't work properly when extension B is removed and
+     * had overriden something registered by extension A.
+     *
      * @param string $name The extension name
+     *
+     * @deprecated
      */
     public function removeExtension($name)
     {
+        if (!isset($this->extensions[$name])) {
+            return;
+        }
+
+        $extension = $this->staging[$name];
+
+        // filters
+        foreach ($extension['filters'] as $name => $filter) {
+            if (isset($this->filters[$name]) && $filter === $this->filters[$name]) {
+                unset($this->filters[$name]);
+            }
+        }
+
+        // functions
+        foreach ($extension['functions'] as $name => $function) {
+            if (isset($this->functions[$name]) && $function === $this->functions[$name]) {
+                unset($this->functions[$name]);
+            }
+        }
+
+        // tests
+        foreach ($extension['tests'] as $name => $test) {
+            if (isset($this->tests[$name]) && $test === $this->tests[$name]) {
+                unset($this->tests[$name]);
+            }
+        }
+
+        // globals
+        foreach ($extension['globals'] as $key => $value) {
+            if (isset($this->globals[$key]) && $value === $this->globals[$key]) {
+                unset($this->globals[$key]);
+            }
+        }
+
+        // token parsers
+        foreach ($extension['token_parsers'] as $parser) {
+            if ($parser instanceof Twig_TokenParserInterface) {
+                $this->parsers->removeTokenParser($parser);
+            } elseif ($parser instanceof Twig_TokenParserBrokerInterface) {
+                $this->parsers->removeTokenParserBroker($parser);
+            }
+        }
+
+        // node visitors
+        foreach ($extension['node_visitors'] as $visitor) {
+            if (false !== $pos = array_search($visitor, $this->visitors)) {
+                unset($this->visitors[$pos]);
+            }
+        }
+
+        // operators
+        if ($extension['operators']) {
+            foreach (array_keys($extension['operators'][0]) as $key) {
+                unset($this->unaryOperators[$key]);
+            }
+
+            foreach (array_keys($extension['operators'][1]) as $key) {
+                unset($this->binaryOperators[$key]);
+            }
+        }
+
         unset($this->extensions[$name]);
-        $this->parsers = null;
-        $this->visitors = null;
-        $this->filters = null;
-        $this->tests = null;
-        $this->functions = null;
-        $this->globals = null;
+        unset($this->staging[$name]);
     }
 
     /**
@@ -683,8 +793,7 @@ class Twig_Environment
      */
     public function addTokenParser(Twig_TokenParserInterface $parser)
     {
-        $this->staging['token_parsers'][] = $parser;
-        $this->parsers = null;
+        $this->parsers->addTokenParser($parser);
     }
 
     /**
@@ -694,29 +803,6 @@ class Twig_Environment
      */
     public function getTokenParsers()
     {
-        if (null === $this->parsers) {
-            $this->parsers = new Twig_TokenParserBroker();
-
-            if (isset($this->staging['token_parsers'])) {
-                foreach ($this->staging['token_parsers'] as $parser) {
-                    $this->parsers->addTokenParser($parser);
-                }
-            }
-
-            foreach ($this->getExtensions() as $extension) {
-                $parsers = $extension->getTokenParsers();
-                foreach ($parsers as $parser) {
-                    if ($parser instanceof Twig_TokenParserInterface) {
-                        $this->parsers->addTokenParser($parser);
-                    } elseif ($parser instanceof Twig_TokenParserBrokerInterface) {
-                        $this->parsers->addTokenParserBroker($parser);
-                    } else {
-                        throw new LogicException('getTokenParsers() must return an array of Twig_TokenParserInterface or Twig_TokenParserBrokerInterface instances');
-                    }
-                }
-            }
-        }
-
         return $this->parsers;
     }
 
@@ -746,8 +832,7 @@ class Twig_Environment
      */
     public function addNodeVisitor(Twig_NodeVisitorInterface $visitor)
     {
-        $this->staging['visitors'][] = $visitor;
-        $this->visitors = null;
+        $this->visitors[] = $visitor;
     }
 
     /**
@@ -757,16 +842,6 @@ class Twig_Environment
      */
     public function getNodeVisitors()
     {
-        if (null === $this->visitors) {
-            foreach ($this->getExtensions() as $extension) {
-                foreach ($extension->getNodeVisitors() as $visitor) {
-                    $this->addNodeVisitor($visitor);
-                }
-            }
-
-            $this->visitors = $this->staging['visitors'];
-        }
-
         return $this->visitors;
     }
 
@@ -778,8 +853,7 @@ class Twig_Environment
      */
     public function addFilter($name, Twig_FilterInterface $filter)
     {
-        $this->staging['filters'][$name] = $filter;
-        $this->filters = null;
+        $this->filters[$name] = $filter;
     }
 
     /**
@@ -794,10 +868,6 @@ class Twig_Environment
      */
     public function getFilter($name)
     {
-        if (null === $this->filters) {
-            $this->getFilters();
-        }
-
         if (isset($this->filters[$name])) {
             return $this->filters[$name];
         }
@@ -840,16 +910,6 @@ class Twig_Environment
      */
     public function getFilters()
     {
-        if (null === $this->filters) {
-            foreach ($this->getExtensions() as $extension) {
-                foreach ($extension->getFilters() as $name => $filter) {
-                    $this->addFilter($name, $filter);
-                }
-            }
-
-            $this->filters = $this->staging['filters'];
-        }
-
         return $this->filters;
     }
 
@@ -861,8 +921,7 @@ class Twig_Environment
      */
     public function addTest($name, Twig_TestInterface $test)
     {
-        $this->staging['tests'][$name] = $test;
-        $this->tests = null;
+        $this->tests[$name] = $test;
     }
 
     /**
@@ -872,16 +931,6 @@ class Twig_Environment
      */
     public function getTests()
     {
-        if (null === $this->tests) {
-            foreach ($this->getExtensions() as $extension) {
-                foreach ($extension->getTests() as $name => $test) {
-                    $this->addTest($name, $test);
-                }
-            }
-
-            $this->tests = $this->staging['tests'];
-        }
-
         return $this->tests;
     }
 
@@ -893,8 +942,7 @@ class Twig_Environment
      */
     public function addFunction($name, Twig_FunctionInterface $function)
     {
-        $this->staging['functions'][$name] = $function;
-        $this->functions = null;
+        $this->functions[$name] = $function;
     }
 
     /**
@@ -909,10 +957,6 @@ class Twig_Environment
      */
     public function getFunction($name)
     {
-        if (null === $this->functions) {
-            $this->getFunctions();
-        }
-
         if (isset($this->functions[$name])) {
             return $this->functions[$name];
         }
@@ -955,16 +999,6 @@ class Twig_Environment
      */
     public function getFunctions()
     {
-        if (null === $this->functions) {
-            foreach ($this->getExtensions() as $extension) {
-                foreach ($extension->getFunctions() as $name => $function) {
-                    $this->addFunction($name, $function);
-                }
-            }
-
-            $this->functions = $this->staging['functions'];
-        }
-
         return $this->functions;
     }
 
@@ -976,8 +1010,7 @@ class Twig_Environment
      */
     public function addGlobal($name, $value)
     {
-        $this->staging['globals'][$name] = $value;
-        $this->globals = null;
+        $this->globals[$name] = $value;
     }
 
     /**
@@ -987,13 +1020,6 @@ class Twig_Environment
      */
     public function getGlobals()
     {
-        if (null === $this->globals) {
-            $this->globals = isset($this->staging['globals']) ? $this->staging['globals'] : array();
-            foreach ($this->getExtensions() as $extension) {
-                $this->globals = array_merge($this->globals, $extension->getGlobals());
-            }
-        }
-
         return $this->globals;
     }
 
@@ -1024,10 +1050,6 @@ class Twig_Environment
      */
     public function getUnaryOperators()
     {
-        if (null === $this->unaryOperators) {
-            $this->initOperators();
-        }
-
         return $this->unaryOperators;
     }
 
@@ -1038,10 +1060,6 @@ class Twig_Environment
      */
     public function getBinaryOperators()
     {
-        if (null === $this->binaryOperators) {
-            $this->initOperators();
-        }
-
         return $this->binaryOperators;
     }
 
@@ -1059,26 +1077,6 @@ class Twig_Environment
         return array_keys($alternatives);
     }
 
-    protected function initOperators()
-    {
-        $this->unaryOperators = array();
-        $this->binaryOperators = array();
-        foreach ($this->getExtensions() as $extension) {
-            $operators = $extension->getOperators();
-
-            if (!$operators) {
-                continue;
-            }
-
-            if (2 !== count($operators)) {
-                throw new InvalidArgumentException(sprintf('"%s::getOperators()" does not return a valid operators array.', get_class($extension)));
-            }
-
-            $this->unaryOperators = array_merge($this->unaryOperators, $operators[0]);
-            $this->binaryOperators = array_merge($this->binaryOperators, $operators[1]);
-        }
-    }
-
     protected function writeCacheFile($file, $content)
     {
         $dir = dirname($file);

lib/Twig/TokenParserBroker.php

@@ -56,6 +56,19 @@ class Twig_TokenParserBroker implements Twig_TokenParserBrokerInterface
         $this->parsers[$parser->getTag()] = $parser;
     }
 
+    /**
+     * Removes a TokenParser.
+     *
+     * @param Twig_TokenParserInterface $parser A Twig_TokenParserInterface instance
+     */
+    public function removeTokenParser(Twig_TokenParserInterface $parser)
+    {
+        $name = $parser->getTag();
+        if (isset($this->parsers[$name]) && $parser === $this->parsers[$name]) {
+            unset($this->parsers[$name]);
+        }
+    }
+
     /**
      * Adds a TokenParserBroker.
      *
@@ -66,6 +79,18 @@ class Twig_TokenParserBroker implements Twig_TokenParserBrokerInterface
         $this->brokers[] = $broker;
     }
 
+    /**
+     * Removes a TokenParserBroker.
+     *
+     * @param Twig_TokenParserBroker $broker A Twig_TokenParserBroker instance
+     */
+    public function removeTokenParserBroker(Twig_TokenParserBroker $broker)
+    {
+        if (false !== $pos = array_search($broker, $this->brokers)) {
+            unset($this->brokers[$pos]);
+        }
+    }
+
     /**
      * Gets a suitable TokenParser for a tag.
      *

test/Twig/Tests/EnvironmentTest.php

@@ -32,4 +32,135 @@ class Twig_Tests_EnvironmentTest extends PHPUnit_Framework_TestCase
     {
         return $filename;
     }
+
+    public function testGlobalsAreSetEvenIfGetGlobalsIsCalledFirst()
+    {
+        $twig = new Twig_Environment(new Twig_Loader_String());
+
+        $globals = $twig->getGlobals();
+        $twig->addGlobal('foo', 'foo');
+        $globals = $twig->getGlobals();
+
+        $this->assertArrayHasKey('foo', $globals);
+    }
+
+    public function testAddExtension()
+    {
+        $twig = new Twig_Environment(new Twig_Loader_String());
+        $twig->addExtension(new Twig_Tests_EnvironmentTest_Extension());
+
+        $this->assertArrayHasKey('test', $twig->getTags());
+        $this->assertArrayHasKey('foo_filter', $twig->getFilters());
+        $this->assertArrayHasKey('foo_function', $twig->getFunctions());
+        $this->assertArrayHasKey('foo_test', $twig->getTests());
+        $this->assertArrayHasKey('foo_unary', $twig->getUnaryOperators());
+        $this->assertArrayHasKey('foo_binary', $twig->getBinaryOperators());
+        $this->assertArrayHasKey('foo_global', $twig->getGlobals());
+        $visitors = $twig->getNodeVisitors();
+        $this->assertEquals('Twig_Tests_EnvironmentTest_NodeVisitor', get_class($visitors[2]));
+    }
+
+    public function testRemoveExtension()
+    {
+        $twig = new Twig_Environment(new Twig_Loader_String());
+        $twig->addExtension(new Twig_Tests_EnvironmentTest_Extension());
+        $twig->removeExtension('test');
+
+        $this->assertFalse(array_key_exists('test', $twig->getTags()));
+        $this->assertFalse(array_key_exists('foo_filter', $twig->getFilters()));
+        $this->assertFalse(array_key_exists('foo_function', $twig->getFunctions()));
+        $this->assertFalse(array_key_exists('foo_test', $twig->getTests()));
+        $this->assertFalse(array_key_exists('foo_unary', $twig->getUnaryOperators()));
+        $this->assertFalse(array_key_exists('foo_binary', $twig->getBinaryOperators()));
+        $this->assertFalse(array_key_exists('foo_global', $twig->getGlobals()));
+        $this->assertCount(2, $twig->getNodeVisitors());
+    }
+}
+
+class Twig_Tests_EnvironmentTest_Extension extends Twig_Extension
+{
+    public function getTokenParsers()
+    {
+        return array(
+            new Twig_Tests_EnvironmentTest_TokenParser(),
+        );
+    }
+
+    public function getNodeVisitors()
+    {
+        return array(
+            new Twig_Tests_EnvironmentTest_NodeVisitor(),
+        );
+    }
+
+    public function getFilters()
+    {
+        return array(
+            'foo_filter' => new Twig_Filter_Function('foo_filter'),
+        );
+    }
+
+    public function getTests()
+    {
+        return array(
+            'foo_test' => new Twig_Test_Function('foo_test'),
+        );
+    }
+
+    public function getFunctions()
+    {
+        return array(
+            'foo_function' => new Twig_Function_Function('foo_function'),
+        );
+    }
+
+    public function getOperators()
+    {
+        return array(
+            array('foo_unary' => array()),
+            array('foo_binary' => array()),
+        );
+    }
+
+    public function getGlobals()
+    {
+        return array(
+            'foo_global' => 'foo_global',
+        );
+    }
+
+    public function getName()
+    {
+        return 'test';
+    }
+}
+
+class Twig_Tests_EnvironmentTest_TokenParser extends Twig_TokenParser
+{
+    public function parse(Twig_Token $token)
+    {
+    }
+
+    public function getTag()
+    {
+        return 'test';
+    }
+}
+
+class Twig_Tests_EnvironmentTest_NodeVisitor implements Twig_NodeVisitorInterface
+{
+    public function enterNode(Twig_NodeInterface $node, Twig_Environment $env)
+    {
+        return $node;
+    }
+
+    public function leaveNode(Twig_NodeInterface $node, Twig_Environment $env)
+    {
+        return $node;
+    }
+
+    public function getPriority()
+    {
+        return 0;
+    }
 }

test/Twig/Tests/NodeVisitor/OptimizerTest.php

@@ -13,7 +13,6 @@ class Twig_Tests_NodeVisitor_OptimizerTest extends PHPUnit_Framework_TestCase
     public function testRenderBlockOptimizer()
     {
         $env = new Twig_Environment(new Twig_Loader_String(), array('cache' => false, 'autoescape' => false));
-        $env->addExtension(new Twig_Extension_Optimizer());
 
         $stream = $env->parse($env->tokenize('{{ block("foo") }}', 'index'));
 
@@ -26,7 +25,6 @@ class Twig_Tests_NodeVisitor_OptimizerTest extends PHPUnit_Framework_TestCase
     public function testRenderParentBlockOptimizer()
     {
         $env = new Twig_Environment(new Twig_Loader_String(), array('cache' => false, 'autoescape' => false));
-        $env->addExtension(new Twig_Extension_Optimizer());
 
         $stream = $env->parse($env->tokenize('{% extends "foo" %}{% block content %}{{ parent() }}{% endblock %}', 'index'));
 
@@ -43,7 +41,6 @@ class Twig_Tests_NodeVisitor_OptimizerTest extends PHPUnit_Framework_TestCase
         }
 
         $env = new Twig_Environment(new Twig_Loader_String(), array('cache' => false, 'autoescape' => false));
-        $env->addExtension(new Twig_Extension_Optimizer());
         $stream = $env->parse($env->tokenize('{{ block(name|lower) }}', 'index'));
 
         $node = $stream->getNode('body')->getNode(0)->getNode(1);
@@ -58,7 +55,6 @@ class Twig_Tests_NodeVisitor_OptimizerTest extends PHPUnit_Framework_TestCase
     public function testForOptimizer($template, $expected)
     {
         $env = new Twig_Environment(new Twig_Loader_String(), array('cache' => false));
-        $env->addExtension(new Twig_Extension_Optimizer());
 
         $stream = $env->parse($env->tokenize($template, 'index'));