From 4f8ba93600ec1f4e19b78990ffeaaa736e4d653c Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Thu, 28 Nov 2024 16:33:09 +0100 Subject: [PATCH] Enforce AbstractBinary for all binary operators --- CHANGELOG | 1 + doc/deprecated.rst | 10 ++- src/ExpressionParser.php | 22 ++--- src/Extension/CoreExtension.php | 5 +- src/Node/Expression/Binary/ElvisBinary.php | 50 +++++++++++ .../Expression/Binary/NullCoalesceBinary.php | 84 +++++++++++++++++++ src/Node/Expression/ConditionalExpression.php | 11 ++- src/Node/Expression/Filter/DefaultFilter.php | 4 +- .../Expression/NullCoalesceExpression.php | 3 + .../Expression/OperatorEscapeInterface.php | 25 ++++++ .../Expression/Ternary/ConditionalTernary.php | 42 ++++++++++ src/NodeVisitor/EscaperNodeVisitor.php | 31 ++++--- src/NodeVisitor/SafeAnalysisNodeVisitor.php | 15 ++-- src/TokenParser/TypesTokenParser.php | 8 +- .../Expression/Binary/NullCoalesceTest.php | 29 +++++++ tests/Node/Expression/ConditionalTest.php | 3 + tests/Node/Expression/NullCoalesceTest.php | 3 + .../Ternary/ConditionalTernaryTest.php | 44 ++++++++++ tests/Node/IncludeTest.php | 4 +- tests/Node/ModuleTest.php | 4 +- 20 files changed, 351 insertions(+), 47 deletions(-) create mode 100644 src/Node/Expression/Binary/ElvisBinary.php create mode 100644 src/Node/Expression/Binary/NullCoalesceBinary.php create mode 100644 src/Node/Expression/OperatorEscapeInterface.php create mode 100644 src/Node/Expression/Ternary/ConditionalTernary.php create mode 100644 tests/Node/Expression/Binary/NullCoalesceTest.php create mode 100644 tests/Node/Expression/Ternary/ConditionalTernaryTest.php diff --git a/CHANGELOG b/CHANGELOG index 79425bedeac..5b3fa9eb5a0 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -4,6 +4,7 @@ # 3.16.0 (2024-11-29) + * Deprecate `ConditionalExpression` and `NullCoalesceExpression` (use `ConditionalTernary` and `NullCoalesceBinary` instead) * Deprecate `InlinePrint` * Fix having macro variables starting with an underscore * Deprecate not passing a `Source` instance to `TokenStream` diff --git a/doc/deprecated.rst b/doc/deprecated.rst index 4b0f1bb4de9..4d4d096adbe 100644 --- a/doc/deprecated.rst +++ b/doc/deprecated.rst @@ -189,7 +189,15 @@ Nodes made ready for ``yield``; the ``use_yield`` Environment option can be turned on when all nodes use the ``#[\Twig\Attribute\YieldReady]`` attribute. - * The ``InlinePrint`` class is deprecated as of Twig 3.16 with no replacement. + * The ``Twig\Node\InlinePrint`` class is deprecated as of Twig 3.16 with no + replacement. + + * The ``Twig\Node\Expression\NullCoalesceExpression`` class is deprecated as + of Twig 3.16, use ``Twig\Node\Expression\Binary\NullCoalesceBinary`` + instead. + + * The ``Twig\Node\Expression\ConditionalExpression`` class is deprecated as of + Twig 3.16, use ``Twig\Node\Expression\Ternary\ConditionalTernary`` instead. Node Visitors ------------- diff --git a/src/ExpressionParser.php b/src/ExpressionParser.php index 2fb528518ba..3cb174de0e2 100644 --- a/src/ExpressionParser.php +++ b/src/ExpressionParser.php @@ -20,11 +20,11 @@ use Twig\Node\Expression\ArrowFunctionExpression; use Twig\Node\Expression\Binary\AbstractBinary; use Twig\Node\Expression\Binary\ConcatBinary; -use Twig\Node\Expression\ConditionalExpression; use Twig\Node\Expression\ConstantExpression; use Twig\Node\Expression\GetAttrExpression; use Twig\Node\Expression\MacroReferenceExpression; use Twig\Node\Expression\NameExpression; +use Twig\Node\Expression\Ternary\ConditionalTernary; use Twig\Node\Expression\TestExpression; use Twig\Node\Expression\Unary\AbstractUnary; use Twig\Node\Expression\Unary\NegUnary; @@ -269,22 +269,16 @@ private function getPrimary(): AbstractExpression private function parseConditionalExpression($expr): AbstractExpression { while ($this->parser->getStream()->nextIf(Token::PUNCTUATION_TYPE, '?')) { - if (!$this->parser->getStream()->nextIf(Token::PUNCTUATION_TYPE, ':')) { - $expr2 = $this->parseExpression(); - if ($this->parser->getStream()->nextIf(Token::PUNCTUATION_TYPE, ':')) { - // Ternary operator (expr ? expr2 : expr3) - $expr3 = $this->parseExpression(); - } else { - // Ternary without else (expr ? expr2) - $expr3 = new ConstantExpression('', $this->parser->getCurrentToken()->getLine()); - } - } else { - // Ternary without then (expr ?: expr3) - $expr2 = $expr; + $expr2 = $this->parseExpression(); + if ($this->parser->getStream()->nextIf(Token::PUNCTUATION_TYPE, ':')) { + // Ternary operator (expr ? expr2 : expr3) $expr3 = $this->parseExpression(); + } else { + // Ternary without else (expr ? expr2) + $expr3 = new ConstantExpression('', $this->parser->getCurrentToken()->getLine()); } - $expr = new ConditionalExpression($expr, $expr2, $expr3, $this->parser->getCurrentToken()->getLine()); + $expr = new ConditionalTernary($expr, $expr2, $expr3, $this->parser->getCurrentToken()->getLine()); } return $expr; diff --git a/src/Extension/CoreExtension.php b/src/Extension/CoreExtension.php index 429763ad143..cb54adc753d 100644 --- a/src/Extension/CoreExtension.php +++ b/src/Extension/CoreExtension.php @@ -26,6 +26,7 @@ use Twig\Node\Expression\Binary\BitwiseXorBinary; use Twig\Node\Expression\Binary\ConcatBinary; use Twig\Node\Expression\Binary\DivBinary; +use Twig\Node\Expression\Binary\ElvisBinary; use Twig\Node\Expression\Binary\EndsWithBinary; use Twig\Node\Expression\Binary\EqualBinary; use Twig\Node\Expression\Binary\FloorDivBinary; @@ -41,6 +42,7 @@ use Twig\Node\Expression\Binary\MulBinary; use Twig\Node\Expression\Binary\NotEqualBinary; use Twig\Node\Expression\Binary\NotInBinary; +use Twig\Node\Expression\Binary\NullCoalesceBinary; use Twig\Node\Expression\Binary\OrBinary; use Twig\Node\Expression\Binary\PowerBinary; use Twig\Node\Expression\Binary\RangeBinary; @@ -320,6 +322,8 @@ public function getOperators(): array '+' => ['precedence' => 500, 'class' => PosUnary::class], ], [ + '?:' => ['precedence' => 5, 'class' => ElvisBinary::class, 'associativity' => ExpressionParser::OPERATOR_RIGHT], + '??' => ['precedence' => 300, 'precedence_change' => new OperatorPrecedenceChange('twig/twig', '3.15', 5), 'class' => NullCoalesceBinary::class, 'associativity' => ExpressionParser::OPERATOR_RIGHT], 'or' => ['precedence' => 10, 'class' => OrBinary::class, 'associativity' => ExpressionParser::OPERATOR_LEFT], 'xor' => ['precedence' => 12, 'class' => XorBinary::class, 'associativity' => ExpressionParser::OPERATOR_LEFT], 'and' => ['precedence' => 15, 'class' => AndBinary::class, 'associativity' => ExpressionParser::OPERATOR_LEFT], @@ -351,7 +355,6 @@ public function getOperators(): array 'is' => ['precedence' => 100, 'associativity' => ExpressionParser::OPERATOR_LEFT], 'is not' => ['precedence' => 100, 'associativity' => ExpressionParser::OPERATOR_LEFT], '**' => ['precedence' => 200, 'class' => PowerBinary::class, 'associativity' => ExpressionParser::OPERATOR_RIGHT], - '??' => ['precedence' => 300, 'precedence_change' => new OperatorPrecedenceChange('twig/twig', '3.15', 5), 'class' => NullCoalesceExpression::class, 'associativity' => ExpressionParser::OPERATOR_RIGHT], ], ]; } diff --git a/src/Node/Expression/Binary/ElvisBinary.php b/src/Node/Expression/Binary/ElvisBinary.php new file mode 100644 index 00000000000..205d1ff45e6 --- /dev/null +++ b/src/Node/Expression/Binary/ElvisBinary.php @@ -0,0 +1,50 @@ +setNode('test', clone $left); + $left->setAttribute('always_defined', true); + } + + public function compile(Compiler $compiler): void + { + $compiler + ->raw('((') + ->subcompile($this->getNode('test')) + ->raw(') ? (') + ->subcompile($this->getNode('left')) + ->raw(') : (') + ->subcompile($this->getNode('right')) + ->raw('))') + ; + } + + public function operator(Compiler $compiler): Compiler + { + return $compiler->raw('?:'); + } + + public function getOperandNamesToEscape(): array + { + return ['left', 'right']; + } +} diff --git a/src/Node/Expression/Binary/NullCoalesceBinary.php b/src/Node/Expression/Binary/NullCoalesceBinary.php new file mode 100644 index 00000000000..6fb088fa62d --- /dev/null +++ b/src/Node/Expression/Binary/NullCoalesceBinary.php @@ -0,0 +1,84 @@ +getTemplateLine()); + // for "block()", we don't need the null test as the return value is always a string + if (!$left instanceof BlockReferenceExpression) { + $test = new AndBinary( + $test, + new NotUnary(new NullTest($left, new TwigTest('null'), new EmptyNode(), $left->getTemplateLine()), $left->getTemplateLine()), + $left->getTemplateLine(), + ); + } + + $this->setNode('test', $test); + } else { + $left->setAttribute('always_defined', true); + } + } + + public function compile(Compiler $compiler): void + { + /* + * This optimizes only one case. PHP 7 also supports more complex expressions + * that can return null. So, for instance, if log is defined, log("foo") ?? "..." works, + * but log($a["foo"]) ?? "..." does not if $a["foo"] is not defined. More advanced + * cases might be implemented as an optimizer node visitor, but has not been done + * as benefits are probably not worth the added complexity. + */ + if ($this->hasNode('test')) { + $compiler + ->raw('((') + ->subcompile($this->getNode('test')) + ->raw(') ? (') + ->subcompile($this->getNode('left')) + ->raw(') : (') + ->subcompile($this->getNode('right')) + ->raw('))') + ; + + return; + } + + parent::compile($compiler); + } + + public function operator(Compiler $compiler): Compiler + { + return $compiler->raw('??'); + } + + public function getOperandNamesToEscape(): array + { + return $this->hasNode('test') ? ['left', 'right'] : ['right']; + } +} diff --git a/src/Node/Expression/ConditionalExpression.php b/src/Node/Expression/ConditionalExpression.php index d7db993579c..69d55a35835 100644 --- a/src/Node/Expression/ConditionalExpression.php +++ b/src/Node/Expression/ConditionalExpression.php @@ -13,11 +13,15 @@ namespace Twig\Node\Expression; use Twig\Compiler; +use Twig\Node\Expression\OperatorEscapeInterface; +use Twig\Node\Expression\Ternary\ConditionalTernary; -class ConditionalExpression extends AbstractExpression +class ConditionalExpression extends AbstractExpression implements OperatorEscapeInterface { public function __construct(AbstractExpression $expr1, AbstractExpression $expr2, AbstractExpression $expr3, int $lineno) { + trigger_deprecation('twig/twig', '3.16', \sprintf('"%s" is deprecated; use "%s" instead.', __CLASS__, ConditionalTernary::class)); + parent::__construct(['expr1' => $expr1, 'expr2' => $expr2, 'expr3' => $expr3], [], $lineno); } @@ -42,4 +46,9 @@ public function compile(Compiler $compiler): void ->raw('))'); } } + + public function getOperandNamesToEscape(): array + { + return ['expr2', 'expr3']; + } } diff --git a/src/Node/Expression/Filter/DefaultFilter.php b/src/Node/Expression/Filter/DefaultFilter.php index a3ee66e47b1..60ebc6b83da 100644 --- a/src/Node/Expression/Filter/DefaultFilter.php +++ b/src/Node/Expression/Filter/DefaultFilter.php @@ -16,11 +16,11 @@ 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; use Twig\Node\Expression\GetAttrExpression; use Twig\Node\Expression\NameExpression; +use Twig\Node\Expression\Ternary\ConditionalTernary; use Twig\Node\Expression\Test\DefinedTest; use Twig\Node\Node; use Twig\TwigFilter; @@ -57,7 +57,7 @@ public function __construct(Node $node, TwigFilter|ConstantExpression $filter, N $test = new DefinedTest(clone $node, new TwigTest('defined'), new EmptyNode(), $node->getTemplateLine()); $false = \count($arguments) ? $arguments->getNode('0') : new ConstantExpression('', $node->getTemplateLine()); - $node = new ConditionalExpression($test, $default, $false, $node->getTemplateLine()); + $node = new ConditionalTernary($test, $default, $false, $node->getTemplateLine()); } else { $node = $default; } diff --git a/src/Node/Expression/NullCoalesceExpression.php b/src/Node/Expression/NullCoalesceExpression.php index 1a5d90286b7..5c9a276890b 100644 --- a/src/Node/Expression/NullCoalesceExpression.php +++ b/src/Node/Expression/NullCoalesceExpression.php @@ -14,6 +14,7 @@ use Twig\Compiler; use Twig\Node\EmptyNode; use Twig\Node\Expression\Binary\AndBinary; +use Twig\Node\Expression\Binary\NullCoalesceBinary; use Twig\Node\Expression\Test\DefinedTest; use Twig\Node\Expression\Test\NullTest; use Twig\Node\Expression\Unary\NotUnary; @@ -28,6 +29,8 @@ class NullCoalesceExpression extends ConditionalExpression */ public function __construct(Node $left, Node $right, int $lineno) { + trigger_deprecation('twig/twig', '3.16', \sprintf('"%s" is deprecated; use "%s" instead.', __CLASS__, NullCoalesceBinary::class)); + 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)); } diff --git a/src/Node/Expression/OperatorEscapeInterface.php b/src/Node/Expression/OperatorEscapeInterface.php new file mode 100644 index 00000000000..7b1e43e35a9 --- /dev/null +++ b/src/Node/Expression/OperatorEscapeInterface.php @@ -0,0 +1,25 @@ + 1. + * + * @author Fabien Potencier + */ +interface OperatorEscapeInterface +{ + /** + * @return string[] + */ + public function getOperandNamesToEscape(): array; +} diff --git a/src/Node/Expression/Ternary/ConditionalTernary.php b/src/Node/Expression/Ternary/ConditionalTernary.php new file mode 100644 index 00000000000..627da7a43cf --- /dev/null +++ b/src/Node/Expression/Ternary/ConditionalTernary.php @@ -0,0 +1,42 @@ + $test, 'left' => $left, 'right' => $right], [], $lineno); + } + + public function compile(Compiler $compiler): void + { + $compiler + ->raw('((') + ->subcompile($this->getNode('test')) + ->raw(') ? (') + ->subcompile($this->getNode('left')) + ->raw(') : (') + ->subcompile($this->getNode('right')) + ->raw('))') + ; + } + + public function getOperandNamesToEscape(): array + { + return ['left', 'right']; + } +} diff --git a/src/NodeVisitor/EscaperNodeVisitor.php b/src/NodeVisitor/EscaperNodeVisitor.php index 9640c541b01..a70726bbd85 100644 --- a/src/NodeVisitor/EscaperNodeVisitor.php +++ b/src/NodeVisitor/EscaperNodeVisitor.php @@ -17,9 +17,9 @@ use Twig\Node\BlockNode; use Twig\Node\BlockReferenceNode; use Twig\Node\Expression\AbstractExpression; -use Twig\Node\Expression\ConditionalExpression; use Twig\Node\Expression\ConstantExpression; use Twig\Node\Expression\FilterExpression; +use Twig\Node\Expression\OperatorEscapeInterface; use Twig\Node\ImportNode; use Twig\Node\ModuleNode; use Twig\Node\Node; @@ -75,7 +75,7 @@ public function leaveNode(Node $node, Environment $env): ?Node return $this->preEscapeFilterNode($node, $env); } elseif ($node instanceof PrintNode && false !== $type = $this->needEscaping()) { $expression = $node->getNode('expr'); - if ($expression instanceof ConditionalExpression) { + if ($expression instanceof OperatorEscapeInterface) { $this->escapeConditional($expression, $env, $type); } else { $node->setNode('expr', $this->escapeExpression($expression, $env, $type)); @@ -93,22 +93,19 @@ public function leaveNode(Node $node, Environment $env): ?Node return $node; } - private function escapeConditional(ConditionalExpression $expression, Environment $env, string $type): void + /** + * @param AbstractExpression&OperatorEscapeInterface $expression + */ + private function escapeConditional($expression, Environment $env, string $type): void { - /** @var AbstractExpression $expr2 */ - $expr2 = $expression->getNode('expr2'); - if ($expr2 instanceof ConditionalExpression) { - $this->escapeConditional($expr2, $env, $type); - } else { - $expression->setNode('expr2', $this->escapeExpression($expr2, $env, $type)); - } - - /** @var AbstractExpression $expr3 */ - $expr3 = $expression->getNode('expr3'); - if ($expr3 instanceof ConditionalExpression) { - $this->escapeConditional($expr3, $env, $type); - } else { - $expression->setNode('expr3', $this->escapeExpression($expr3, $env, $type)); + foreach ($expression->getOperandNamesToEscape() as $name) { + /** @var AbstractExpression $operand */ + $operand = $expression->getNode($name); + if ($operand instanceof OperatorEscapeInterface) { + $this->escapeConditional($operand, $env, $type); + } else { + $expression->setNode($name, $this->escapeExpression($operand, $env, $type)); + } } } diff --git a/src/NodeVisitor/SafeAnalysisNodeVisitor.php b/src/NodeVisitor/SafeAnalysisNodeVisitor.php index 9eda8c8e134..3be82304f4b 100644 --- a/src/NodeVisitor/SafeAnalysisNodeVisitor.php +++ b/src/NodeVisitor/SafeAnalysisNodeVisitor.php @@ -13,7 +13,6 @@ use Twig\Environment; use Twig\Node\Expression\BlockReferenceExpression; -use Twig\Node\Expression\ConditionalExpression; use Twig\Node\Expression\ConstantExpression; use Twig\Node\Expression\FilterExpression; use Twig\Node\Expression\FunctionExpression; @@ -21,6 +20,7 @@ use Twig\Node\Expression\MacroReferenceExpression; use Twig\Node\Expression\MethodCallExpression; use Twig\Node\Expression\NameExpression; +use Twig\Node\Expression\OperatorEscapeInterface; use Twig\Node\Expression\ParentExpression; use Twig\Node\Node; @@ -96,10 +96,15 @@ public function leaveNode(Node $node, Environment $env): ?Node } elseif ($node instanceof ParentExpression) { // parent block is safe by definition $this->setSafe($node, ['all']); - } elseif ($node instanceof ConditionalExpression) { - // intersect safeness of both operands - $safe = $this->intersectSafe($this->getSafe($node->getNode('expr2')), $this->getSafe($node->getNode('expr3'))); - $this->setSafe($node, $safe); + } elseif ($node instanceof OperatorEscapeInterface) { + // intersect safeness of operands + $operands = $node->getOperandNamesToEscape(); + if (2 < \count($operands)) { + throw new \LogicException(\sprintf('Operators with more than 2 operands are not supported yet, got %d.', \count($operands))); + } elseif (2 === \count($operands)) { + $safe = $this->intersectSafe($this->getSafe($node->getNode($operands[0])), $this->getSafe($node->getNode($operands[1]))); + $this->setSafe($node, $safe); + } } elseif ($node instanceof FilterExpression) { // filter expression is safe when the filter is safe if ($node->hasAttribute('twig_callable')) { diff --git a/src/TokenParser/TypesTokenParser.php b/src/TokenParser/TypesTokenParser.php index b97eb3b2e4c..02aef81cc2e 100644 --- a/src/TokenParser/TypesTokenParser.php +++ b/src/TokenParser/TypesTokenParser.php @@ -63,9 +63,13 @@ private function parseSimpleMappingExpression(TokenStream $stream): array $first = false; $nameToken = $stream->expect(Token::NAME_TYPE); - $isOptional = null !== $stream->nextIf(Token::PUNCTUATION_TYPE, '?'); - $stream->expect(Token::PUNCTUATION_TYPE, ':', 'A type name must be followed by a colon (:)'); + if ($stream->nextIf(Token::OPERATOR_TYPE, '?:')) { + $isOptional = true; + } else { + $isOptional = null !== $stream->nextIf(Token::PUNCTUATION_TYPE, '?'); + $stream->expect(Token::PUNCTUATION_TYPE, ':', 'A type name must be followed by a colon (:)'); + } $valueToken = $stream->expect(Token::STRING_TYPE); diff --git a/tests/Node/Expression/Binary/NullCoalesceTest.php b/tests/Node/Expression/Binary/NullCoalesceTest.php new file mode 100644 index 00000000000..b2c79320ad6 --- /dev/null +++ b/tests/Node/Expression/Binary/NullCoalesceTest.php @@ -0,0 +1,29 @@ +assertEquals($test, $node->getNode('test')); + $this->assertEquals($left, $node->getNode('left')); + $this->assertEquals($right, $node->getNode('right')); + } + + public static function provideTests(): iterable + { + $tests = []; + + $test = new ConstantExpression(1, 1); + $left = new ConstantExpression(2, 1); + $right = new ConstantExpression(3, 1); + $node = new ConditionalTernary($test, $left, $right, 1); + $tests[] = [$node, '((1) ? (2) : (3))']; + + return $tests; + } +} diff --git a/tests/Node/IncludeTest.php b/tests/Node/IncludeTest.php index b218748d7d0..8a73a76cec5 100644 --- a/tests/Node/IncludeTest.php +++ b/tests/Node/IncludeTest.php @@ -12,8 +12,8 @@ */ use Twig\Node\Expression\ArrayExpression; -use Twig\Node\Expression\ConditionalExpression; use Twig\Node\Expression\ConstantExpression; +use Twig\Node\Expression\Ternary\ConditionalTernary; use Twig\Node\IncludeNode; use Twig\Test\NodeTestCase; @@ -46,7 +46,7 @@ public static function provideTests(): iterable EOF ]; - $expr = new ConditionalExpression( + $expr = new ConditionalTernary( new ConstantExpression(true, 1), new ConstantExpression('foo', 1), new ConstantExpression('foo', 1), diff --git a/tests/Node/ModuleTest.php b/tests/Node/ModuleTest.php index 4dbce150485..5caddd93b42 100644 --- a/tests/Node/ModuleTest.php +++ b/tests/Node/ModuleTest.php @@ -15,8 +15,8 @@ use Twig\Loader\ArrayLoader; use Twig\Node\BodyNode; use Twig\Node\EmptyNode; -use Twig\Node\Expression\ConditionalExpression; use Twig\Node\Expression\ConstantExpression; +use Twig\Node\Expression\Ternary\ConditionalTernary; use Twig\Node\Expression\Variable\AssignContextVariable; use Twig\Node\Expression\Variable\AssignTemplateVariable; use Twig\Node\Expression\Variable\TemplateVariable; @@ -223,7 +223,7 @@ public function getSourceContext(): Source $set = new SetNode(false, new Nodes([new AssignContextVariable('foo', 4)]), new Nodes([new ConstantExpression('foo', 4)]), 4); $body = new BodyNode([$set]); - $extends = new ConditionalExpression( + $extends = new ConditionalTernary( new ConstantExpression(true, 2), new ConstantExpression('foo', 2), new ConstantExpression('foo', 2),