From c599125a39f05d43fb40c1c93a897b29d234b499 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Thu, 29 Aug 2024 15:13:33 +0200 Subject: [PATCH 1/9] Unify error messages when a filter expects a mapping/sequence --- src/Extension/CoreExtension.php | 24 +++++++++---------- .../exception_in_extension_extends.test | 2 +- .../exception_in_extension_include.test | 2 +- .../Fixtures/filters/replace_invalid_arg.test | 2 +- .../functions/cycle_empty_mapping.test | 2 +- .../functions/cycle_empty_sequence.test | 2 +- 6 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/Extension/CoreExtension.php b/src/Extension/CoreExtension.php index c8e854658..4a5bf233f 100644 --- a/src/Extension/CoreExtension.php +++ b/src/Extension/CoreExtension.php @@ -366,7 +366,7 @@ final class CoreExtension extends AbstractExtension } if (!$count = \count($values)) { - throw new RuntimeError('The "cycle" function does not work on empty sequences.'); + throw new RuntimeError('The "cycle" function expects a non-empty sequence.'); } return $values[$position % $count]; @@ -436,7 +436,7 @@ final class CoreExtension extends AbstractExtension $values = self::toArray($values); if (0 === \count($values)) { - throw new RuntimeError('The "random" function cannot pick from an empty sequence/mapping.'); + throw new RuntimeError('The "random" function cannot pick from an empty sequence or mapping.'); } return $values[array_rand($values, 1)]; @@ -573,7 +573,7 @@ final class CoreExtension extends AbstractExtension public static function replace($str, $from): string { if (!is_iterable($from)) { - throw new RuntimeError(\sprintf('The "replace" filter expects a sequence/mapping or "Traversable" as replace values, got "%s".', get_debug_type($from))); + throw new RuntimeError(\sprintf('The "replace" filter expects a sequence or a mapping, got "%s".', get_debug_type($from))); } return strtr($str ?? '', self::toArray($from)); @@ -670,7 +670,7 @@ final class CoreExtension extends AbstractExtension foreach ($arrays as $argNumber => $array) { if (!is_iterable($array)) { - throw new RuntimeError(\sprintf('The "merge" filter only works with sequences/mappings or "Traversable", got "%s" for argument %d.', get_debug_type($array), $argNumber + 1)); + throw new RuntimeError(\sprintf('The "merge" filter expects a sequence or a mapping, got "%s" for argument %d.', get_debug_type($array), $argNumber + 1)); } $result = array_merge($result, self::toArray($array)); @@ -977,7 +977,7 @@ final class CoreExtension extends AbstractExtension if ($array instanceof \Traversable) { $array = iterator_to_array($array); } elseif (!\is_array($array)) { - throw new RuntimeError(\sprintf('The "sort" filter only works with sequences/mappings or "Traversable", got "%s".', get_debug_type($array))); + throw new RuntimeError(\sprintf('The "sort" filter expects a sequence or a mapping, got "%s".', get_debug_type($array))); } if (null !== $arrow) { @@ -1577,7 +1577,7 @@ final class CoreExtension extends AbstractExtension public static function batch($items, $size, $fill = null, $preserveKeys = true): array { if (!is_iterable($items)) { - throw new RuntimeError(\sprintf('The "batch" filter expects a sequence/mapping or "Traversable", got "%s".', get_debug_type($items))); + throw new RuntimeError(\sprintf('The "batch" filter expects a sequence or a mapping, got "%s".', get_debug_type($items))); } $size = (int) ceil($size); @@ -1824,7 +1824,7 @@ final class CoreExtension extends AbstractExtension public static function column($array, $name, $index = null): array { if (!is_iterable($array)) { - throw new RuntimeError(\sprintf('The "column" filter only works with sequences/mappings or "Traversable", got "%s" as first argument.', get_debug_type($array))); + throw new RuntimeError(\sprintf('The "column" filter expects a sequence or a mapping, got "%s".', get_debug_type($array))); } if ($array instanceof \Traversable) { @@ -1859,7 +1859,7 @@ final class CoreExtension extends AbstractExtension public static function find(Environment $env, $array, $arrow) { if (!is_iterable($array)) { - throw new RuntimeError(\sprintf('The "find" filter expects a sequence/mapping or "Traversable", got "%s".', get_debug_type($array))); + throw new RuntimeError(\sprintf('The "find" filter expects a sequence or a mapping, got "%s".', get_debug_type($array))); } self::checkArrowInSandbox($env, $arrow, 'find', 'filter'); @@ -1879,7 +1879,7 @@ final class CoreExtension extends AbstractExtension public static function map(Environment $env, $array, $arrow) { if (!is_iterable($array)) { - throw new RuntimeError(\sprintf('The "map" filter expects a sequence/mapping or "Traversable", got "%s".', get_debug_type($array))); + throw new RuntimeError(\sprintf('The "map" filter expects a sequence or a mapping, got "%s".', get_debug_type($array))); } self::checkArrowInSandbox($env, $arrow, 'map', 'filter'); @@ -1898,7 +1898,7 @@ final class CoreExtension extends AbstractExtension public static function reduce(Environment $env, $array, $arrow, $initial = null) { if (!is_iterable($array)) { - throw new RuntimeError(\sprintf('The "reduce" filter only works with sequences/mappings or "Traversable", got "%s" as first argument.', get_debug_type($array))); + throw new RuntimeError(\sprintf('The "reduce" filter expects a sequence or a mapping, got "%s".', get_debug_type($array))); } self::checkArrowInSandbox($env, $arrow, 'reduce', 'filter'); @@ -1917,7 +1917,7 @@ final class CoreExtension extends AbstractExtension public static function arraySome(Environment $env, $array, $arrow) { if (!is_iterable($array)) { - throw new RuntimeError(\sprintf('The "has some" filter only works with sequences/mappings or "Traversable", got "%s" as first argument.', get_debug_type($array))); + throw new RuntimeError(\sprintf('The "has some" test expects a sequence or a mapping, got "%s".', get_debug_type($array))); } self::checkArrowInSandbox($env, $arrow, 'has some', 'operator'); @@ -1937,7 +1937,7 @@ final class CoreExtension extends AbstractExtension public static function arrayEvery(Environment $env, $array, $arrow) { if (!is_iterable($array)) { - throw new RuntimeError(\sprintf('The "has every" filter only works with sequences/mappings or "Traversable", got "%s" as first argument.', get_debug_type($array))); + throw new RuntimeError(\sprintf('The "has every" test expects a sequence or a mapping, got "%s".', get_debug_type($array))); } self::checkArrowInSandbox($env, $arrow, 'has every', 'operator'); diff --git a/tests/Fixtures/exceptions/exception_in_extension_extends.test b/tests/Fixtures/exceptions/exception_in_extension_extends.test index d3361bf09..3b9ddeec8 100644 --- a/tests/Fixtures/exceptions/exception_in_extension_extends.test +++ b/tests/Fixtures/exceptions/exception_in_extension_extends.test @@ -9,4 +9,4 @@ Exception thrown from a child for an extension error --DATA-- return [] --EXCEPTION-- -Twig\Error\RuntimeError: The "random" function cannot pick from an empty sequence/mapping in "base.twig" at line 4. +Twig\Error\RuntimeError: The "random" function cannot pick from an empty sequence or mapping in "base.twig" at line 4. diff --git a/tests/Fixtures/exceptions/exception_in_extension_include.test b/tests/Fixtures/exceptions/exception_in_extension_include.test index be25db3b8..42927c2f6 100644 --- a/tests/Fixtures/exceptions/exception_in_extension_include.test +++ b/tests/Fixtures/exceptions/exception_in_extension_include.test @@ -9,4 +9,4 @@ Exception thrown from an include for an extension error --DATA-- return [] --EXCEPTION-- -Twig\Error\RuntimeError: The "random" function cannot pick from an empty sequence/mapping in "content.twig" at line 4. +Twig\Error\RuntimeError: The "random" function cannot pick from an empty sequence or mapping in "content.twig" at line 4. diff --git a/tests/Fixtures/filters/replace_invalid_arg.test b/tests/Fixtures/filters/replace_invalid_arg.test index ea1632500..3b1429c90 100644 --- a/tests/Fixtures/filters/replace_invalid_arg.test +++ b/tests/Fixtures/filters/replace_invalid_arg.test @@ -5,4 +5,4 @@ Exception for invalid argument type in replace call --DATA-- return ['stdClass' => new \stdClass()] --EXCEPTION-- -Twig\Error\RuntimeError: The "replace" filter expects a sequence/mapping or "Traversable" as replace values, got "stdClass" in "index.twig" at line 2. +Twig\Error\RuntimeError: The "replace" filter expects a sequence or a mapping, got "stdClass" in "index.twig" at line 2. diff --git a/tests/Fixtures/functions/cycle_empty_mapping.test b/tests/Fixtures/functions/cycle_empty_mapping.test index ca241d8f3..65b1d9491 100644 --- a/tests/Fixtures/functions/cycle_empty_mapping.test +++ b/tests/Fixtures/functions/cycle_empty_mapping.test @@ -5,4 +5,4 @@ --DATA-- return [] --EXCEPTION-- -Twig\Error\RuntimeError: The "cycle" function does not work on empty sequences in "index.twig" at line 2. +Twig\Error\RuntimeError: The "cycle" function expects a non-empty sequence in "index.twig" at line 2. diff --git a/tests/Fixtures/functions/cycle_empty_sequence.test b/tests/Fixtures/functions/cycle_empty_sequence.test index 846913b20..5d60bb1fa 100644 --- a/tests/Fixtures/functions/cycle_empty_sequence.test +++ b/tests/Fixtures/functions/cycle_empty_sequence.test @@ -5,4 +5,4 @@ --DATA-- return [] --EXCEPTION-- -Twig\Error\RuntimeError: The "cycle" function does not work on empty sequences in "index.twig" at line 2. +Twig\Error\RuntimeError: The "cycle" function expects a non-empty sequence in "index.twig" at line 2. From 4e08e3ef5f83f49877591351451721225c741a1f Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Mon, 30 Sep 2024 08:41:48 +0200 Subject: [PATCH 2/9] Fix tests --- extra/html-extra/HtmlExtension.php | 4 ++-- .../Tests/Fixtures/html_classes_with_unsupported_arg.test | 2 +- .../Tests/Fixtures/html_classes_with_unsupported_key.test | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/extra/html-extra/HtmlExtension.php b/extra/html-extra/HtmlExtension.php index 4231c6aa5..d8a6c0036 100644 --- a/extra/html-extra/HtmlExtension.php +++ b/extra/html-extra/HtmlExtension.php @@ -97,7 +97,7 @@ final class HtmlExtension extends AbstractExtension } elseif (\is_array($arg)) { foreach ($arg as $class => $condition) { if (!\is_string($class)) { - throw new RuntimeError(\sprintf('The html_classes function argument %d (key %d) should be a string, got "%s".', $i, $class, get_debug_type($class))); + throw new RuntimeError(\sprintf('The "html_classes" function argument %d (key %d) should be a string, got "%s".', $i, $class, get_debug_type($class))); } if (!$condition) { continue; @@ -105,7 +105,7 @@ final class HtmlExtension extends AbstractExtension $classes[] = $class; } } else { - throw new RuntimeError(\sprintf('The html_classes function argument %d should be either a string or an array, got "%s".', $i, get_debug_type($arg))); + throw new RuntimeError(\sprintf('The "html_classes" function argument %d should be either a string or an array, got "%s".', $i, get_debug_type($arg))); } } diff --git a/extra/html-extra/Tests/Fixtures/html_classes_with_unsupported_arg.test b/extra/html-extra/Tests/Fixtures/html_classes_with_unsupported_arg.test index 85faed624..21ca373f8 100644 --- a/extra/html-extra/Tests/Fixtures/html_classes_with_unsupported_arg.test +++ b/extra/html-extra/Tests/Fixtures/html_classes_with_unsupported_arg.test @@ -5,4 +5,4 @@ --DATA-- return [] --EXCEPTION-- -Twig\Error\RuntimeError: The html_classes function argument 0 should be either a string or an array, got "boolean" in "index.twig" at line 2. +Twig\Error\RuntimeError: The "html_classes" function argument 0 should be either a string or an array, got "bool" in "index.twig" at line 2. diff --git a/extra/html-extra/Tests/Fixtures/html_classes_with_unsupported_key.test b/extra/html-extra/Tests/Fixtures/html_classes_with_unsupported_key.test index b74748c58..708cb255b 100644 --- a/extra/html-extra/Tests/Fixtures/html_classes_with_unsupported_key.test +++ b/extra/html-extra/Tests/Fixtures/html_classes_with_unsupported_key.test @@ -5,4 +5,4 @@ --DATA-- return [] --EXCEPTION-- -Twig\Error\RuntimeError: The html_classes function argument 0 (key 0) should be a string, got "integer" in "index.twig" at line 2. +Twig\Error\RuntimeError: The "html_classes" function argument 0 (key 0) should be a string, got "int" in "index.twig" at line 2. From d133ab7e708b75e320efaf6039b827e7e94306fc Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Mon, 30 Sep 2024 11:52:45 +0200 Subject: [PATCH 3/9] Add support for detecting if an expression had explicit parentheses --- src/ExpressionParser.php | 9 ++------- src/Node/Expression/AbstractExpression.php | 15 +++++++++++++++ src/Node/Expression/Unary/AbstractUnary.php | 4 ++-- 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/ExpressionParser.php b/src/ExpressionParser.php index 97e3ae003..33da2b29e 100644 --- a/src/ExpressionParser.php +++ b/src/ExpressionParser.php @@ -169,15 +169,10 @@ class ExpressionParser return $this->parsePostfixExpression(new $class($expr, $token->getLine())); } elseif ($token->test(Token::PUNCTUATION_TYPE, '(')) { $this->parser->getStream()->next(); - $expr = $this->parseExpression(); + $expr = $this->parseExpression()->setExplicitParentheses(); $this->parser->getStream()->expect(Token::PUNCTUATION_TYPE, ')', 'An opened parenthesis is not properly closed'); - $expr = $this->parsePostfixExpression($expr); - if ($expr instanceof NegUnary) { - $expr->wrapInParentheses(); - } - - return $expr; + return $this->parsePostfixExpression($expr); } return $this->parsePrimaryExpression(); diff --git a/src/Node/Expression/AbstractExpression.php b/src/Node/Expression/AbstractExpression.php index 1692f5671..22d8617cd 100644 --- a/src/Node/Expression/AbstractExpression.php +++ b/src/Node/Expression/AbstractExpression.php @@ -25,4 +25,19 @@ abstract class AbstractExpression extends Node { return $this->hasAttribute('is_generator') && $this->getAttribute('is_generator'); } + + /** + * @return static + */ + public function setExplicitParentheses(): self + { + $this->setAttribute('with_parentheses', true); + + return $this; + } + + public function hasExplicitParentheses(): bool + { + return $this->hasAttribute('with_parentheses') && $this->getAttribute('with_parentheses'); + } } diff --git a/src/Node/Expression/Unary/AbstractUnary.php b/src/Node/Expression/Unary/AbstractUnary.php index b9b44ed4f..2482d1e70 100644 --- a/src/Node/Expression/Unary/AbstractUnary.php +++ b/src/Node/Expression/Unary/AbstractUnary.php @@ -30,14 +30,14 @@ abstract class AbstractUnary extends AbstractExpression public function compile(Compiler $compiler): void { - if ($this->getAttribute('with_parentheses')) { + if ($this->hasExplicitParentheses()) { $compiler->raw('('); } else { $compiler->raw(' '); } $this->operator($compiler); $compiler->subcompile($this->getNode('node')); - if ($this->getAttribute('with_parentheses')) { + if ($this->hasExplicitParentheses()) { $compiler->raw(')'); } } From b46ca43f6168d6c91f4fb6acfc4fc42a88f1475c Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Mon, 30 Sep 2024 12:00:38 +0200 Subject: [PATCH 4/9] Remove obsolete method --- src/Node/Expression/Unary/AbstractUnary.php | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/Node/Expression/Unary/AbstractUnary.php b/src/Node/Expression/Unary/AbstractUnary.php index 2482d1e70..e6943272e 100644 --- a/src/Node/Expression/Unary/AbstractUnary.php +++ b/src/Node/Expression/Unary/AbstractUnary.php @@ -23,11 +23,6 @@ abstract class AbstractUnary extends AbstractExpression parent::__construct(['node' => $node], ['with_parentheses' => false], $lineno); } - public function wrapInParentheses(): void - { - $this->setAttribute('with_parentheses', true); - } - public function compile(Compiler $compiler): void { if ($this->hasExplicitParentheses()) { From 824143d5e59a2639c4502dd7e962925a6d4d760b Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Mon, 30 Sep 2024 10:55:51 +0200 Subject: [PATCH 5/9] Deprecate not passing AbstractExpression args to most constructor arguments for classes extending AbstractExpression --- CHANGELOG | 1 + doc/deprecated.rst | 14 ++++++++++++++ src/Node/Expression/Binary/AbstractBinary.php | 11 +++++++++++ src/Node/Expression/BlockReferenceExpression.php | 7 +++++++ src/Node/Expression/Filter/DefaultFilter.php | 8 ++++++++ src/Node/Expression/Filter/RawFilter.php | 8 ++++++++ src/Node/Expression/FilterExpression.php | 7 +++++++ src/Node/Expression/InlinePrint.php | 7 +++++++ src/Node/Expression/NullCoalesceExpression.php | 11 +++++++++++ src/Node/Expression/Test/DefinedTest.php | 8 ++++++++ src/Node/Expression/TestExpression.php | 7 +++++++ src/Node/Expression/Unary/AbstractUnary.php | 7 +++++++ 12 files changed, 96 insertions(+) diff --git a/CHANGELOG b/CHANGELOG index 9edf14f51..f4a07a9c7 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,5 +1,6 @@ # 3.15.0 (2024-XX-XX) + * Deprecate not passing `AbstractExpression` args to most constructor arguments for classes extending `AbstractExpression` * Fix `power` expressions with a negative number in parenthesis (`(-1) ** 2`) * Deprecate instantiating `Node` directly. Use `EmptyNode` or `Nodes` instead. * Add support for inline comments diff --git a/doc/deprecated.rst b/doc/deprecated.rst index 2488e761d..afcec62f5 100644 --- a/doc/deprecated.rst +++ b/doc/deprecated.rst @@ -296,3 +296,17 @@ Node * Instantiating ``Twig\Node\Node`` directly is deprecated as of Twig 3.15. Use ``EmptyNode`` or ``Nodes`` instead depending on the use case. The ``Twig\Node\Node`` class will be abstract in Twig 4.0. + +* Not passing ``AbstractExpression`` arguments to the following ``Node`` class + constructors is deprecated as of Twig 3.15: + + * ``AbstractBinary`` + * ``AbstractUnary`` + * ``BlockReferenceExpression`` + * ``TestExpression`` + * ``DefinedTest`` + * ``FilterExpression`` + * ``RawFilter`` + * ``DefaultFilter`` + * ``InlinePrint`` + * ``NullCoalesceExpression`` diff --git a/src/Node/Expression/Binary/AbstractBinary.php b/src/Node/Expression/Binary/AbstractBinary.php index c424e5cc5..ccd3be44f 100644 --- a/src/Node/Expression/Binary/AbstractBinary.php +++ b/src/Node/Expression/Binary/AbstractBinary.php @@ -18,8 +18,19 @@ use Twig\Node\Node; abstract class AbstractBinary extends AbstractExpression { + /** + * @param AbstractExpression $left + * @param AbstractExpression $right + */ public function __construct(Node $left, Node $right, int $lineno) { + if (!$left instanceof AbstractExpression) { + trigger_deprecation('twig/twig', '3.15', 'Not passing a "%s" instance to the "left" argument of "%s" is deprecated ("%s" given).', AbstractExpression::class, static::class, get_class($left)); + } + if (!$right instanceof AbstractExpression) { + trigger_deprecation('twig/twig', '3.15', 'Not passing a "%s" instance to the "right" argument of "%s" is deprecated ("%s" given).', AbstractExpression::class, static::class, get_class($right)); + } + parent::__construct(['left' => $left, 'right' => $right], [], $lineno); } diff --git a/src/Node/Expression/BlockReferenceExpression.php b/src/Node/Expression/BlockReferenceExpression.php index acd231e15..ed88c6094 100644 --- a/src/Node/Expression/BlockReferenceExpression.php +++ b/src/Node/Expression/BlockReferenceExpression.php @@ -22,8 +22,15 @@ use Twig\Node\Node; */ class BlockReferenceExpression extends AbstractExpression { + /** + * @param AbstractExpression $name + */ public function __construct(Node $name, ?Node $template, int $lineno) { + if (!$name instanceof AbstractExpression) { + trigger_deprecation('twig/twig', '3.15', 'Not passing a "%s" instance to the "node" argument of "%s" is deprecated ("%s" given).', AbstractExpression::class, static::class, get_class($node)); + } + $nodes = ['name' => $name]; if (null !== $template) { $nodes['template'] = $template; diff --git a/src/Node/Expression/Filter/DefaultFilter.php b/src/Node/Expression/Filter/DefaultFilter.php index fa93e227c..a3ee66e47 100644 --- a/src/Node/Expression/Filter/DefaultFilter.php +++ b/src/Node/Expression/Filter/DefaultFilter.php @@ -15,6 +15,7 @@ use Twig\Attribute\FirstClassTwigCallableReady; use Twig\Compiler; use Twig\Extension\CoreExtension; use Twig\Node\EmptyNode; +use Twig\Node\Expression\AbstractExpression; use Twig\Node\Expression\ConditionalExpression; use Twig\Node\Expression\ConstantExpression; use Twig\Node\Expression\FilterExpression; @@ -34,9 +35,16 @@ use Twig\TwigTest; */ class DefaultFilter extends FilterExpression { + /** + * @param AbstractExpression $node + */ #[FirstClassTwigCallableReady] public function __construct(Node $node, TwigFilter|ConstantExpression $filter, Node $arguments, int $lineno) { + if (!$node instanceof AbstractExpression) { + trigger_deprecation('twig/twig', '3.15', 'Not passing a "%s" instance to the "node" argument of "%s" is deprecated ("%s" given).', AbstractExpression::class, static::class, get_class($node)); + } + if ($filter instanceof TwigFilter) { $name = $filter->getName(); $default = new FilterExpression($node, $filter, $arguments, $node->getTemplateLine()); diff --git a/src/Node/Expression/Filter/RawFilter.php b/src/Node/Expression/Filter/RawFilter.php index adb6de789..e70501e24 100644 --- a/src/Node/Expression/Filter/RawFilter.php +++ b/src/Node/Expression/Filter/RawFilter.php @@ -14,6 +14,7 @@ namespace Twig\Node\Expression\Filter; use Twig\Attribute\FirstClassTwigCallableReady; use Twig\Compiler; use Twig\Node\EmptyNode; +use Twig\Node\Expression\AbstractExpression; use Twig\Node\Expression\ConstantExpression; use Twig\Node\Expression\FilterExpression; use Twig\Node\Node; @@ -24,9 +25,16 @@ use Twig\TwigFilter; */ class RawFilter extends FilterExpression { + /** + * @param AbstractExpression $node + */ #[FirstClassTwigCallableReady] public function __construct(Node $node, TwigFilter|ConstantExpression|null $filter = null, ?Node $arguments = null, int $lineno = 0) { + if (!$node instanceof AbstractExpression) { + trigger_deprecation('twig/twig', '3.15', 'Not passing a "%s" instance to the "node" argument of "%s" is deprecated ("%s" given).', AbstractExpression::class, static::class, get_class($node)); + } + parent::__construct($node, $filter ?: new TwigFilter('raw', null, ['is_safe' => ['all']]), $arguments ?: new EmptyNode(), $lineno ?: $node->getTemplateLine()); } diff --git a/src/Node/Expression/FilterExpression.php b/src/Node/Expression/FilterExpression.php index efc91193e..31fb90f1e 100644 --- a/src/Node/Expression/FilterExpression.php +++ b/src/Node/Expression/FilterExpression.php @@ -20,9 +20,16 @@ use Twig\TwigFilter; class FilterExpression extends CallExpression { + /** + * @param AbstractExpression $node + */ #[FirstClassTwigCallableReady] public function __construct(Node $node, TwigFilter|ConstantExpression $filter, Node $arguments, int $lineno) { + if (!$node instanceof AbstractExpression) { + trigger_deprecation('twig/twig', '3.15', 'Not passing a "%s" instance to the "node" argument of "%s" is deprecated ("%s" given).', AbstractExpression::class, static::class, get_class($node)); + } + if ($filter instanceof TwigFilter) { $name = $filter->getName(); $filterName = new ConstantExpression($name, $lineno); diff --git a/src/Node/Expression/InlinePrint.php b/src/Node/Expression/InlinePrint.php index 0a3c2e4f9..82d17d626 100644 --- a/src/Node/Expression/InlinePrint.php +++ b/src/Node/Expression/InlinePrint.php @@ -19,8 +19,15 @@ use Twig\Node\Node; */ final class InlinePrint extends AbstractExpression { + /** + * @param AbstractExpression $node + */ public function __construct(Node $node, int $lineno) { + if (!$node instanceof AbstractExpression) { + trigger_deprecation('twig/twig', '3.15', 'Not passing a "%s" instance to the "node" argument of "%s" is deprecated ("%s" given).', AbstractExpression::class, static::class, get_class($node)); + } + parent::__construct(['node' => $node], [], $lineno); } diff --git a/src/Node/Expression/NullCoalesceExpression.php b/src/Node/Expression/NullCoalesceExpression.php index dd2384f95..1a5d90286 100644 --- a/src/Node/Expression/NullCoalesceExpression.php +++ b/src/Node/Expression/NullCoalesceExpression.php @@ -22,8 +22,19 @@ use Twig\TwigTest; class NullCoalesceExpression extends ConditionalExpression { + /** + * @param AbstractExpression $left + * @param AbstractExpression $right + */ public function __construct(Node $left, Node $right, int $lineno) { + if (!$left instanceof AbstractExpression) { + trigger_deprecation('twig/twig', '3.15', 'Not passing a "%s" instance to the "left" argument of "%s" is deprecated ("%s" given).', AbstractExpression::class, static::class, get_class($left)); + } + if (!$right instanceof AbstractExpression) { + trigger_deprecation('twig/twig', '3.15', 'Not passing a "%s" instance to the "right" argument of "%s" is deprecated ("%s" given).', AbstractExpression::class, static::class, get_class($right)); + } + $test = new DefinedTest(clone $left, new TwigTest('defined'), new EmptyNode(), $left->getTemplateLine()); // for "block()", we don't need the null test as the return value is always a string if (!$left instanceof BlockReferenceExpression) { diff --git a/src/Node/Expression/Test/DefinedTest.php b/src/Node/Expression/Test/DefinedTest.php index 24d3ee82c..005ba39cc 100644 --- a/src/Node/Expression/Test/DefinedTest.php +++ b/src/Node/Expression/Test/DefinedTest.php @@ -14,6 +14,7 @@ namespace Twig\Node\Expression\Test; use Twig\Attribute\FirstClassTwigCallableReady; use Twig\Compiler; use Twig\Error\SyntaxError; +use Twig\Node\Expression\AbstractExpression; use Twig\Node\Expression\ArrayExpression; use Twig\Node\Expression\BlockReferenceExpression; use Twig\Node\Expression\ConstantExpression; @@ -37,9 +38,16 @@ use Twig\TwigTest; */ class DefinedTest extends TestExpression { + /** + * @param AbstractExpression $node + */ #[FirstClassTwigCallableReady] public function __construct(Node $node, TwigTest|string $name, ?Node $arguments, int $lineno) { + if (!$node instanceof AbstractExpression) { + trigger_deprecation('twig/twig', '3.15', 'Not passing a "%s" instance to the "node" argument of "%s" is deprecated ("%s" given).', AbstractExpression::class, static::class, get_class($node)); + } + if ($node instanceof NameExpression) { $node->setAttribute('is_defined_test', true); } elseif ($node instanceof GetAttrExpression) { diff --git a/src/Node/Expression/TestExpression.php b/src/Node/Expression/TestExpression.php index 080d85aaa..3ad6ac557 100644 --- a/src/Node/Expression/TestExpression.php +++ b/src/Node/Expression/TestExpression.php @@ -20,8 +20,15 @@ use Twig\TwigTest; class TestExpression extends CallExpression { #[FirstClassTwigCallableReady] + /** + * @param AbstractExpression $node + */ public function __construct(Node $node, string|TwigTest $test, ?Node $arguments, int $lineno) { + if (!$node instanceof AbstractExpression) { + trigger_deprecation('twig/twig', '3.15', 'Not passing a "%s" instance to the "node" argument of "%s" is deprecated ("%s" given).', AbstractExpression::class, static::class, get_class($node)); + } + $nodes = ['node' => $node]; if (null !== $arguments) { $nodes['arguments'] = $arguments; diff --git a/src/Node/Expression/Unary/AbstractUnary.php b/src/Node/Expression/Unary/AbstractUnary.php index b9b44ed4f..eeb6af24c 100644 --- a/src/Node/Expression/Unary/AbstractUnary.php +++ b/src/Node/Expression/Unary/AbstractUnary.php @@ -18,8 +18,15 @@ use Twig\Node\Node; abstract class AbstractUnary extends AbstractExpression { + /** + * @param AbstractExpression $node + */ public function __construct(Node $node, int $lineno) { + if (!$node instanceof AbstractExpression) { + trigger_deprecation('twig/twig', '3.15', 'Not passing a "%s" instance argument to "%s" is deprecated ("%s" given).', AbstractExpression::class, static::class, get_class($node)); + } + parent::__construct(['node' => $node], ['with_parentheses' => false], $lineno); } From 43b6e098df51fe8e163e9e335d388e3a116757d5 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Mon, 30 Sep 2024 13:47:10 +0200 Subject: [PATCH 6/9] Change some type hints to be more precise --- src/NodeVisitor/EscaperNodeVisitor.php | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/NodeVisitor/EscaperNodeVisitor.php b/src/NodeVisitor/EscaperNodeVisitor.php index a4a415c9a..c942f825b 100644 --- a/src/NodeVisitor/EscaperNodeVisitor.php +++ b/src/NodeVisitor/EscaperNodeVisitor.php @@ -17,6 +17,7 @@ use Twig\Node\AutoEscapeNode; use Twig\Node\BlockNode; use Twig\Node\BlockReferenceNode; use Twig\Node\DoNode; +use Twig\Node\Expression\AbstractExpression; use Twig\Node\Expression\ConditionalExpression; use Twig\Node\Expression\ConstantExpression; use Twig\Node\Expression\FilterExpression; @@ -94,8 +95,13 @@ final class EscaperNodeVisitor implements NodeVisitorInterface private function shouldUnwrapConditional(ConditionalExpression $expression, Environment $env, string $type): bool { - $expr2Safe = $this->isSafeFor($type, $expression->getNode('expr2'), $env); - $expr3Safe = $this->isSafeFor($type, $expression->getNode('expr3'), $env); + /** @var AbstractExpression $expr2 */ + $expr2 = $expression->getNode('expr2'); + /** @var AbstractExpression $expr3 */ + $expr3 = $expression->getNode('expr3'); + + $expr2Safe = $this->isSafeFor($type, $expr2, $env); + $expr3Safe = $this->isSafeFor($type, $expr3, $env); return $expr2Safe !== $expr3Safe; } @@ -103,12 +109,14 @@ final class EscaperNodeVisitor implements NodeVisitorInterface private function unwrapConditional(ConditionalExpression $expression, Environment $env, string $type): ConditionalExpression { // convert "echo a ? b : c" to "a ? echo b : echo c" recursively + /** @var AbstractExpression $expr2 */ $expr2 = $expression->getNode('expr2'); if ($expr2 instanceof ConditionalExpression && $this->shouldUnwrapConditional($expr2, $env, $type)) { $expr2 = $this->unwrapConditional($expr2, $env, $type); } else { $expr2 = $this->escapeInlinePrintNode(new InlinePrint($expr2, $expr2->getTemplateLine()), $env, $type); } + /** @var AbstractExpression $expr3 */ $expr3 = $expression->getNode('expr3'); if ($expr3 instanceof ConditionalExpression && $this->shouldUnwrapConditional($expr3, $env, $type)) { $expr3 = $this->unwrapConditional($expr3, $env, $type); @@ -116,11 +124,15 @@ final class EscaperNodeVisitor implements NodeVisitorInterface $expr3 = $this->escapeInlinePrintNode(new InlinePrint($expr3, $expr3->getTemplateLine()), $env, $type); } - return new ConditionalExpression($expression->getNode('expr1'), $expr2, $expr3, $expression->getTemplateLine()); + /** @var AbstractExpression $expr1 */ + $expr1 = $expression->getNode('expr1'); + + return new ConditionalExpression($expr1, $expr2, $expr3, $expression->getTemplateLine()); } - private function escapeInlinePrintNode(InlinePrint $node, Environment $env, string $type): Node + private function escapeInlinePrintNode(InlinePrint $node, Environment $env, string $type): AbstractExpression { + /** @var AbstractExpression $expression */ $expression = $node->getNode('node'); if ($this->isSafeFor($type, $expression, $env)) { @@ -132,6 +144,7 @@ final class EscaperNodeVisitor implements NodeVisitorInterface private function escapePrintNode(PrintNode $node, Environment $env, string $type): Node { + /** @var AbstractExpression $expression */ $expression = $node->getNode('expr'); if ($this->isSafeFor($type, $expression, $env)) { @@ -157,6 +170,7 @@ final class EscaperNodeVisitor implements NodeVisitorInterface return $filter; } + /** @var AbstractExpression $node */ $node = $filter->getNode('node'); if ($this->isSafeFor($type, $node, $env)) { return $filter; @@ -167,7 +181,7 @@ final class EscaperNodeVisitor implements NodeVisitorInterface return $filter; } - private function isSafeFor(string $type, Node $expression, Environment $env): bool + private function isSafeFor(string $type, AbstractExpression $expression, Environment $env): bool { $safe = $this->safeAnalysis->getSafe($expression); @@ -194,7 +208,7 @@ final class EscaperNodeVisitor implements NodeVisitorInterface return $this->defaultStrategy ?: false; } - private function getEscaperFilter(Environment $env, string $type, Node $node): FilterExpression + private function getEscaperFilter(Environment $env, string $type, AbstractExpression $node): FilterExpression { $line = $node->getTemplateLine(); $filter = $env->getFilter('escape'); From 816c510cef96d99cf489b08f742992c4fdcb0b03 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Sun, 29 Sep 2024 22:59:41 +0200 Subject: [PATCH 7/9] =?UTF-8?q?Deprecate=20using=20~=C2=A0with=20+=20or=20?= =?UTF-8?q?-=20in=20an=20expression=20without=20using=20parentheses=20to?= =?UTF-8?q?=20clarify=20precedence?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- CHANGELOG | 1 + doc/deprecated.rst | 21 +++++++++++++++++++ src/ExpressionParser.php | 14 +++++++++++++ src/Extension/CoreExtension.php | 1 + .../Fixtures/operators/concat_vs_add_sub.test | 16 ++++++++++++++ .../operators/contat_vs_add_sub.legacy.test | 13 ++++++++++++ 6 files changed, 66 insertions(+) create mode 100644 tests/Fixtures/operators/concat_vs_add_sub.test create mode 100644 tests/Fixtures/operators/contat_vs_add_sub.legacy.test diff --git a/CHANGELOG b/CHANGELOG index f4a07a9c7..6100fefe9 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,5 +1,6 @@ # 3.15.0 (2024-XX-XX) + * Deprecate using `~` with `+` or `-` in an expression without using parentheses to clarify precedence * Deprecate not passing `AbstractExpression` args to most constructor arguments for classes extending `AbstractExpression` * Fix `power` expressions with a negative number in parenthesis (`(-1) ** 2`) * Deprecate instantiating `Node` directly. Use `EmptyNode` or `Nodes` instead. diff --git a/doc/deprecated.rst b/doc/deprecated.rst index afcec62f5..10cc1214c 100644 --- a/doc/deprecated.rst +++ b/doc/deprecated.rst @@ -310,3 +310,24 @@ Node * ``DefaultFilter`` * ``InlinePrint`` * ``NullCoalesceExpression`` + +Operators +--------- + +* Using ``~`` with ``+`` or ``-`` in an expression without using parentheses to + clarify precedence is deprecated as of Twig 3.15 (in Twig 4.0, parentheses + won't be needed anymore as ``+`` / ``-`` will have a higher precedence than + ``~``). + + For example, the following expression will trigger a deprecation in Twig 3.15:: + + {{ '42' ~ 1 + 41 }} + + To avoid the deprecation, wrap the concatenation in parentheses to clarify + the precedence:: + + {{ ('42' ~ 1) + 41 }} {# this is equivalent to what Twig 3.x does without the parentheses #} + + {# or #} + + {{ '42' ~ (1 + 41) }} {# this is equivalent to what Twig 4.x will do without the parentheses #} diff --git a/src/ExpressionParser.php b/src/ExpressionParser.php index 33da2b29e..db6b15550 100644 --- a/src/ExpressionParser.php +++ b/src/ExpressionParser.php @@ -20,7 +20,9 @@ use Twig\Node\Expression\ArrayExpression; use Twig\Node\Expression\ArrowFunctionExpression; use Twig\Node\Expression\AssignNameExpression; use Twig\Node\Expression\Binary\AbstractBinary; +use Twig\Node\Expression\Binary\AddBinary; use Twig\Node\Expression\Binary\ConcatBinary; +use Twig\Node\Expression\Binary\SubBinary; use Twig\Node\Expression\ConditionalExpression; use Twig\Node\Expression\ConstantExpression; use Twig\Node\Expression\GetAttrExpression; @@ -91,6 +93,18 @@ class ExpressionParser $token = $this->parser->getCurrentToken(); } + if ( + ($expr instanceof AddBinary || $expr instanceof SubBinary) + && + ( + ($expr->getNode('left') instanceof ConcatBinary && !$expr->getNode('left')->hasExplicitParentheses()) + || + ($expr->getNode('right') instanceof ConcatBinary && !$expr->getNode('right')->hasExplicitParentheses()) + ) + ) { + trigger_deprecation('twig/twig', '3.15', \sprintf('As "+" / "-" will have a higher precedence than "~" in Twig 4.0, please add parentheses to keep the current behavior in "%s" at line %d.', $this->parser->getStream()->getSourceContext()->getName(), $token->getLine())); + } + if (0 === $precedence) { return $this->parseConditionalExpression($expr); } diff --git a/src/Extension/CoreExtension.php b/src/Extension/CoreExtension.php index 4a5bf233f..1dc051d8b 100644 --- a/src/Extension/CoreExtension.php +++ b/src/Extension/CoreExtension.php @@ -323,6 +323,7 @@ final class CoreExtension extends AbstractExtension '..' => ['precedence' => 25, 'class' => RangeBinary::class, 'associativity' => ExpressionParser::OPERATOR_LEFT], '+' => ['precedence' => 30, 'class' => AddBinary::class, 'associativity' => ExpressionParser::OPERATOR_LEFT], '-' => ['precedence' => 30, 'class' => SubBinary::class, 'associativity' => ExpressionParser::OPERATOR_LEFT], + // Precedence of the ~ operator will change to 27 in Twig 4.0 '~' => ['precedence' => 40, 'class' => ConcatBinary::class, 'associativity' => ExpressionParser::OPERATOR_LEFT], '*' => ['precedence' => 60, 'class' => MulBinary::class, 'associativity' => ExpressionParser::OPERATOR_LEFT], '/' => ['precedence' => 60, 'class' => DivBinary::class, 'associativity' => ExpressionParser::OPERATOR_LEFT], diff --git a/tests/Fixtures/operators/concat_vs_add_sub.test b/tests/Fixtures/operators/concat_vs_add_sub.test new file mode 100644 index 000000000..298a3499b --- /dev/null +++ b/tests/Fixtures/operators/concat_vs_add_sub.test @@ -0,0 +1,16 @@ +--TEST-- ++/- will have a higher precedence over ~ in Twig 4.0 +--TEMPLATE-- +{{ 1 + 41 }} +{{ '42==' ~ '42' }} +{{ '42==' ~ (1 + 41) }} +{{ '42==' ~ (43 - 1) }} +{{ ('42' ~ 43) - 1 }} +--DATA-- +return [] +--EXPECT-- +42 +42==42 +42==42 +42==42 +4242 diff --git a/tests/Fixtures/operators/contat_vs_add_sub.legacy.test b/tests/Fixtures/operators/contat_vs_add_sub.legacy.test new file mode 100644 index 000000000..dc4a1ef50 --- /dev/null +++ b/tests/Fixtures/operators/contat_vs_add_sub.legacy.test @@ -0,0 +1,13 @@ +--TEST-- ++/- will have a higher precedence over ~ in Twig 4.0 +--DEPRECATION-- +Since twig/twig 3.15: As "+" / "-" will have a higher precedence than "~" in Twig 4.0, please add parentheses to keep the current behavior in "index.twig" at line 2. +Since twig/twig 3.15: As "+" / "-" will have a higher precedence than "~" in Twig 4.0, please add parentheses to keep the current behavior in "index.twig" at line 3. +--TEMPLATE-- +{{ '42' ~ 1 + 41 }} +{{ '42' ~ 43 - 1 }} +--DATA-- +return [] +--EXPECT-- +462 +4242 From 9f8c52d022c4180157809051c804ed39be25288e Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Mon, 30 Sep 2024 14:02:17 +0200 Subject: [PATCH 8/9] Add some type hints --- src/ExpressionParser.php | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/ExpressionParser.php b/src/ExpressionParser.php index db6b15550..23b24aee2 100644 --- a/src/ExpressionParser.php +++ b/src/ExpressionParser.php @@ -95,14 +95,18 @@ class ExpressionParser if ( ($expr instanceof AddBinary || $expr instanceof SubBinary) - && - ( - ($expr->getNode('left') instanceof ConcatBinary && !$expr->getNode('left')->hasExplicitParentheses()) - || - ($expr->getNode('right') instanceof ConcatBinary && !$expr->getNode('right')->hasExplicitParentheses()) - ) ) { - trigger_deprecation('twig/twig', '3.15', \sprintf('As "+" / "-" will have a higher precedence than "~" in Twig 4.0, please add parentheses to keep the current behavior in "%s" at line %d.', $this->parser->getStream()->getSourceContext()->getName(), $token->getLine())); + /** @var AbstractExpression $left */ + $left = $expr->getNode('left'); + /** @var AbstractExpression $right */ + $right = $expr->getNode('right'); + if ( + ($left instanceof ConcatBinary && !$left->hasExplicitParentheses()) + || + ($right instanceof ConcatBinary && !$right->hasExplicitParentheses()) + ) { + trigger_deprecation('twig/twig', '3.15', \sprintf('As "+" / "-" will have a higher precedence than "~" in Twig 4.0, please add parentheses to keep the current behavior in "%s" at line %d.', $this->parser->getStream()->getSourceContext()->getName(), $token->getLine())); + } } if (0 === $precedence) { From b93eb3ce5830ba64bcaeb0eaca02244d67d2e663 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Mon, 30 Sep 2024 14:05:03 +0200 Subject: [PATCH 9/9] Refactor code --- src/ExpressionParser.php | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/ExpressionParser.php b/src/ExpressionParser.php index 23b24aee2..ed61dcc2a 100644 --- a/src/ExpressionParser.php +++ b/src/ExpressionParser.php @@ -93,9 +93,19 @@ class ExpressionParser $token = $this->parser->getCurrentToken(); } - if ( - ($expr instanceof AddBinary || $expr instanceof SubBinary) - ) { + $this->triggerPrecedenceDeprecations($expr, $token); + + if (0 === $precedence) { + return $this->parseConditionalExpression($expr); + } + + return $expr; + } + + private function triggerPrecedenceDeprecations(AbstractExpression $expr, Token $token): void + { + // Precedence of the ~ operator will be lower than + and - in Twig 4.0 + if ($expr instanceof AddBinary || $expr instanceof SubBinary) { /** @var AbstractExpression $left */ $left = $expr->getNode('left'); /** @var AbstractExpression $right */ @@ -108,12 +118,6 @@ class ExpressionParser trigger_deprecation('twig/twig', '3.15', \sprintf('As "+" / "-" will have a higher precedence than "~" in Twig 4.0, please add parentheses to keep the current behavior in "%s" at line %d.', $this->parser->getStream()->getSourceContext()->getName(), $token->getLine())); } } - - if (0 === $precedence) { - return $this->parseConditionalExpression($expr); - } - - return $expr; } /**