Repository: twigphp/Twig (php)

Issue:
Errors thrown for an invalid parent don't report their file and line as the Twig template
See the fiddle at https://twigfiddle.com/hmgy4z

The template name and the line are reported as part of the message, not as part of the exception location, as done for most other Twig errors (and so the Symfony error page would not extract a code snippet from the Twig file).

The cause is that the location is set by using `\Twig\Template::loadTemplate` in the compile code ([see the compilation logic](https://github.com/twigphp/Twig/blob/19f98c7298359f62a65ccf670b123e8836582394/src/Node/ModuleNode.php#L317-L325)) but that method takes only a template name, not a source context. And when it builds a source context from it, it cannot give it a source path, even if the template had the info.

A suggestion to fix that would be to change `\Twig\Template::loadTemplate` to accept a SourceContext rather than a template name (we might support both if needed) for cases which have it (most places now, as the compiled code always extract the name from the source context anyway).

Pull request description:
Remove template source from Template::loadTemplate()
Closes #3266

The first commit supports string and Source. But I think we don't even need to pass anything (that's the other commits).


Patch:
diff --git a/src/Node/EmbedNode.php b/src/Node/EmbedNode.php
index 597f95e4..fe4365b5 100644
--- a/src/Node/EmbedNode.php
+++ b/src/Node/EmbedNode.php
@@ -36,11 +36,9 @@ class EmbedNode extends IncludeNode
     protected function addGetTemplate(Compiler $compiler, string $template = ''): void
     {
         $compiler
-            ->raw('$this->loadTemplate(')
+            ->raw('$this->load(')
             ->string($this->getAttribute('name'))
             ->raw(', ')
-            ->repr($this->getTemplateName())
-            ->raw(', ')
             ->repr($this->getTemplateLine())
             ->raw(', ')
             ->string($this->getAttribute('index'))
diff --git a/src/Node/Expression/BlockReferenceExpression.php b/src/Node/Expression/BlockReferenceExpression.php
index 8fe3f8b1..cb7d38c5 100644
--- a/src/Node/Expression/BlockReferenceExpression.php
+++ b/src/Node/Expression/BlockReferenceExpression.php
@@ -66,11 +66,9 @@ class BlockReferenceExpression extends AbstractExpression implements SupportDefi
             $compiler->write('$this');
         } else {
             $compiler
-                ->write('$this->loadTemplate(')
+                ->write('$this->load(')
                 ->subcompile($this->getNode('template'))
                 ->raw(', ')
-                ->repr($this->getTemplateName())
-                ->raw(', ')
                 ->repr($this->getTemplateLine())
                 ->raw(')')
             ;
diff --git a/src/Node/ImportNode.php b/src/Node/ImportNode.php
index 77a9af93..92bdd5eb 100644
--- a/src/Node/ImportNode.php
+++ b/src/Node/ImportNode.php
@@ -48,11 +48,9 @@ class ImportNode extends Node
             $compiler->raw('$this');
         } else {
             $compiler
-                ->raw('$this->loadTemplate(')
+                ->raw('$this->load(')
                 ->subcompile($this->getNode('expr'))
                 ->raw(', ')
-                ->repr($this->getTemplateName())
-                ->raw(', ')
                 ->repr($this->getTemplateLine())
                 ->raw(')->unwrap()')
             ;
diff --git a/src/Node/IncludeNode.php b/src/Node/IncludeNode.php
index 4761cb83..6e17300f 100644
--- a/src/Node/IncludeNode.php
+++ b/src/Node/IncludeNode.php
@@ -84,11 +84,9 @@ class IncludeNode extends Node implements NodeOutputInterface
     protected function addGetTemplate(Compiler $compiler/* , string $template = '' */)
     {
         $compiler
-            ->raw('$this->loadTemplate(')
+            ->raw('$this->load(')
             ->subcompile($this->getNode('expr'))
             ->raw(', ')
-            ->repr($this->getTemplateName())
-            ->raw(', ')
             ->repr($this->getTemplateLine())
             ->raw(')')
         ;
diff --git a/src/Node/ModuleNode.php b/src/Node/ModuleNode.php
index b3f4e6c2..648cf955 100644
--- a/src/Node/ModuleNode.php
+++ b/src/Node/ModuleNode.php
@@ -134,11 +134,9 @@ final class ModuleNode extends Node
             $compiler->subcompile($parent);
         } else {
             $compiler
-                ->raw('$this->loadTemplate(')
+                ->raw('$this->load(')
                 ->subcompile($parent)
                 ->raw(', ')
-                ->repr($this->getSourceContext()->getName())
-                ->raw(', ')
                 ->repr($parent->getTemplateLine())
                 ->raw(')')
             ;
@@ -218,11 +216,9 @@ final class ModuleNode extends Node
 
                 $compiler
                     ->addDebugInfo($node)
-                    ->write(\sprintf('$_trait_%s = $this->loadTemplate(', $i))
+                    ->write(\sprintf('$_trait_%s = $this->load(', $i))
                     ->subcompile($node)
                     ->raw(', ')
-                    ->repr($node->getTemplateName())
-                    ->raw(', ')
                     ->repr($node->getTemplateLine())
                     ->raw(");\n")
                     ->write(\sprintf("if (!\$_trait_%s->unwrap()->isTraitable()) {\n", $i))
@@ -353,11 +349,9 @@ final class ModuleNode extends Node
             $compiler->addDebugInfo($parent);
             if ($parent instanceof ConstantExpression) {
                 $compiler
-                    ->write('$this->parent = $this->loadTemplate(')
+                    ->write('$this->parent = $this->load(')
                     ->subcompile($parent)
                     ->raw(', ')
-                    ->repr($this->getSourceContext()->getName())
-                    ->raw(', ')
                     ->repr($parent->getTemplateLine())
                     ->raw(");\n")
                 ;
diff --git a/src/Template.php b/src/Template.php
index 156752f8..4973aa75 100644
--- a/src/Template.php
+++ b/src/Template.php
@@ -89,7 +89,7 @@ abstract class Template
         }
 
         if (!isset($this->parents[$parent])) {
-            $this->parents[$parent] = $this->loadTemplate($parent);
+            $this->parents[$parent] = $this->load($parent, -1);
         }
 
         return $this->parents[$parent];
@@ -270,21 +270,15 @@ abstract class Template
     /**
      * @param string|TemplateWrapper|array<string|TemplateWrapper> $template
      */
-    protected function loadTemplate($template, $templateName = null, $line = null, $index = null): self|TemplateWrapper
+    protected function load(string|TemplateWrapper|array $template, int $line, int|null $index = null): self
     {
         try {
             if (\is_array($template)) {
-                return $this->env->resolveTemplate($template);
+                return $this->env->resolveTemplate($template)->unwrap();
             }
 
             if ($template instanceof TemplateWrapper) {
-                return $template;
-            }
-
-            if ($template instanceof self) {
-                trigger_deprecation('twig/twig', '3.9', 'Passing a "%s" instance to "%s" is deprecated.', self::class, __METHOD__);
-
-                return $template;
+                return $template->unwrap();
             }
 
             if ($template === $this->getTemplateName()) {
@@ -299,14 +293,14 @@ abstract class Template
             return $this->env->loadTemplate($class, $template, $index);
         } catch (Error $e) {
             if (!$e->getSourceContext()) {
-                $e->setSourceContext($templateName ? new Source('', $templateName) : $this->getSourceContext());
+                $e->setSourceContext($this->getSourceContext());
             }
 
             if ($e->getTemplateLine() > 0) {
                 throw $e;
             }
 
-            if (!$line) {
+            if (-1 === $line) {
                 $e->guess();
             } else {
                 $e->setTemplateLine($line);
@@ -316,6 +310,28 @@ abstract class Template
         }
     }
 
+    /**
+     * @param string|TemplateWrapper|array<string|TemplateWrapper> $template
+     */
+    protected function loadTemplate($template, $templateName = null, int|null $line = null, int|null $index = null): self|TemplateWrapper
+    {
+        trigger_deprecation('twig/twig', '3.21', 'The "%s" method is deprecated.', __METHOD__);
+
+        if (null === $line) {
+            trigger_deprecation('twig/twig', '3.21', 'Passing a "null" line number to "%s" is deprecated.', __METHOD__);
+
+            $line = -1;
+        }
+
+        if ($template instanceof self) {
+            trigger_deprecation('twig/twig', '3.9', 'Passing a "%s" instance to "%s" is deprecated.', self::class, __METHOD__);
+
+            return $template;
+        }
+
+        return $this->load($template, $line, $index);
+    }
+
     /**
      * @internal
      *


Test patch:
diff --git a/tests/Fixtures/templates/include.twig b/tests/Fixtures/templates/include.twig
new file mode 100644
index 00000000..597c9f06
--- /dev/null
+++ b/tests/Fixtures/templates/include.twig
@@ -0,0 +1,4 @@
+
+
+
+{% extends 'invalid.twig' %}
diff --git a/tests/Fixtures/templates/index.twig b/tests/Fixtures/templates/index.twig
new file mode 100644
index 00000000..a15e87d1
--- /dev/null
+++ b/tests/Fixtures/templates/index.twig
@@ -0,0 +1,1 @@
+{% include "include.twig" %}
diff --git a/tests/Node/ExtendsTest.php b/tests/Node/ExtendsTest.php
new file mode 100644
index 00000000..a47302d4
--- /dev/null
+++ b/tests/Node/ExtendsTest.php
@@ -0,0 +1,65 @@
+<?php
+
+namespace Twig\Tests\Node;
+
+/*
+ * This file is part of Twig.
+ *
+ * (c) Fabien Potencier
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+use Twig\Environment;
+use Twig\Error\LoaderError;
+use Twig\Loader\ArrayLoader;
+use Twig\Loader\FilesystemLoader;
+use Twig\Test\NodeTestCase;
+
+class ExtendsTest extends NodeTestCase
+{
+    public function testErrorFromArrayLoader()
+    {
+        $twig = new Environment(new ArrayLoader([
+            'index.twig' => '{% include "include.twig" %}',
+            'include.twig' => $include = <<<EOF
+
+
+
+            {% extends 'invalid.twig' %}
+            EOF,
+        ]), ['debug' => true]);
+        try {
+            $twig->render('index.twig');
+            $this->fail('Expected LoaderError to be thrown');
+        } catch (LoaderError $e) {
+            $this->assertSame('Template "invalid.twig" is not defined.', $e->getRawMessage());
+            $this->assertSame(4, $e->getTemplateLine());
+            $this->assertSame('include.twig', $e->getSourceContext()->getName());
+            $this->assertSame($include, $e->getSourceContext()->getCode());
+        }
+    }
+
+    public function testErrorFromFilesystemLoader()
+    {
+        $twig = new Environment(new FilesystemLoader([
+            $dir = dirname(__DIR__).'/Fixtures/templates',
+        ]), ['debug' => true]);
+        $include = file_get_contents($dir.'/include.twig');
+        try {
+            $twig->render('index.twig');
+            $this->fail('Expected LoaderError to be thrown');
+        } catch (LoaderError $e) {
+            $this->assertStringContainsString('Unable to find template "invalid.twig"', $e->getRawMessage());
+            $this->assertSame(4, $e->getTemplateLine());
+            $this->assertSame('include.twig', $e->getSourceContext()->getName());
+            $this->assertSame($include, $e->getSourceContext()->getCode());
+        }
+    }
+
+    public static function provideTests(): iterable
+    {
+        return [];
+    }
+}
diff --git a/tests/Node/ImportTest.php b/tests/Node/ImportTest.php
index a5513706..fb8dd9ba 100644
--- a/tests/Node/ImportTest.php
+++ b/tests/Node/ImportTest.php
@@ -37,7 +37,7 @@ class ImportTest extends NodeTestCase
 
         $tests[] = [$node, <<<EOF
 // line 1
-\$macros["macro"] = \$this->macros["macro"] = \$this->loadTemplate("foo.twig", null, 1)->unwrap();
+\$macros["macro"] = \$this->macros["macro"] = \$this->load("foo.twig", 1)->unwrap();
 EOF
         ];
 
diff --git a/tests/Node/IncludeTest.php b/tests/Node/IncludeTest.php
index 8a73a76c..ea80979b 100644
--- a/tests/Node/IncludeTest.php
+++ b/tests/Node/IncludeTest.php
@@ -42,7 +42,7 @@ class IncludeTest extends NodeTestCase
         $node = new IncludeNode($expr, null, false, false, 1);
         $tests[] = [$node, <<<'EOF'
 // line 1
-yield from $this->loadTemplate("foo.twig", null, 1)->unwrap()->yield($context);
+yield from $this->load("foo.twig", 1)->unwrap()->yield($context);
 EOF
         ];
 
@@ -55,7 +55,7 @@ EOF
         $node = new IncludeNode($expr, null, false, false, 1);
         $tests[] = [$node, <<<'EOF'
 // line 1
-yield from $this->loadTemplate(((true) ? ("foo") : ("foo")), null, 1)->unwrap()->yield($context);
+yield from $this->load(((true) ? ("foo") : ("foo")), 1)->unwrap()->yield($context);
 EOF
         ];
 
@@ -64,14 +64,14 @@ EOF
         $node = new IncludeNode($expr, $vars, false, false, 1);
         $tests[] = [$node, <<<'EOF'
 // line 1
-yield from $this->loadTemplate("foo.twig", null, 1)->unwrap()->yield(CoreExtension::merge($context, ["foo" => true]));
+yield from $this->load("foo.twig", 1)->unwrap()->yield(CoreExtension::merge($context, ["foo" => true]));
 EOF
         ];
 
         $node = new IncludeNode($expr, $vars, true, false, 1);
         $tests[] = [$node, <<<'EOF'
 // line 1
-yield from $this->loadTemplate("foo.twig", null, 1)->unwrap()->yield(CoreExtension::toArray(["foo" => true]));
+yield from $this->load("foo.twig", 1)->unwrap()->yield(CoreExtension::toArray(["foo" => true]));
 EOF
         ];
 
@@ -79,7 +79,7 @@ EOF
         $tests[] = [$node, <<<EOF
 // line 1
 try {
-    \$_v%s = \$this->loadTemplate("foo.twig", null, 1);
+    \$_v%s = \$this->load("foo.twig", 1);
 } catch (LoaderError \$e) {
     // ignore missing template
     \$_v%s = null;
diff --git a/tests/Node/ModuleTest.php b/tests/Node/ModuleTest.php
index 5caddd93..a2032ad8 100644
--- a/tests/Node/ModuleTest.php
+++ b/tests/Node/ModuleTest.php
@@ -183,9 +183,9 @@ class __TwigTemplate_%x extends Template
     {
         \$macros = \$this->macros;
         // line 2
-        \$macros["macro"] = \$this->macros["macro"] = \$this->loadTemplate("foo.twig", "foo.twig", 2)->unwrap();
+        \$macros["macro"] = \$this->macros["macro"] = \$this->load("foo.twig", 2)->unwrap();
         // line 1
-        \$this->parent = \$this->loadTemplate("layout.twig", "foo.twig", 1);
+        \$this->parent = \$this->load("layout.twig", 1);
         yield from \$this->parent->unwrap()->yield(\$context, array_merge(\$this->blocks, \$blocks));
     }
 
@@ -271,7 +271,7 @@ class __TwigTemplate_%x extends Template
     protected function doGetParent(array \$context): bool|string|Template|TemplateWrapper
     {
         // line 2
-        return \$this->loadTemplate(((true) ? ("foo") : ("foo")), "foo.twig", 2);
+        return \$this->load(((true) ? ("foo") : ("foo")), 2);
     }
 
     protected function doDisplay(array \$context, array \$blocks = []): iterable