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.

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

Optimized TextNodes have no SourceContext #4046

Closed
wants to merge 1 commit into from
Closed

Conversation

Lumaraf
Copy link
Contributor

@Lumaraf Lumaraf commented Apr 19, 2024

Fixes #4045

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

@@ -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.

@fabpot
Copy link
Contributor

fabpot commented May 1, 2024

I think this one can be closed now, right?

@fabpot fabpot closed this May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Optimized TextNodes have no SourceContext
3 participants