refactored Templating

* made the renderer argument of Storage ctor mandatory * refactored the Engine class to avoid code duplication * simplified the check for a template that extends another one but with a different renderer
symfony · Jan 13, 2011 · c38c0c3 · c38c0c3
1 parent 285e09f
commit c38c0c3
Show file tree

Hide file tree

Showing 11 changed files with 32 additions and 52 deletions.
diff --git a/src/Symfony/Bundle/TwigBundle/Loader/Loader.php b/src/Symfony/Bundle/TwigBundle/Loader/Loader.php
@@ -46,10 +46,6 @@ public function getSource($name)
 
         list($name, $options) = $this->converter->fromShortNotation($name);
 
-        if ('twig' !== $options['renderer']) {
-            throw new \LogicException(sprintf('A "%s" template cannot extend a "Twig" template.', $options['renderer']));
-        }
-
         $template = $this->loader->load($name, $options);
 
         if (false === $template) {

diff --git a/src/Symfony/Component/Templating/Engine.php b/src/Symfony/Component/Templating/Engine.php
@@ -25,7 +25,6 @@ class Engine implements \ArrayAccess
     protected $loader;
     protected $renderers;
     protected $current;
-    protected $currentRenderer;
     protected $helpers;
     protected $parents;
     protected $stack;
@@ -86,30 +85,12 @@ public function setRenderers(array $renderers = array())
      */
     public function render($name, array $parameters = array())
     {
-        if (isset($this->cache[$name])) {
-            list($tpl, $options, $template) = $this->cache[$name];
-        } else {
-            list($tpl, $options) = $this->splitTemplateName($name);
-
-            // load
-            $template = $this->loader->load($tpl, $options);
-
-            if (false === $template) {
-                throw new \InvalidArgumentException(sprintf('The template "%s" does not exist (renderer: %s).', $name, $options['renderer']));
-            }
-
-            $this->cache[$name] = array($tpl, $options, $template);
-        }
+        $template = $this->load($name);
 
         // renderer
-        $renderer = $template->getRenderer() ? $template->getRenderer() : $options['renderer'];
+        $renderer = $template->getRenderer();
 
-        // a decorator must use the same renderer as its children
-        if (null !== $this->currentRenderer && $renderer !== $this->currentRenderer) {
-            throw new \LogicException(sprintf('A "%s" template cannot extend a "%s" template.', $this->currentRenderer, $renderer));
-        }
-
-        if (!isset($this->renderers[$options['renderer']])) {
+        if (!isset($this->renderers[$renderer])) {
             throw new \InvalidArgumentException(sprintf('The renderer "%s" is not registered.', $renderer));
         }
 
@@ -130,9 +111,12 @@ public function render($name, array $parameters = array())
             $this->stack[] = $slots->get('_content');
             $slots->set('_content', $content);
 
-            $this->currentRenderer = $renderer;
+            // a decorator must use the same renderer as its children
+            $parent = $this->load($this->parents[$name]);
+            if ($renderer !== $parentRenderer = ($parent->getRenderer() ? $parent->getRenderer() : $renderer)) {
+                throw new \LogicException(sprintf('Template "%s" extends "%s" but a "%s" template cannot extend a "%s" one.', $name, $this->parents[$name], $renderer, $parentRenderer));
+            }
             $content = $this->render($this->parents[$name], $parameters);
-            $this->currentRenderer = null;
 
             $slots->set('_content', array_pop($this->stack));
         }
@@ -162,20 +146,20 @@ public function exists($name)
     public function load($name)
     {
         if (isset($this->cache[$name])) {
-            list($tpl, $options, $template) = $this->cache[$name];
-        } else {
-            list($tpl, $options) = $this->splitTemplateName($name);
+            return $this->cache[$name];
+        }
 
-            // load
-            $template = $this->loader->load($tpl, $options);
+        list($tpl, $options) = $this->splitTemplateName($name);
 
-            if (false === $template) {
-                return false;
-            }
+        // load
+        $template = $this->loader->load($tpl, $options);
 
-            $this->cache[$name] = array($tpl, $options, $template);
+        if (false === $template) {
+            throw new \InvalidArgumentException(sprintf('The template "%s" does not exist (renderer: %s).', $name, $options['renderer']));
         }
 
+        $this->cache[$name] = $template;
+
         return $template;
     }
 

diff --git a/src/Symfony/Component/Templating/Loader/FilesystemLoader.php b/src/Symfony/Component/Templating/Loader/FilesystemLoader.php
@@ -50,7 +50,7 @@ public function __construct($templatePathPatterns)
     public function load($template, array $options = array())
     {
         if (self::isAbsolutePath($template) && file_exists($template)) {
-            return new FileStorage($template);
+            return new FileStorage($template, $options['renderer']);
         }
 
         $options = $this->mergeDefaultOptions($options);
@@ -68,7 +68,7 @@ public function load($template, array $options = array())
                     $this->debugger->log(sprintf('Loaded template file "%s" (renderer: %s)', $file, $options['renderer']));
                 }
 
-                return new FileStorage($file);
+                return new FileStorage($file, $options['renderer']);
             }
 
             if (null !== $this->debugger) {

diff --git a/src/Symfony/Component/Templating/Storage/Storage.php b/src/Symfony/Component/Templating/Storage/Storage.php
@@ -27,7 +27,7 @@ abstract class Storage
      * @param string $template The template name
      * @param string $renderer The renderer name
      */
-    public function __construct($template, $renderer = null)
+    public function __construct($template, $renderer)
     {
         $this->template = $template;
         $this->renderer = $renderer;

diff --git a/tests/Symfony/Tests/Component/Templating/EngineTest.php b/tests/Symfony/Tests/Component/Templating/EngineTest.php
@@ -191,7 +191,7 @@ public function setTemplate($name, $template)
     public function load($template, array $options = array())
     {
         if (isset($this->templates[$template.'.'.$options['renderer']])) {
-            return new StringStorage($this->templates[$template.'.'.$options['renderer']]);
+            return new StringStorage($this->templates[$template.'.'.$options['renderer']], $options['renderer']);
         }
 
         return false;

diff --git a/tests/Symfony/Tests/Component/Templating/Loader/CacheLoaderTest.php b/tests/Symfony/Tests/Component/Templating/Loader/CacheLoaderTest.php
@@ -69,7 +69,7 @@ public function getSpecialTemplate()
     public function load($template, array $options = array())
     {
         if (method_exists($this, $method = 'get'.ucfirst($template).'Template')) {
-            return new StringStorage($this->$method());
+            return new StringStorage($this->$method(), '.php');
         }
 
         return false;

diff --git a/tests/Symfony/Tests/Component/Templating/Loader/FilesystemLoaderTest.php b/tests/Symfony/Tests/Component/Templating/Loader/FilesystemLoaderTest.php
@@ -48,7 +48,7 @@ public function testLoad()
         $pathPattern = self::$fixturesPath.'/templates/%name%.%renderer%';
         $path = self::$fixturesPath.'/templates';
         $loader = new ProjectTemplateLoader2($pathPattern);
-        $storage = $loader->load($path.'/foo.php');
+        $storage = $loader->load($path.'/foo.php', array('renderer' => 'php'));
         $this->assertInstanceOf('Symfony\Component\Templating\Storage\FileStorage', $storage, '->load() returns a FileStorage if you pass an absolute path');
         $this->assertEquals($path.'/foo.php', (string) $storage, '->load() returns a FileStorage pointing to the passed absolute path');
 

diff --git a/tests/Symfony/Tests/Component/Templating/Renderer/PhpRendererTest.php b/tests/Symfony/Tests/Component/Templating/Renderer/PhpRendererTest.php
@@ -22,10 +22,10 @@ public function testEvaluate()
     {
         $renderer = new PhpRenderer();
 
-        $template = new StringStorage('<?php echo $foo ?>');
+        $template = new StringStorage('<?php echo $foo ?>', 'php');
         $this->assertEquals('bar', $renderer->evaluate($template, array('foo' => 'bar')), '->evaluate() renders templates that are instances of StringStorage');
 
-        $template = new FileStorage(__DIR__.'/../Fixtures/templates/foo.php');
+        $template = new FileStorage(__DIR__.'/../Fixtures/templates/foo.php', 'php');
         $this->assertEquals('bar', $renderer->evaluate($template, array('foo' => 'bar')), '->evaluate() renders templates that are instances of FileStorage');
     }
 }
diff --git a/tests/Symfony/Tests/Component/Templating/Storage/FileStorageTest.php b/tests/Symfony/Tests/Component/Templating/Storage/FileStorageTest.php
@@ -18,9 +18,9 @@ class FileStorageTest extends \PHPUnit_Framework_TestCase
 {
     public function testGetContent()
     {
-        $storage = new FileStorage('foo');
+        $storage = new FileStorage('foo', 'php');
         $this->assertInstanceOf('Symfony\Component\Templating\Storage\Storage', $storage, 'FileStorage is an instance of Storage');
-        $storage = new FileStorage(__DIR__.'/../Fixtures/templates/foo.php');
+        $storage = new FileStorage(__DIR__.'/../Fixtures/templates/foo.php', 'php');
         $this->assertEquals('<?php echo $foo ?>', $storage->getContent(), '->getContent() returns the content of the template');
     }
 }
diff --git a/tests/Symfony/Tests/Component/Templating/Storage/StorageTest.php b/tests/Symfony/Tests/Component/Templating/Storage/StorageTest.php
@@ -18,14 +18,14 @@ class StorageTest extends \PHPUnit_Framework_TestCase
 {
     public function testMagicToString()
     {
-        $storage = new TestStorage('foo');
+        $storage = new TestStorage('foo', 'php');
         $this->assertEquals('foo', (string) $storage, '__toString() returns the template name');
     }
 
     public function testGetRenderer()
     {
-        $storage = new TestStorage('foo', $renderer = new PhpRenderer());
-        $this->assertTrue($storage->getRenderer() === $renderer, '->getRenderer() returns the renderer');
+        $storage = new TestStorage('foo', 'php');
+        $this->assertEquals('php', $storage->getRenderer(), '->getRenderer() returns the renderer');
     }
 }
 

diff --git a/tests/Symfony/Tests/Component/Templating/Storage/StringStorageTest.php b/tests/Symfony/Tests/Component/Templating/Storage/StringStorageTest.php
@@ -18,9 +18,9 @@ class StringStorageTest extends \PHPUnit_Framework_TestCase
 {
     public function testGetContent()
     {
-        $storage = new StringStorage('foo');
+        $storage = new StringStorage('foo', 'php');
         $this->assertInstanceOf('Symfony\Component\Templating\Storage\Storage', $storage, 'StringStorage is an instance of Storage');
-        $storage = new StringStorage('foo');
+        $storage = new StringStorage('foo', 'php');
         $this->assertEquals('foo', $storage->getContent(), '->getContent() returns the content of the template');
     }
 }