EmptyInterface

[brandonkelly]

Adds a new EmptyInterface that any object could implement, giving it control over what CoreExtension::testEmpty() returns for it.

For example, if an object’s simple existence should be counted as not empty, it could do:

use Twig\Extension\EmptyInterface;

class MyClass implements EmptyInterface
{
    // ...

    public function getIsEmpty(): bool
    {
        return false;
    }
}

This will help with issues like craftcms/cms#18727, where empty tests and the default filter can cause performance issues simply by traversing over all the existing properties (many of which may cause DB queries, etc.).

[stof]

I'm not convinced we should introduce this extension point in Twig core, especially when the craftcms issue says that the clean solution for their problem would actually be to use the ?? operator instead of the |default filter in most places.

[brandonkelly]

I disagree. is empty and |default are very commonly-used Twig features, and template designers should be able to use them freely without worrying about their potential performance impact. Right now that’s not the case.

This is a super simple change in Twig, and a super easy feature for platforms like Craft, Django, etc., to implement, that would eliminate a lot of rakes that are currently being stepped on.

[brandonkelly]

Alternatively, maybe just change the order of the checks? If Stringable were checked first, that would also fix this, without any changes needed by implementing platforms.

[mmikkel]

I'm not convinced we should introduce this extension point in Twig core, especially when the craftcms issue says that the clean solution for their problem would actually be to use the ?? operator instead of the |default filter in most places.

In most cases, yes – but |default and ?? aren't always interchangeable 😊 And to Brandon's point, I think many template designers tend to prefer |default just because it's the more intuitive option, and I also think a lot of them would not be aware of its inner workings when used on a (potentially) complex object.

IMO, if Twig can help avoid performance pitfalls with the testEmpty() method, I think it should.

[johnnynotsolucky]

Alternatively, maybe just change the order of the checks? If Stringable were checked first, that would also fix this, without any changes needed by implementing platforms.

@brandonkelly moving the order of checks around would probably be a breaking change for values where usage depends on the current order. ArrayCollection, for example, is iterable and \Stringable, where the string test will return false because its implementation never returns an empty string.

$col = new Doctrine\Common\Collections\ArrayCollection();
iterator_count($col); // 0
(string) $col; // Doctrine\Common\Collections\ArrayCollection@00000000000017380000000000000000

[stof]

I think many template designers tend to prefer |default just because it's the more intuitive option

Being more intuitive is actually a fallacy. my_boolean_var|default(true) is actually the same than true because false is considered empty. That's far from being intuitive.

IMO, if Twig can help avoid performance pitfalls with the testEmpty() method, I think it should.

This new interface does not guarantee avoiding performance pitfalls (it could actually create them depending on what the implementation does). What it guarantees is that it becomes impossible to document the behavior of the empty test (and so of the default filter) in the Twig documentation because objects could override the behavior of the filter.

If you know the kind of values you have in a variable, you could use dedicated methods on those objects when relevant instead of using default if you know that their Traversable implementation is expensive (they can also implement Countable in less expensive way if possible, as testEmpty will use Countable instead of going through the iterator if possible).
And if this is about dealing with potentially undefined variables, ?? is definitely a better solution (see the behavior for booleans as the main reason).

[brandonkelly]

This new interface does not guarantee avoiding performance pitfalls (it could actually create them depending on what the implementation does).

Every extension point has the possibility of impacting performance depending on the implementation. In this case there is already a clear performance pitfall, and an opportunity to drastically improve it. Seems like a pretty safe bet to me.

What it guarantees is that it becomes impossible to document the behavior of the empty test (and so of the default filter) in the Twig documentation because objects could override the behavior of the filter.

Not to show off, but here’s a stab at how it could be handled in the docs, based on the existing documentation around Countable:

For objects that implement Twig\Extension\EmptyInterface, empty will check the return value of the getIsEmpty() method.