-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add ??? Empty Coalesce Operator #2787
base: 2.x
Are you sure you want to change the base?
Conversation
Signed-off-by: Andrew Welch <[email protected]>
Signed-off-by: Andrew Welch <[email protected]>
Signed-off-by: Andrew Welch <[email protected]>
What is the difference with the Well, one of them is that it relies on PHP's |
btw, your implementation also ignores |
Thanks for looking @stof !
It's the same reason why the
...using the |
I'm a big fan of this idea — It comes up frequently enough in my Twig templates (where I'm comparing a chain of values and I want to use/output the first non-empty one) that I've created my own stock extension for it, and it's a default add to all my projects. I think My only reservation about this in-practice is the case of It's definitely a non-standard op, but I think the utility of it is worth adding the sugar to Twig even though no similar operator exists upstream in PHP. |
+1 to this PR - i am using @khalwat's null-coalescing operator as a plugin on a site and it is really nice to have in any context where your Twig data contains, for instance, a lot of empty arrays. |
Why not use |
It actually uses |
@nicolas-grekas — The primary benefit is that (Also, it needs to be right-associative... I think |
@nicolas-grekas what would the Twig code for this be using the
In PHP |
twig is a different language, we can remove this behavior if we want to. |
That's definitely true, but I'd rather define a new operator for new functionality than hoist new functionality on an operator for which there's already a defined behavior. Many people who write in Twig also write in PHP, and it might be very confusing if |
that would require changing the existing operator, and that makes mistake detection harder in case you make typos in your variable names. I would vote for keeping the existing behavior of the operator. |
Another vote to not modify existing operator behaviors! (And a vote for |
Welp, looks like the It's not the same thing, but it does show some precedence for a three character operator... so I hope |
If we’re calling this the “Empty Coalesce Operator”, I would expect that any value PHP considers “empty” would also be considered empty here, so |
Yeah that's the idea @brandonkelly -- it's called the Empty Coalesce Operator because the behavior is what we'd expect from chained/nested |
https://github.com/twigphp/Twig/pull/2787/files#diff-b445caeb1cc1391b4cc1966bd1b1f76cR28 After compilation, wouldn't the current implementation eventually evaluate both the left-hand side and the right-hand side twice at runtime? I mean it's possible that If there is a static variable (such as a static counter to log how many time the func has been executed) in |
Signed-off-by: Andrew Welch <[email protected]>
@jfcherng suggestions on how to mitigate the behavior you've mentioned? |
@khalwat Sorry I cannot really answer that. But that is what I saw when I was following the
$compiler
->raw('(('.self::class.'::empty(')
->subcompile($this->getNode('left'))
->raw(') ? null : ')
->subcompile($this->getNode('left'))
->raw(') ?? ('.self::class.'::empty(')
->subcompile($this->getNode('right'))
->raw(') ? null : ')
->subcompile($this->getNode('right'))
->raw('))')
; With this, I am assuming (empty(L) ? null : L) ?? (empty(R) ? null : R) The worst case is evaluating both L and R twice when executing the compiled template. Maybe worth a note in the docs if this cannot be resolved? |
c2a7ac3
to
d997512
Compare
How about using |
@dharkness |
@brandonkelly I can only speak for PHP, but both
|
|
||
@trigger_error(sprintf('Using the "Twig_Extension_Core" class is deprecated since Twig version 2.7, use "Twig\Extension\CoreExtension" instead.'), E_USER_DEPRECATED); | ||
final class CoreExtension extends AbstractExtension |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change is wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, the error message is wrong (a copy and paste error) but if we want this removed as per your earlier comment, then it probably doesn't matter. It'd be fixed by just removing the whole deprecation nonsense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a change to remove the pointless Twig_Node_Expression_EmptyCoalesce
class entirely, which should resolve this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not fixed. You are still making a huge mess in the CoreExtension file, reverting the legacy file to a full class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using this to merge into: https://github.com/twigphp/Twig/blob/2.x/src/Extension/CoreExtension.php
Should I be using it from a different branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stof I'd love to get this addressed, but I could use some guidance.
The only reason there is a change to lib/Twig/Extension/Core.php
is to include the ???
operator. To fix this so that it's not creating a "huge mess" I just need to know what branch/source I should be diff'ing with to add this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should not be making any change in lib/Twig/Extension/Core.php
, which is a legacy BC file (except you are reverting this to not be a BC file anymore). The change should be done only in src/Extension/CoreExtension.php
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
your PR is reverting #2863 on this file, which is what the huge mess is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. That change to make lib/Twig/Extension/Core.php
wasn't made until after the PR I made, thus my confusion about what's going on here.
I will revert the now legacy lib/Twig/Extension/Core.php
and make my changes in src/Extension/CoreExtension.php
Signed-off-by: Andrew Welch <[email protected]>
Your example is easy to do with operators {% set bar = null %}
{% set foo = '' %}
{% set baz = [] %}
{% set foobar = woof ?? null ?: bar ?: foo ?: baz ?: 'bark' %}
{{ foobar }} |
@GromNaN that presupposes that you know the state of the variables. The point is that any of them could be undefined, null, or empty, and you don't know ahead of time. |
I did't imply this feature was not useful, just trying to help using the existing features. If the existence of any variable is unknown, the example can be:
|
@GromNaN I don’t think anyone here is unaware of existing syntax options. You could have made the same point about |
Excepted that with |
What's the status of this? Thought I'd bump it up since it seems it got abandoned without any particular reason. Did the team decide this isn't desirable? the comment voting from the community seems to suggest otherwise. So I guess the question is
|
Currently, the PR is borked. I'd be happy to attempt to redo it if there is interest from the maintainers. |
I want to add my +1 on that. And thanks for the work on that.
The readability is just a whole other level. |
Wouldn't |
I guess you're right. I stumbled upon this via a plugin that allows to use |
Just my two cents, but the first thing I thought about is
It could even be a RFC proposed to PHP first, in order to have the same symbol for both twig and php. |
@VincentLanglet would love to see an RFC for it in PHP, too. The reason I prefer |
Do you have examples of other languages with |
@VincentLanglet link is in the first post in the thread; someone added it to Swift using |
Empty Coalesce adds the
???
operator to Twig that will return the first thing that is defined, not null, and not empty. This is particularly useful when you're dealing with a number of fallback/default values that may or may not exist, and may or may not be empty.The
???
Empty Coalescing operator is similar to the??
null coalesce operator, but also ignores empty strings (""
), 0, 0.0, null, false, and empty arrays ([]
) as well.Because this is an Empty Coalesce Operator, it functions identically to the PHP
empty()
function in terms of return values.Example:
This will output:
...because:
woof
is undefinedbar
isnull
foo
is an empty stringbaz
is an empty arrayThere is precedence for this operator in languages such as Swift:
https://medium.com/@JanLeMann/providing-meaningful-default-values-for-empty-or-absence-optionals-via-nil-or-empty-coalescing-379abd22ae77