From 0d283d304a0bb6c368548787eb293e29a8cd50d1 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Sat, 4 Jul 2026 11:11:00 +0200 Subject: [PATCH] Throw a SyntaxError instead of a PHP fatal error when a macro argument is defined twice --- CHANGELOG | 2 +- src/Node/Expression/TempNameExpression.php | 6 +++++- src/Node/MacroNode.php | 19 ++++++++++++++----- tests/Fixtures/macros/duplicate_argument.test | 8 ++++++++ .../macros/duplicate_reserved_argument.test | 8 ++++++++ 5 files changed, 36 insertions(+), 7 deletions(-) create mode 100644 tests/Fixtures/macros/duplicate_argument.test create mode 100644 tests/Fixtures/macros/duplicate_reserved_argument.test diff --git a/CHANGELOG b/CHANGELOG index d21184696..c930ffc2b 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,6 @@ # 3.28.1 (2026-XX-XX) - * n/a + * Fix duplicated macro argument names triggering a PHP fatal error instead of a `SyntaxError` # 3.28.0 (2026-07-03) diff --git a/src/Node/Expression/TempNameExpression.php b/src/Node/Expression/TempNameExpression.php index f996aab05..f6e5f5513 100644 --- a/src/Node/Expression/TempNameExpression.php +++ b/src/Node/Expression/TempNameExpression.php @@ -18,6 +18,10 @@ class TempNameExpression extends AbstractExpression { public const RESERVED_NAMES = ['varargs', 'context', 'macros', 'blocks', 'this']; + // Prefix applied to reserved names so their compiled PHP variables cannot clash + // with the internal ones ($varargs, $context, $macros, $blocks, $this) + public const RESERVED_NAME_PREFIX = "\u{035C}"; + public function __construct(string|int|null $name, int $lineno) { // All names supported by ExpressionParser::parsePrimaryExpression() should be excluded @@ -32,7 +36,7 @@ class TempNameExpression extends AbstractExpression if (null !== $name && (\is_int($name) || ctype_digit($name))) { $name = (int) $name; } elseif (\in_array($name, self::RESERVED_NAMES, true)) { - $name = "\u{035C}".$name; + $name = self::RESERVED_NAME_PREFIX.$name; } parent::__construct([], ['name' => $name], $lineno); diff --git a/src/Node/MacroNode.php b/src/Node/MacroNode.php index db3ca458c..614a0e535 100644 --- a/src/Node/MacroNode.php +++ b/src/Node/MacroNode.php @@ -15,6 +15,7 @@ use Twig\Attribute\YieldReady; use Twig\Compiler; use Twig\Error\SyntaxError; use Twig\Node\Expression\ArrayExpression; +use Twig\Node\Expression\TempNameExpression; use Twig\Node\Expression\Variable\LocalVariable; /** @@ -47,10 +48,16 @@ class MacroNode extends Node $arguments = $args; } + $seen = []; foreach ($arguments->getKeyValuePairs() as $pair) { - if ("\u{035C}".self::VARARGS_NAME === $pair['key']->getAttribute('name')) { + $argName = $pair['key']->getAttribute('name'); + if (TempNameExpression::RESERVED_NAME_PREFIX.self::VARARGS_NAME === $argName) { 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), $pair['value']->getTemplateLine(), $pair['value']->getSourceContext()); } + if (isset($seen[$argName])) { + throw new SyntaxError(\sprintf('Argument "%s" is defined twice for macro "%s".', $this->stripReservedPrefix($argName), $name), $pair['value']->getTemplateLine(), $pair['value']->getSourceContext()); + } + $seen[$argName] = true; } parent::__construct(['body' => $body, 'arguments' => $arguments], ['name' => $name], $lineno); @@ -88,10 +95,7 @@ class MacroNode extends Node foreach ($arguments->getKeyValuePairs() as $pair) { $name = $pair['key']; - $var = $name->getAttribute('name'); - if (str_starts_with($var, "\u{035C}")) { - $var = substr($var, \strlen("\u{035C}")); - } + $var = $this->stripReservedPrefix($name->getAttribute('name')); $compiler ->write('') ->string($var) @@ -118,4 +122,9 @@ class MacroNode extends Node ->write("}\n\n") ; } + + private function stripReservedPrefix(string $name): string + { + return str_starts_with($name, TempNameExpression::RESERVED_NAME_PREFIX) ? substr($name, \strlen(TempNameExpression::RESERVED_NAME_PREFIX)) : $name; + } } diff --git a/tests/Fixtures/macros/duplicate_argument.test b/tests/Fixtures/macros/duplicate_argument.test new file mode 100644 index 000000000..fa99891ee --- /dev/null +++ b/tests/Fixtures/macros/duplicate_argument.test @@ -0,0 +1,8 @@ +--TEST-- +macro with a duplicated argument name +--TEMPLATE-- +{% macro test(a, b, a) %}{% endmacro %} +--DATA-- +return [] +--EXCEPTION-- +Twig\Error\SyntaxError: Argument "a" is defined twice for macro "test" in "index.twig" at line 2. diff --git a/tests/Fixtures/macros/duplicate_reserved_argument.test b/tests/Fixtures/macros/duplicate_reserved_argument.test new file mode 100644 index 000000000..0ca060531 --- /dev/null +++ b/tests/Fixtures/macros/duplicate_reserved_argument.test @@ -0,0 +1,8 @@ +--TEST-- +macro with a duplicated argument name matching a reserved variable name +--TEMPLATE-- +{% macro test(context, context) %}{% endmacro %} +--DATA-- +return [] +--EXCEPTION-- +Twig\Error\SyntaxError: Argument "context" is defined twice for macro "test" in "index.twig" at line 2.