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

Compile Elvis operator into Elvis operator ?: #3896

Merged
merged 1 commit into from
Oct 22, 2023

Conversation

GromNaN
Copy link
Contributor

@GromNaN GromNaN commented Oct 21, 2023

When using ternary operator without "then" part, the "condition" part is evaluated twice, which is inconsistent with how PHP works.
The Twig template A ?: B is currently compiled as A ? A : B in PHP. This PR change it to A ?: B.

If A is a complex expression, it improves performance to only execute the expression once.
If A is has a side effect (like updating a variable), the expression being executed twice could result in a bug. (example in PHP)

Example with @WebProfiler/Collector/form.html.twig line 9 (see deep diff)

{{ collector.data.nb_errors ?: collector.data.forms|length }}

Previous compilation:

echo twig_escape_filter($this->env, (((isset($context["error_count"]) || array_key_exists("error_count", $context) ? $context["error_count"] : (function () { throw new RuntimeError('Variable "error_count" does not exist.', 9, $this->source); })())) ? ((isset($context["error_count"]) || array_key_exists("error_count", $context) ? $context["error_count"] : (function () { throw new RuntimeError('Variable "error_count" does not exist.', 9, $this->source); })())) : (twig_get_attribute($this->env, $this->source, (isset($context["collector"]) || array_key_exists("collector", $context) ? $context["collector"] : (function () { throw new RuntimeError('Variable "collector" does not exist.', 9, $this->source); })()), "countDefines", [], "any", false, false, false, 9))), "html", null, true);

After this change:

echo twig_escape_filter($this->env, ((isset($context["error_count"]) || array_key_exists("error_count", $context) ? $context["error_count"] : (function () { throw new RuntimeError('Variable "error_count" does not exist.', 9, $this->source); })()) ?: twig_get_attribute($this->env, $this->source, (isset($context["collector"]) || array_key_exists("collector", $context) ? $context["collector"] : (function () { throw new RuntimeError('Variable "collector" does not exist.', 9, $this->source); })()), "countDefines", [], "any", false, false, false, 9)), "html", null, true);

@fabpot
Copy link
Contributor

fabpot commented Oct 22, 2023

Thank you @GromNaN.

@fabpot fabpot merged commit 415f4cc into twigphp:3.x Oct 22, 2023
57 checks passed
@GromNaN GromNaN deleted the compile-elvis branch October 22, 2023 18:32
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.

2 participants