twigphp / Twig

Twig, the flexible, fast, and secure template language for PHP
https://twig.symfony.com/
BSD 3-Clause "New" or "Revised" License
8.14k stars 1.25k forks source link

[Feature] Optional chaining operator #3260

Open hason opened 4 years ago

hason commented 4 years ago

Add support for the optional chaining operator ?. that is available in:

{# Before #}
{{ foo.bar.baz is defined ? foo.bar.baz }}

{# After #}
{{ foo?.bar?.baz }}
VincentLanglet commented 4 years ago

it should even be the default behaviour.

stof commented 4 years ago

-1 for being the default behavior. That would make it a lot harder to identify mistakes (typo in a property name) when you don't expect it to be optional.

VincentLanglet commented 4 years ago

-1 for being the default behavior. That would make it a lot harder to identify mistakes (typo in a property name) when you don't expect it to be optional.

In php, there is tools like phpstan returning error for

$foo->getBar()->getBaz()

if getBar can be null. But there is nothing like this for twig. So mistake is easier to make.

I find sad the fact that

{% if foo.bar.baz %}
...
{% endif %}

throw an exception and display an error 500 in production when foo.bar is null.

This should be user-first and not dev-first.

What about a strict mode, enable in dev ?

stof commented 4 years ago

Well, first, it does not throw an exception by default in production, as strict_variables is opt-in and will make . return null for non-existent attributes.

If what you want is to have everything optional in prod and strict in dev, Twig already has that feature since the 1.0 release (and probably even before in some 0.x releases).

To me, the benefit of a ?. operator would be that it could be used even in strict mode

VincentLanglet commented 4 years ago

Well, first, it does not throw an exception by default in production, as strict_variables is opt-in and will make . return null for non-existent attributes.

If what you want is to have everything optional in prod and strict in dev, Twig already has that feature since the 1.0 release (and probably even before in some 0.x releases).

To be, the benefit of a ?. operator would be that it could be used even in strict mode

Oh sorry, didn't know that. That's great. I agree with you then.

Herz3h commented 4 years ago

Would be cool to have this feature 👍

hason commented 3 years ago

Null-safe operator was introduced in PHP 8 https://wiki.php.net/rfc/nullsafe_operator

Bilge commented 3 years ago

We should have this.

acalvino4 commented 2 years ago

Have any maintainers taken a look at this yet? this would be an awesome feature that would aid in writing shorter, more expressive templating. I know maintainers often have a lot on their plate, so I understand if there are other priorities, but I would at least like to know if this is being considered / on a roadmap.

I am guessing that the use of this construct would just get translated to the underlying php operator, in which case it would probably be pretty easy to implement, but would only be compatible with php8, so maybe that is holding the feature back?

spackmat commented 2 years ago

As mentioned in #3609 this would also be a nice little improvement for filters like in myNullableString?|myStringFilter so that those don't get called at all and don't throw a deprecation in PHP 8.1 when the value to filter is null.

acalvino4 commented 2 years ago

@nicolas-grekas @hason As top contributors, do you know (or know who would know) if an implementation of this would be accepted or is maybe even on the roadmap?

PS - sorry for the direct tag, but as no maintainer has replied to my request for comment here or on the slack channel, I didn't know of a better way to get a response.

Bilge commented 2 years ago

You should ask @fabpot. It's a lot more likely to be accepted if there's a PR, but I can understand the reluctance to work on something that might be rejected.

fabpot commented 2 years ago

I would review a PR implementing this new behavior. But if we want to be "simple" to implement, it might mean bumping the min PHP version to 8.0 (7.2 right now for the 3.x branch).

Bilge commented 2 years ago

Not that I am in any way an expert, but I can't see any reason why the implementation has to be constrained by platform feature support of the same. Twig has a history of implementing features before PHP (e.g. named parameters) and that's a good thing.

fabpot commented 2 years ago

Implementation will tell us.

stof commented 2 years ago

@Bilge Twig named parameters are a compile-time feature (which is why Twig macros don't support named parameters btw).

@fabpot I'm not sure we can use the native optional chaining to implement that in Twig, due to . compiling to a twig_get_attribute function call for instance. A Twig chain will not compile to a PHP chain.

fabpot commented 2 years ago

Indeed stof. But again, let’s talk about implementation in a PR

xepozz commented 1 year ago

Any news?

xabbuh commented 1 year ago

@xepozz Until now nobody proposed a pull request implementing this feature.

realjjaveweb commented 1 year ago

I don't really have time for PR and refinements, but here's some inspiration, it shouldn't be that hard:

class NullsafeExtension extends \Twig\Extension\AbstractExtension
{
    public function getTokenParsers()
    {
        return array(new NullsafeTokenParser());
    }
}

class NullsafeTokenParser extends \Twig\TokenParser\AbstractTokenParser
{
    public function parse(\Twig\Token $token)
    {
        $stream = $this->parser->getStream();
        $node = $this->parser->getExpressionParser()->parseExpression();
        while ($stream->test(\Twig\Token::OPERATOR_TYPE, '?.')) {
            $stream->next();
            $node = new \Twig\Node\Expr\Nullsafe(
                $node, $this->parser->getExpressionParser()->parsePrimaryExpression()
            );
        }
        return $node;
    }
    public function getTag()
    {
        return 'nullsafe';
    }
}

Then somehow load it into Twig environment

$twig = new Twig_Environment($loader);
$twig->addExtension(new NullsafeExtension());

and finally it coul be used as

{{ some.?somethingMayNotBeThere.?somethingElseMayNotBeThere() }}

realjjaveweb commented 1 year ago

I would like to also note, that I have a feeling that some.somethingMayNotBeThere.somethingElseMayNotBeThere()|default('some fallback value') can work pretty much the same, not sure if that relies on some strict/nonstrict mode though

acalvino4 commented 1 year ago

I started playing around with an implementation of this, and as soon as I got a basic something working, I realized that I can't actually think of a situation where it is needed/helpful. Some of these points have been made earlier in the conversation here, but I think it may be helpful to summarize.

Say we have:

{% set foo = { bar: 'baz' } %}

First of all (as @stof pointed out), without strict mode (the default), . already just returns null at any point in a chain where a variable is undefined:

{# with `strict_variables` set to false #}
{{ foo.bar }} {# output: 'baz' #}
{{ foo.bad }} {# output: '' #}
{{ bad.bar }} {# output: '' #}

I think it is pretty common practice for prod environments to be set to non-strict variables (you don't want visitors seeing error messages), and dev environments to be set to strict variables; hence that case does need to be handled too, and I think it is.

If the goal is to perform a test, the is defined avoids an error in all cases and returns what you'd expect:

{# with `strict_variables` set to true or false #}
{% if foo.bar is defined %}defined{% else %}not defined{% endif %} {# output: 'defined' #}
{% if foo.bad is defined %}defined{% else %}not defined{% endif %} {# output: 'not defined' #}
{% if bad.bar is defined %}defined{% else %}not defined{% endif %} {# output: 'not defined' #}

If the goal is to assign to a variable, the default filter will do the job (as @realjjaveweb pointed out):

{# with `strict_variables` set to true or false #}
{% set var = foo.bar|default(null) %}{{ var }} {# output: 'baz' #}
{% set var = foo.bad|default(null) %}{{ var }} {# output: '' #}
{% set var = bad.bar|default(null) %}{{ var }} {# output: '' #}

So the only place where the ?. construct might be helpful is when you have strict_variables enabled and want to avoid using the default filter. But that would only deal with the last line of the last code block; the middle line (with var = foo.bad) would still require |default to avoid a twig error because the nullsafe operator only checks the expression before itself for null, not the expression after (at least in typical implementations of the construct).

I'm curious if given this summary, anyone still knows of a use for this in twig. My current take, given Twig's current behaviors and abilities is

stof commented 1 year ago

@acalvino4 if you look at the behavior of the |default filter, you will see that it does not really do the job. Any defined value considered as empty will trigger the default.

acalvino4 commented 1 year ago

I see, so for the case {% set var = foo.bad|default(null) %} where foo.bad is defined but equal to '', then var === null rather than var === ''.

So I acknowledge that there is slightly different behavior. If all you are doing is printing var somewhere ({{ var }}) that difference is moot.

Hence I am curious how many real-world use-cases can't easily be handled with default or is defined. Not saying there aren't any - just saying that all the one's I've thought of and come across I can handle with existing twig functionality..

realjjaveweb commented 1 year ago

@acalvino4 I totally agree. In 99% cases it's about textual output. Outputing null is unlikely and if that's happening very often in the app one should create custom function/filter since that's quite an edge case. Only more "real" less edgy case I can think of is outputing boolean - however - most likely you will want to provide some custom common output - so either if it's often - define custom filter, otherwise I will remind that twig does support ternary operator so you can do

{{ foo|default(false) ? 'yes' : 'no' }}
{% set foo = false %}
{{ foo|default(false) ? 'yes' : 'no' }}
{% set foo = true %}
{{ foo|default(false) ? 'yes' : 'no' }
### OUTPUT: ###
no
no
yes

You can test it on https://twigfiddle.com/

realjjaveweb commented 1 year ago

One more important note note - @stof is a bit misleading here default filter is NOT the same thing as PHP's empty()! The twig's empty docs say:

empty checks if a variable is an empty string, an empty array, an empty hash, exactly false, or exactly null

so this...

{% set foo = 0 %}
{{ foo|default('not there') }}

=> outputs 0. rest was already said by @acalvino4

What wasn't said here is the documentation says that if you want to tackle undefined booleans - you should use ?? operator:

Using the default filter on a boolean variable might trigger unexpected behavior, as false is treated as an empty value. Consider using ?? instead: {% set foo = false %} {{ foo|default(true) }} {# true #} {{ foo ?? true }} {# false #}

stof commented 1 year ago

@realjjaveweb I never said it was the same that PHP empty. It is the same than *Twig** empty. but false is empty for instance.

spackmat commented 1 year ago

The discussion started to drift away from the initially described problem: The optional chaining operator only really gets useful, when there is a chain of objects. Sure, when the chain only has two elements, there are pretty simple other ways to handle a nullable object (the default-filter or tenary were named). But when there is a real chain, like nullableObject.nullableRelatedObject.nullableOtherRelatedObject.property, a ?. operator like the ?-> in PHP can be very handy to avoid nasty checks or even more nasty rare error conditions.

realjjaveweb commented 1 year ago

@spackmat yes, but common chains in twig can be handled by |default filter. The issue is basically only for null vs bool. @stof sorry, my mistake, it sounded to me a bit like it, my bad

element-code commented 1 year ago

https://github.com/twigphp/Twig/issues/3260#issuecomment-1540765585

If the goal is to perform a test, the is defined avoids an error in all cases and returns what you'd expect: [...] If the goal is to assign to a variable, the default filter will do the job (as @realjjaveweb pointed out): [...]

There is a third goal: output of content.

Imagine the Output {{ order.orderCustomer.customer.customFields.acmeExtensionCustomersSAPCustomerNumber }}, having to check with defined beforehand kinda sucks, defining variables for each output sucks too. However, this case can be handled with the default filter (as long as you don't care about 0 or false being handled as null)

realjjaveweb commented 1 year ago

@element-code quick correction: default only falls to default for not defined and "empty string, an empty array, an empty hash, exactly false, or exactly null", NOT for 0/'0'. BTW: defined DOESN'T do the null check! (it's not like PHP's isset).

ryanleichty commented 1 year ago

Just started working with twig files, coming from JavaScript. Would love to have this 🔥

rudiedirkx commented 5 months ago

@acalvino4 @spackmat My usecase (in strict_variables mode) is

{{ site.getLatestResult('host').getHtmlPill() }}

Exception: Impossible to invoke a method ("getHtmlPill") on a null variable.

getLatestResult('host') is expensive, so the only way to do that now is

{% set hostResult = site.getLatestResult('host') %}
{{ hostResult ? hostResult.getHtmlPill() }}

Not horrible, but not as readable as

{{ site.getLatestResult('host')?.getHtmlPill() }}

Using default() doesn't throw the strict error, BUT it calls getLatestResult() twice (that's what default() compiles into). (So does ?? unfortunately, but that aside.) I assume ?. (or .?) wouldn't.


With |default('XXX'):

yield Twig\Extension\EscaperExtension::escape(
    $this->env,
    (
        // Once to see if it's empty
        TwigBridge\Node\GetAttrNode::attribute(
            $this->env,
            $this->source,
            TwigBridge\Node\GetAttrNode::attribute(
                $this->env,
                $this->source,
                $context["site"],
                "getLatestResult",
                ["host"],
                "method",
                false,
                true,
                false,
                58
            ),
            "getHtmlPill",
            [],
            "method",
            true,
            true,
            false,
            58
        ) ? (
            Twig\Extension\CoreExtension::defaultFilter(
                // Once again if it's not
                TwigBridge\Node\GetAttrNode::attribute(
                    $this->env,
                    $this->source,
                    TwigBridge\Node\GetAttrNode::attribute(
                        $this->env,
                        $this->source,
                        $context["site"],
                        "getLatestResult",
                        ["host"],
                        "method",
                        false,
                        true,
                        false,
                        58
                    ),
                    "getHtmlPill",
                    [],
                    "method",
                    false,
                    false,
                    false,
                    58
                ),
                "XXX"
            )
        ) : ("XXX")
    ),
    "html",
    null,
    true
);

Actually that looks like a bug... Why is it called twice? Once before defaultFilter() ...? With the default value "XXX" twice... Twig 3.9.3