From d4db91535d8bd167f460beaea62542bbe6bd6861 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Thu, 11 Apr 2019 23:10:44 +0200 Subject: [PATCH 1/2] added missing hints in some runtime exceptions --- src/Error/Error.php | 8 +--- src/Error/LoaderError.php | 12 ------ src/Node/Expression/CallExpression.php | 20 ++++++---- src/Node/Expression/Test/DefinedTest.php | 2 +- src/Node/MacroNode.php | 2 +- src/Node/ModuleNode.php | 40 ++++++++++--------- src/Node/Node.php | 17 ++++++++ src/Node/WithNode.php | 7 +++- src/Template.php | 10 ++++- test/Twig/Tests/ExpressionParserTest.php | 6 ++- .../call_argument_defined_twice.test | 8 ++++ .../call_positional_arg_after_named_arg.test | 8 ++++ .../tests/defined_on_complex_expr.test | 8 ++++ 13 files changed, 95 insertions(+), 53 deletions(-) create mode 100644 test/Twig/Tests/Fixtures/expressions/call_argument_defined_twice.test create mode 100644 test/Twig/Tests/Fixtures/expressions/call_positional_arg_after_named_arg.test create mode 100644 test/Twig/Tests/Fixtures/tests/defined_on_complex_expr.test diff --git a/src/Error/Error.php b/src/Error/Error.php index e43a36374..6dd9b9d4d 100644 --- a/src/Error/Error.php +++ b/src/Error/Error.php @@ -63,7 +63,7 @@ class Error extends \Exception * @param Source|string|null $source The source context where the error occurred * @param \Exception $previous The previous exception */ - public function __construct($message, $lineno = -1, $source = null, \Exception $previous = null, $autoGuess = true) + public function __construct($message, $lineno = -1, $source = null, \Exception $previous = null) { if (null === $source) { $name = null; @@ -79,13 +79,7 @@ class Error extends \Exception $this->lineno = $lineno; $this->filename = $name; - - if ($autoGuess && (-1 === $lineno || null === $name || null === $this->sourcePath)) { - $this->guessTemplateInfo(); - } - $this->rawMessage = $message; - $this->updateRepr(); } diff --git a/src/Error/LoaderError.php b/src/Error/LoaderError.php index 5a1cd1ecc..dc5a9f1af 100644 --- a/src/Error/LoaderError.php +++ b/src/Error/LoaderError.php @@ -14,22 +14,10 @@ namespace Twig\Error; /** * Exception thrown when an error occurs during template loading. * - * Automatic template information guessing is always turned off as - * if a template cannot be loaded, there is nothing to guess. - * However, when a template is loaded from another one, then, we need - * to find the current context and this is automatically done by - * Twig\Template::displayWithErrorHandling(). - * - * This strategy makes Twig\Environment::resolveTemplate() much faster. - * * @author Fabien Potencier */ class LoaderError extends Error { - public function __construct($message, $lineno = -1, $source = null, \Exception $previous = null) - { - parent::__construct($message, $lineno, $source, $previous, false); - } } class_alias('Twig\Error\LoaderError', 'Twig_Error_Loader'); diff --git a/src/Node/Expression/CallExpression.php b/src/Node/Expression/CallExpression.php index 162393bdf..d202a7395 100644 --- a/src/Node/Expression/CallExpression.php +++ b/src/Node/Expression/CallExpression.php @@ -121,7 +121,7 @@ abstract class CallExpression extends AbstractExpression $named = true; $name = $this->normalizeName($name); } elseif ($named) { - throw new SyntaxError(sprintf('Positional arguments cannot be used after named arguments for %s "%s".', $callType, $callName), $this->getTemplateLine(), null, null, false); + throw new SyntaxError(sprintf('Positional arguments cannot be used after named arguments for %s "%s".', $callType, $callName), $this->getTemplateLine(), $this->getSourceContext()); } $parameters[$name] = $node; @@ -153,14 +153,14 @@ abstract class CallExpression extends AbstractExpression if (\array_key_exists($name, $parameters)) { if (\array_key_exists($pos, $parameters)) { - throw new SyntaxError(sprintf('Argument "%s" is defined twice for %s "%s".', $name, $callType, $callName), $this->getTemplateLine(), null, null, false); + throw new SyntaxError(sprintf('Argument "%s" is defined twice for %s "%s".', $name, $callType, $callName), $this->getTemplateLine(), $this->getSourceContext()); } if (\count($missingArguments)) { throw new SyntaxError(sprintf( 'Argument "%s" could not be assigned for %s "%s(%s)" because it is mapped to an internal PHP function which cannot determine default value for optional argument%s "%s".', $name, $callType, $callName, implode(', ', $names), \count($missingArguments) > 1 ? 's' : '', implode('", "', $missingArguments) - ), $this->getTemplateLine(), null, null, false); + ), $this->getTemplateLine(), $this->getSourceContext()); } $arguments = array_merge($arguments, $optionalArguments); @@ -182,7 +182,7 @@ abstract class CallExpression extends AbstractExpression $missingArguments[] = $name; } } else { - throw new SyntaxError(sprintf('Value for argument "%s" is required for %s "%s".', $name, $callType, $callName), $this->getTemplateLine(), null, null, false); + throw new SyntaxError(sprintf('Value for argument "%s" is required for %s "%s".', $name, $callType, $callName), $this->getTemplateLine(), $this->getSourceContext()); } } @@ -212,10 +212,14 @@ abstract class CallExpression extends AbstractExpression } } - throw new SyntaxError(sprintf( - 'Unknown argument%s "%s" for %s "%s(%s)".', - \count($parameters) > 1 ? 's' : '', implode('", "', array_keys($parameters)), $callType, $callName, implode(', ', $names) - ), $unknownParameter ? $unknownParameter->getTemplateLine() : $this->getTemplateLine(), null, null, false); + throw new SyntaxError( + sprintf( + 'Unknown argument%s "%s" for %s "%s(%s)".', + \count($parameters) > 1 ? 's' : '', implode('", "', array_keys($parameters)), $callType, $callName, implode(', ', $names) + ), + $unknownParameter ? $unknownParameter->getTemplateLine() : $this->getTemplateLine(), + $unknownParameter ? $unknownParameter->getSourceContext() : $this->getSourceContext() + ); } return $arguments; diff --git a/src/Node/Expression/Test/DefinedTest.php b/src/Node/Expression/Test/DefinedTest.php index 6197fb93c..2222e11cf 100644 --- a/src/Node/Expression/Test/DefinedTest.php +++ b/src/Node/Expression/Test/DefinedTest.php @@ -47,7 +47,7 @@ class DefinedTest extends TestExpression } elseif ($node instanceof ConstantExpression || $node instanceof ArrayExpression) { $node = new ConstantExpression(true, $node->getTemplateLine()); } else { - throw new SyntaxError('The "defined" test only works with simple variables.', $this->getTemplateLine(), null, null, false); + throw new SyntaxError('The "defined" test only works with simple variables.', $lineno); } parent::__construct($node, $name, $arguments, $lineno); diff --git a/src/Node/MacroNode.php b/src/Node/MacroNode.php index 31f4baa7a..8e3cc76a3 100644 --- a/src/Node/MacroNode.php +++ b/src/Node/MacroNode.php @@ -27,7 +27,7 @@ class MacroNode extends Node { foreach ($arguments as $argumentName => $argument) { if (self::VARARGS_NAME === $argumentName) { - throw new SyntaxError(sprintf('The argument "%s" in macro "%s" cannot be defined because the variable "%s" is reserved for arbitrary arguments.', self::VARARGS_NAME, $name, self::VARARGS_NAME), $argument->getTemplateLine(), null, null, false); + throw new SyntaxError(sprintf('The argument "%s" in macro "%s" cannot be defined because the variable "%s" is reserved for arbitrary arguments.', self::VARARGS_NAME, $name, self::VARARGS_NAME), $argument->getTemplateLine(), $argument->getSourceContext()); } } diff --git a/src/Node/ModuleNode.php b/src/Node/ModuleNode.php index 665f93147..8b9047080 100644 --- a/src/Node/ModuleNode.php +++ b/src/Node/ModuleNode.php @@ -28,15 +28,13 @@ use Twig\Source; */ class ModuleNode extends Node { - private $source; - public function __construct(\Twig_NodeInterface $body, AbstractExpression $parent = null, \Twig_NodeInterface $blocks, \Twig_NodeInterface $macros, \Twig_NodeInterface $traits, $embeddedTemplates, $name, $source = '') { if (!$name instanceof Source) { @trigger_error(sprintf('Passing a string as the $name argument of %s() is deprecated since version 1.27. Pass a \Twig\Source instance instead.', __METHOD__), E_USER_DEPRECATED); - $this->source = new Source($source, $name); + $source = new Source($source, $name); } else { - $this->source = $name; + $source = $name; } $nodes = [ @@ -57,15 +55,16 @@ class ModuleNode extends Node // embedded templates are set as attributes so that they are only visited once by the visitors parent::__construct($nodes, [ // source to be remove in 2.0 - 'source' => $this->source->getCode(), + 'source' => $source->getCode(), // filename to be remove in 2.0 (use getTemplateName() instead) - 'filename' => $this->source->getName(), + 'filename' => $source->getName(), 'index' => null, 'embedded_templates' => $embeddedTemplates, ], 1); // populate the template name of all node children - $this->setTemplateName($this->source->getName()); + $this->setTemplateName($source->getName()); + $this->setSourceContext($source); } public function setIndex($index) @@ -143,7 +142,7 @@ class ModuleNode extends Node ->raw('$this->loadTemplate(') ->subcompile($parent) ->raw(', ') - ->repr($this->source->getName()) + ->repr($this->getSourceContext()->getName()) ->raw(', ') ->repr($parent->getTemplateLine()) ->raw(')') @@ -178,8 +177,8 @@ class ModuleNode extends Node } $compiler // if the template name contains */, add a blank to avoid a PHP parse error - ->write('/* '.str_replace('*/', '* /', $this->source->getName())." */\n") - ->write('class '.$compiler->getEnvironment()->getTemplateClass($this->source->getName(), $this->getAttribute('index'))) + ->write('/* '.str_replace('*/', '* /', $this->getSourceContext()->getName())." */\n") + ->write('class '.$compiler->getEnvironment()->getTemplateClass($this->getSourceContext()->getName(), $this->getAttribute('index'))) ->raw(sprintf(" extends %s\n", $compiler->getEnvironment()->getBaseTemplateClass())) ->write("{\n") ->indent() @@ -206,13 +205,16 @@ class ModuleNode extends Node foreach ($this->getNode('traits') as $i => $trait) { $this->compileLoadTemplate($compiler, $trait->getNode('template'), sprintf('$_trait_%s', $i)); + $node = $node = $trait->getNode('template'); $compiler - ->addDebugInfo($trait->getNode('template')) + ->addDebugInfo($node) ->write(sprintf("if (!\$_trait_%s->isTraitable()) {\n", $i)) ->indent() ->write("throw new RuntimeError('Template \"'.") ->subcompile($trait->getNode('template')) - ->raw(".'\" cannot be used as a trait.');\n") + ->raw(".'\" cannot be used as a trait.', ") + ->repr($node->getTemplateLine()) + ->raw(", \$this->getSourceContext());\n") ->outdent() ->write("}\n") ->write(sprintf("\$_trait_%s_blocks = \$_trait_%s->getBlocks();\n\n", $i, $i)) @@ -228,7 +230,9 @@ class ModuleNode extends Node ->string($key) ->raw(' is not defined in trait ') ->subcompile($trait->getNode('template')) - ->raw(".'));\n") + ->raw(".'), ") + ->repr($node->getTemplateLine()) + ->raw(", \$this->getSourceContext());\n") ->outdent() ->write("}\n\n") @@ -327,7 +331,7 @@ class ModuleNode extends Node ->write('$this->parent = $this->loadTemplate(') ->subcompile($parent) ->raw(', ') - ->repr($this->source->getName()) + ->repr($this->getSourceContext()->getName()) ->raw(', ') ->repr($parent->getTemplateLine()) ->raw(");\n") @@ -366,7 +370,7 @@ class ModuleNode extends Node ->write("public function getTemplateName()\n", "{\n") ->indent() ->write('return ') - ->repr($this->source->getName()) + ->repr($this->getSourceContext()->getName()) ->raw(";\n") ->outdent() ->write("}\n\n") @@ -456,11 +460,11 @@ class ModuleNode extends Node ->write("public function getSourceContext()\n", "{\n") ->indent() ->write('return new Source(') - ->string($compiler->getEnvironment()->isDebug() ? $this->source->getCode() : '') + ->string($compiler->getEnvironment()->isDebug() ? $this->getSourceContext()->getCode() : '') ->raw(', ') - ->string($this->source->getName()) + ->string($this->getSourceContext()->getName()) ->raw(', ') - ->string($this->source->getPath()) + ->string($this->getSourceContext()->getPath()) ->raw(");\n") ->outdent() ->write("}\n") diff --git a/src/Node/Node.php b/src/Node/Node.php index d60367ff2..fb9aeb19c 100644 --- a/src/Node/Node.php +++ b/src/Node/Node.php @@ -13,6 +13,7 @@ namespace Twig\Node; use Twig\Compiler; +use Twig\Source; /** * Represents a node in the AST. @@ -27,6 +28,7 @@ class Node implements \Twig_NodeInterface protected $tag; private $name; + private $sourceContext; /** * Constructor. @@ -235,6 +237,21 @@ class Node implements \Twig_NodeInterface return $this->name; } + public function setSourceContext(Source $source) + { + $this->sourceContext = $source; + foreach ($this->nodes as $node) { + if (null !== $node) { + $node->setSourceContext($source); + } + } + } + + public function getSourceContext() + { + return $this->sourceContext; + } + /** * @deprecated since 1.27 (to be removed in 2.0) */ diff --git a/src/Node/WithNode.php b/src/Node/WithNode.php index d14bac252..f5ae9246d 100644 --- a/src/Node/WithNode.php +++ b/src/Node/WithNode.php @@ -35,14 +35,17 @@ class WithNode extends Node $compiler->addDebugInfo($this); if ($this->hasNode('variables')) { + $node = $this->getNode('variables'); $varsName = $compiler->getVarName(); $compiler ->write(sprintf('$%s = ', $varsName)) - ->subcompile($this->getNode('variables')) + ->subcompile($node) ->raw(";\n") ->write(sprintf("if (!twig_test_iterable(\$%s)) {\n", $varsName)) ->indent() - ->write("throw new RuntimeError('Variables passed to the \"with\" tag must be a hash.');\n") + ->write("throw new RuntimeError('Variables passed to the \"with\" tag must be a hash.', ") + ->repr($node->getTemplateLine()) + ->raw(", \$this->getSourceContext());\n") ->outdent() ->write("}\n") ->write(sprintf("\$%s = twig_to_array(\$%s);\n", $varsName, $varsName)) diff --git a/src/Template.php b/src/Template.php index b56872775..e2146c280 100644 --- a/src/Template.php +++ b/src/Template.php @@ -227,7 +227,10 @@ abstract class Template implements \Twig_TemplateInterface throw $e; } catch (\Exception $e) { - throw new RuntimeError(sprintf('An exception has been thrown during the rendering of a template ("%s").', $e->getMessage()), -1, $template->getSourceContext(), $e); + $e = new RuntimeError(sprintf('An exception has been thrown during the rendering of a template ("%s").', $e->getMessage()), -1, $template->getSourceContext(), $e); + $e->guess(); + + throw $e; } } elseif (false !== $parent = $this->getParent($context)) { $parent->displayBlock($name, $context, array_merge($this->blocks, $blocks), false); @@ -438,7 +441,10 @@ abstract class Template implements \Twig_TemplateInterface throw $e; } catch (\Exception $e) { - throw new RuntimeError(sprintf('An exception has been thrown during the rendering of a template ("%s").', $e->getMessage()), -1, $this->getSourceContext(), $e); + $e = new RuntimeError(sprintf('An exception has been thrown during the rendering of a template ("%s").', $e->getMessage()), -1, $this->getSourceContext(), $e); + $e->guess(); + + throw $e; } } diff --git a/test/Twig/Tests/ExpressionParserTest.php b/test/Twig/Tests/ExpressionParserTest.php index 169693807..6a447a99a 100644 --- a/test/Twig/Tests/ExpressionParserTest.php +++ b/test/Twig/Tests/ExpressionParserTest.php @@ -55,8 +55,9 @@ class Twig_Tests_ExpressionParserTest extends \PHPUnit\Framework\TestCase public function testArrayExpression($template, $expected) { $env = new Environment($this->getMockBuilder('\Twig\Loader\LoaderInterface')->getMock(), ['cache' => false, 'autoescape' => false]); - $stream = $env->tokenize(new Source($template, '')); + $stream = $env->tokenize($source = new Source($template, '')); $parser = new Parser($env); + $expected->setSourceContext($source); $this->assertEquals($expected, $parser->parse($stream)->getNode('body')->getNode(0)->getNode('expr')); } @@ -175,8 +176,9 @@ class Twig_Tests_ExpressionParserTest extends \PHPUnit\Framework\TestCase public function testStringExpression($template, $expected) { $env = new Environment($this->getMockBuilder('\Twig\Loader\LoaderInterface')->getMock(), ['cache' => false, 'autoescape' => false, 'optimizations' => 0]); - $stream = $env->tokenize(new Source($template, '')); + $stream = $env->tokenize($source = new Source($template, '')); $parser = new Parser($env); + $expected->setSourceContext($source); $this->assertEquals($expected, $parser->parse($stream)->getNode('body')->getNode(0)->getNode('expr')); } diff --git a/test/Twig/Tests/Fixtures/expressions/call_argument_defined_twice.test b/test/Twig/Tests/Fixtures/expressions/call_argument_defined_twice.test new file mode 100644 index 000000000..36539a6d1 --- /dev/null +++ b/test/Twig/Tests/Fixtures/expressions/call_argument_defined_twice.test @@ -0,0 +1,8 @@ +--TEST-- +Argument is defined twice in a call +--TEMPLATE-- +{{ date(987654, date = 123456) }} +--DATA-- +return [] +--EXCEPTION-- +Twig\Error\SyntaxError: Argument "date" is defined twice for function "date" in "index.twig" at line 2. diff --git a/test/Twig/Tests/Fixtures/expressions/call_positional_arg_after_named_arg.test b/test/Twig/Tests/Fixtures/expressions/call_positional_arg_after_named_arg.test new file mode 100644 index 000000000..729c67485 --- /dev/null +++ b/test/Twig/Tests/Fixtures/expressions/call_positional_arg_after_named_arg.test @@ -0,0 +1,8 @@ +--TEST-- +Positional arguments after named arguments in a call +--TEMPLATE-- +{{ date(date = 123456, 'Y-m-d') }} +--DATA-- +return [] +--EXCEPTION-- +Twig\Error\SyntaxError: Positional arguments cannot be used after named arguments for function "date" in "index.twig" at line 2. diff --git a/test/Twig/Tests/Fixtures/tests/defined_on_complex_expr.test b/test/Twig/Tests/Fixtures/tests/defined_on_complex_expr.test new file mode 100644 index 000000000..2d0615832 --- /dev/null +++ b/test/Twig/Tests/Fixtures/tests/defined_on_complex_expr.test @@ -0,0 +1,8 @@ +--TEST-- +"defined" support for "complex" expressions +--TEMPLATE-- +{{ (1 + 2) is defined ? 'ok' : 'ko' }} +--DATA-- +return [] +--EXCEPTION-- +Twig\Error\SyntaxError: The "defined" test only works with simple variables in "index.twig" at line 2. From 094df799be30a977ccf3bfac54b9dd2d97cdb6fa Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Fri, 12 Apr 2019 10:30:51 +0200 Subject: [PATCH 2/2] added more tests --- CHANGELOG | 2 ++ .../exceptions/exception_in_extension_extends.test | 12 ++++++++++++ .../exceptions/exception_in_extension_include.test | 12 ++++++++++++ 3 files changed, 26 insertions(+) create mode 100644 test/Twig/Tests/Fixtures/exceptions/exception_in_extension_extends.test create mode 100644 test/Twig/Tests/Fixtures/exceptions/exception_in_extension_include.test diff --git a/CHANGELOG b/CHANGELOG index 9fd7bf1a1..c7248c3fb 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,5 +1,7 @@ * 1.39.0 (2019-XX-XX) + * fixed some wrong location in error messages + * made exception creation faster * made escaping on ternary expressions (?: and ??) more fine-grained * added the possibility to give a nice name to string templates (template_from_string function) * fixed the "with" behavior to always include the globals (for consistency with the "include" and "embed" tags) diff --git a/test/Twig/Tests/Fixtures/exceptions/exception_in_extension_extends.test b/test/Twig/Tests/Fixtures/exceptions/exception_in_extension_extends.test new file mode 100644 index 000000000..2ab298059 --- /dev/null +++ b/test/Twig/Tests/Fixtures/exceptions/exception_in_extension_extends.test @@ -0,0 +1,12 @@ +--TEST-- +Exception thrown from a child for an extension error +--TEMPLATE-- +{% extends 'base.twig' %} +--TEMPLATE(base.twig)-- + + +{{ random([]) }} +--DATA-- +return [] +--EXCEPTION-- +Twig\Error\RuntimeError: The random function cannot pick from an empty array in "base.twig" at line 4. diff --git a/test/Twig/Tests/Fixtures/exceptions/exception_in_extension_include.test b/test/Twig/Tests/Fixtures/exceptions/exception_in_extension_include.test new file mode 100644 index 000000000..e2281b290 --- /dev/null +++ b/test/Twig/Tests/Fixtures/exceptions/exception_in_extension_include.test @@ -0,0 +1,12 @@ +--TEST-- +Exception thrown from an include for an extension error +--TEMPLATE-- +{% include 'content.twig' %} +--TEMPLATE(content.twig)-- + + +{{ random([]) }} +--DATA-- +return [] +--EXCEPTION-- +Twig\Error\RuntimeError: The random function cannot pick from an empty array in "content.twig" at line 4.