Skip to content
New issue

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

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

Already on GitHub? # to your account

[Twig 4] Introduce ForElseNode #4473

Closed
wants to merge 1 commit into from
Closed

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Nov 24, 2024

Currently there is no line number information for {% else %} inside a for loop.

Normally, this wouldn't be such a big deal.

But TwigStan uses this line information to map errors in PHP back to the original Twig source.

Let's say you have the following template:

{% for product in products %}
    <h2>FOR</h2>
{% else %}
    <p>No products found</p>
{% endfor %}

And products is of type non-empty-array<Product>, it means that the else will never happen.

This is currently reported as:

Negated boolean expression is always false.
🔖 booleanNot.alwaysFalse
🐘 compiled_index.php:83
🌱 templates/product/index.html.twig:2
🎯 src/Controller/ProductController.php:15

But as you can see, it points to line number 2, instead of line number 3.

In the compiled code, it looks like this:

// line 1
foreach ($context['_seq'] as $context["_key"] => $context["product"]) {
    // line 2
    yield "        <h2>FOR</h2>\n    ";
    $context['_iterated'] = \true;
}
if (!$context['_iterated']) {
    // line 4
    yield "        <p>No products found</p>\n    ";
}

With this change, we will change the compiled code to become:

// line 1
foreach ($context['_seq'] as $context["_key"] => $context["product"]) {
    // line 2
    yield "        <h2>FOR</h2>\n    ";
    $context['_iterated'] = \true;
}
// line 3
if (!$context['_iterated']) {
    // line 4
    yield "        <p>No products found</p>\n    ";
}

Currently there is no line number information for `{% else %}` inside a `for` loop.

Normally, this wouldn't be such a big deal.

But TwigStan uses this line information to map errors in PHP back to the original Twig source.

Let's say you have the following template:

```twig
{% for product in products %}
    <h2>{{ product.name }}</h2>
{% else %}
    <p>No products found</p>
{% endfor %}
```

And `products` is of type `non-empty-array<Product>`, it means that the else will never happen.

This is currently reported as:
```
Negated boolean expression is always false.
🔖 booleanNot.alwaysFalse
🐘 compiled_index.php:83
🌱 templates/product/index.html.twig:2
🎯 src/Controller/ProductController.php:15
```

But as you can see, it points to line number 2, instead of line number 3.

In the compiled code, it looks like this:
```php
// line /Volumes/CS/opensource/twigstan-demo/templates/product/index.html.twig:1
foreach ($context['_seq'] as $context["_key"] => $context["product"]) {
    // line /Volumes/CS/opensource/twigstan-demo/templates/product/index.html.twig:2
    yield "        <h2>";
    // line /Volumes/CS/opensource/twigstan-demo/templates/product/index.html.twig:2
    yield \Twig\Extension\CoreExtension::getAttribute($this->env, $this->source, $context
    // line /Volumes/CS/opensource/twigstan-demo/templates/product/index.html.twig:2
    yield "</h2>\n    ";
    $context['_iterated'] = \true;
}
if (!$context['_iterated']) {
    // line /Volumes/CS/opensource/twigstan-demo/templates/product/index.html.twig:4
    yield "        <p>No products found</p>\n    ";
}
```

With this change, we will change the compiled code to become:
```php
// line /Volumes/CS/opensource/twigstan-demo/templates/product/index.html.twig:1
foreach ($context['_seq'] as $context["_key"] => $context["product"]) {
    // line /Volumes/CS/opensource/twigstan-demo/templates/product/index.html.twig:2
    yield "        <h2>";
    // line /Volumes/CS/opensource/twigstan-demo/templates/product/index.html.twig:2
    yield \Twig\Extension\CoreExtension::getAttribute($this->env, $this->source, $context
    // line /Volumes/CS/opensource/twigstan-demo/templates/product/index.html.twig:2
    yield "</h2>\n    ";
    $context['_iterated'] = \true;
}
// line /Volumes/CS/opensource/twigstan-demo/templates/product/index.html.twig:3
if (!$context['_iterated']) {
    // line /Volumes/CS/opensource/twigstan-demo/templates/product/index.html.twig:4
    yield "        <p>No products found</p>\n    ";
}
```
@fabpot fabpot mentioned this pull request Jan 24, 2025
@fabpot
Copy link
Contributor

fabpot commented Jan 24, 2025

@ruudk I've done the work here for 3.x: #4553

fabpot added a commit that referenced this pull request Jan 24, 2025
This PR was merged into the 3.x branch.

Discussion
----------

Add ForElseNode

Replaces #4473 for 3.x

Commits
-------

6097675 Add ForElseNode
@ruudk
Copy link
Contributor Author

ruudk commented Jan 24, 2025

Thank you! I will rebase as soon as V4 is synced.

@fabpot fabpot closed this Jan 24, 2025
@ruudk
Copy link
Contributor Author

ruudk commented Jan 24, 2025

I guess this was closed accidentally?

@ruudk
Copy link
Contributor Author

ruudk commented Jan 24, 2025

Ah, it's already merged.

@fabpot
Copy link
Contributor

fabpot commented Jan 24, 2025

Ah, it's already merged.

Yes, I made the needed changes when merging 3.x to 4.x (basically resolving the conflicts).

@ruudk
Copy link
Contributor Author

ruudk commented Jan 24, 2025

Got it, thank you for getting this merged 💙

@ruudk ruudk deleted the v4-for-else branch January 24, 2025 14:38
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants