Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Optimized TextNodes have no SourceContext #4046

Closed
wants to merge 1 commit into from
Closed

Optimized TextNodes have no SourceContext #4046

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 17 additions & 5 deletions src/NodeVisitor/OptimizerNodeVisitor.php
View file
Open in desktop
Expand Up @@ -42,6 +42,7 @@ final class OptimizerNodeVisitor implements NodeVisitorInterface
{
public const OPTIMIZE_ALL = -1;
public const OPTIMIZE_NONE = 0;
public const OPTIMIZE_PRINT = 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

making this optimization configurable would deserve a dedicated PR (as it deserve a changelog entry for such new feature)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, i extracted those changes into a separate PR #4051.

public const OPTIMIZE_FOR = 2;
public const OPTIMIZE_RAW_FILTER = 4;
public const OPTIMIZE_TEXT_NODES = 8;
Expand All @@ -55,7 +56,7 @@ final class OptimizerNodeVisitor implements NodeVisitorInterface
*/
public function __construct(int $optimizers = -1)
{
if ($optimizers > (self::OPTIMIZE_FOR | self::OPTIMIZE_RAW_FILTER)) {
if ($optimizers !== self::OPTIMIZE_ALL && ($optimizers & ~(self::OPTIMIZE_PRINT | self::OPTIMIZE_FOR | self::OPTIMIZE_RAW_FILTER | self::OPTIMIZE_TEXT_NODES)) !== 0) {
throw new \InvalidArgumentException(sprintf('Optimizer mode "%s" is not valid.', $optimizers));
}

Expand All @@ -81,7 +82,9 @@ public function leaveNode(Node $node, Environment $env): ?Node
$node = $this->optimizeRawFilter($node);
}

$node = $this->optimizePrintNode($node);
if (self::OPTIMIZE_PRINT === (self::OPTIMIZE_PRINT & $this->optimizers)) {
$node = $this->optimizePrintNode($node);
}

if (self::OPTIMIZE_TEXT_NODES === (self::OPTIMIZE_TEXT_NODES & $this->optimizers)) {
$node = $this->mergeTextNodeCalls($node);
Expand All @@ -107,13 +110,20 @@ private function mergeTextNodeCalls(Node $node): Node
return $node;
}

$firstChildNode = $node->getNode($names[0]);
$line = $firstChildNode->getTemplateLine();
$sourceContext = $firstChildNode->getSourceContext();
if (Node::class === get_class($node)) {
return new TextNode($text, $node->getTemplateLine());
$textNode = new TextNode($text, $line);
$textNode->setSourceContext($sourceContext);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will break if $sourceContext is null.

But maybe the right fix would be for \Twig\Node\Node::setNode to set the source context in the new node in case we have a source context and the node being set does not have one. This would be consistent with the fact that setSourceContext also sets the node on all children at the time it is set.
this way, it would work for the nodes created by OptimizerNodeVisitor but would also solve the similar issue for any other custom node visitor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting the SourceContext in setNode sounds like a great idea. That solves my current problem with missing SourceContexts after optimization and simplifies all of my current NodeVisitor use cases where i have to set the SourceContext everywhere i create new nodes.

I created a separate PR for that small change: #4050

return $textNode;
}

foreach ($names as $i => $name) {
if (0 === $i) {
$node->setNode($name, new TextNode($text, $node->getTemplateLine()));
$textNode = new TextNode($text, $line);
$textNode->setSourceContext($sourceContext);
$node->setNode($name, $textNode);
} else {
$node->removeNode($name);
}
Expand All @@ -138,7 +148,9 @@ private function optimizePrintNode(Node $node): Node
$exprNode = $node->getNode('expr');

if ($exprNode instanceof ConstantExpression && \is_string($exprNode->getAttribute('value'))) {
return new TextNode($exprNode->getAttribute('value'), $exprNode->getTemplateLine());
$textNode = new TextNode($exprNode->getAttribute('value'), $node->getTemplateLine());
$textNode->setSourceContext($node->getSourceContext());
return $textNode;
}

if (
Expand Down