FlowQuery operations: incompatible check on first array element in canEvaluate

[patricekaufmann]

Description

When using references node properties, it is possible for the editors to delete the referenced elements, eg. a Neos.Neos:Document. The referenced identifier remains in the database as part of the property value.

Once the editor deletes the first referenced element, it leads to problems in most of the FlowQuery operations. The problem has its root in the canEvaluate function of most FlowQuery operations. Inside those functions, the first array element is accessed via $context[0]. This leads to problems when Neos filters the deleted elements as the array will not be re-indexed and therefore it's no longer safe to access the first element via [0]. The key might be different.

Affected Operations:

Steps to Reproduce

1. Create references property and select 3 elements
2. Delete the first element
3. Use the references array of nodes in a FlowQuery operation

Expected behavior

FlowQuery operations should not perform checks in canEvaluate that are not relevant for the actual execution of the evaluate function. Most operations use a forEach loop in the evaluate function and don't rely on the first ([0]) element.

Workaround

Manually reindex the array by using Array.slice(array,0) in Fusion.

Possible Solution that requires PHP 7.3.0

It's possible to check the first element by the first key dynamically to bypass the problem of the values still being stored in the database:

    public function canEvaluate($context)
    {
        $firstKey = is_array($context) ? array_key_first($context) : null;
        return count($context) === 0 || (isset($firstKey) && isset($context[$firstKey]) && ($context[$firstKey] instanceof NodeInterface));
    }

Possible Solution that does not require PHP 7.3.0

This solution does not require PHP 7.3.0 - however it is not guaranteeed that the first element is checked because the pointer might point on a different item. From what I can tell, the ´evaluate´ method does not rely on the first element anyway.

    public function canEvaluate($context)
    {
        return count($context) === 0 || (current($context) instanceof NodeInterface);
    }

[mhsdesign]

I think the [0] check was made to ducktype the array and it will assume, if the first (0) element is a node, all will be nodes. So current($context) might be okay? Neos 5.3 support will be ending soon, and then we only need to support php 7.3...

[mhsdesign]

can we get the first $el via foreach and break?

[daniellienert]

I would suggest to change it to

return is_array($context) && reset($context) instanceof TraversableNodeInterface;

[mhsdesign]

I think instead of fixing eel we should instead avoid returning non 0 indexed arrays:

${q(node).property("someReference")}

This might indeed return an array with holes like [1 => NODE, 2 => NODE, 5 => NODE] if the identifiers in fields 0, 3 and 4 are not resolvable.

Thats because of the "unsafe" array_filter method in resolvePropertyReferences https://github.com/neos/neos-development-collection/blob/378a029d0cc7ea6acb853751e7592873584a4aac/Neos.ContentRepository/Classes/Domain/Model/Node.php#L961 which will leave holes in the array.

Using array_filter was introduced with Neos 2.2 so this is technically a regression of https://github.com/neos/neos-development-collection/commit/87804e12082e7d6d06bd22f50739e20eeaa45539 ^^

I think its fine to say flowquery can only work properly on lists (0 indexed arrays) and we just need to be a bit more careful with that. This will not be a problem in Neos 9 and i would argue to rather fix Node::getProperty instead of flowquery like you proposed. I will close this stale pr for now https://github.com/neos/neos-development-collection/pull/3946

My fix regarding Node::getProperty can be reviewed here: https://github.com/neos/neos-development-collection/pull/4731