BUGFIX: Have FlowQuery operations reliably detect nodes during canEvaluate #3946

mhsdesign · 2022-11-02T10:30:18Z

Original PR: #3723

Rebase for 5.3 #3838 (to be closed)

Rebase for 7.3 (this PR)

When a collection of nodes is referenced some items may become invisible over time or may be deleted. In this case the references may end up with an array that has no key 0. Since this key was used to check wether a flowQuery operation can be applied this could lead to errors.

The change adjusts the detection by looking at using reset to get the first value if the context is an array and by using an foreach that returns in the first cycle for traversables.

Review Instructions

To test this you can use the contentReferences Element in Neos.Demo.
Create 3 contents
Create a reference element that references all three
Hide the content that is selected first
Apply @process.filterContent( q(value).filter('[instanceof Neos.Neos:Content]').get()) to that.
Without the change in live context no content is shown because the first item is not available live. This change allows to still proceed with the remaining nodes.

…erations The new implementation checks that the first item is a NodeInterface regardless wether the array key is 0 or the item is inside a traversable.

…eeded use statements

The check now make a distinction between the nodeInterface and the TraversableNodeInterface

This is needed because find operates on the old NodeInterface which is not compatible with the current mocks.

Reapplies 928cc4a As i oversaw those changes in a merge conflict

…omments

mhsdesign · 2022-11-02T10:51:28Z

Fixed conflict with 928cc4a

mhsdesign

I added pending review comments from #3723

Sadly - i cannot reopen the original pr and force push - as we deleted the master branch so the github ui doesnt offer to reopen (until we would readd the master branch ...)

mhsdesign · 2022-11-02T11:11:04Z

Neos.ContentRepository/Classes/Eel/FlowQueryOperations/CanEvaluateNodeContextTrait.php

+            foreach ($context as $item) {
+                return $item instanceof NodeInterface;
+            }
+            return true;


https://github.com/neos/neos-development-collection/pull/3723/files#r920521176

i dont think we need this line.
it would be for the case that $context has no iterations, and in this case why should it pass?

Because an empty context can be evaluated.

mhsdesign · 2022-11-02T11:11:24Z

Neos.ContentRepository/Classes/Eel/FlowQueryOperations/FindOperation.php

-        foreach ($context as $contextNode) {
-            if (!$contextNode instanceof NodeInterface) {
-                return false;
+        if (is_iterable($context)) {


https://github.com/neos/neos-development-collection/pull/3723/files#r866009691

Neos.ContentRepository/Classes/Eel/FlowQueryOperations/HasOperation.php

patricekaufmann · 2023-02-17T13:55:46Z

I actually just ran into #3624 again, spent some time debugging to eventually figure out I already did that 1 year ago :D

What is the general progress on this PR? It would be super nice to fix those edge case errors that confuse editors.

Sebobo · 2023-06-28T06:48:52Z

Neos.ContentRepository/Classes/Eel/FlowQueryOperations/CanEvaluateNodeContextTrait.php

+            return count($context) > 0 ? reset($context) instanceof NodeInterface : true;
+        } elseif ($context instanceof \Traversable) {
+            foreach ($context as $item) {
+                return $item instanceof NodeInterface;


This would return true if the first array item is a node, but the second is not.

yes but this was done previously like this as well. Its just super simple duck-typing to speed up performance ...

Then please adjust the docblock above to describe the reasoning etc.

mhsdesign · 2023-06-28T08:01:16Z

elseif ($context instanceof \Traversable) {
            foreach ($context as $item) {

i dislike having a side effect in the method canEvaluate. Imo it should be pure and not change the pointer of a traversable.

foreach will call rewind and next and current on the traversable and thus modifies the state. Maybe current($traversable); does as-well?

mficzel · 2023-06-28T08:10:04Z

That is quite expensive to check the whole context array but given the current architecture it would be the only thing to do. Not sure it is wise to add this to the 8.3 branch.

If done for Neos 9 we could use the Nodes or References objects into the flowQuery context that are already iterable. That way we would only have to check with a single instanceof Nodes. This would still require changes to flowQuery in 9.0 but especially to the q() function that would have to create a Nodes object but for 9.0 there is. a chance to do this right.

patricekaufmann · 2023-06-28T09:40:53Z

i dislike having a side effect in the method canEvaluate. Imo it should be pure and not change the pointer of a traversable.

foreach will call rewind and next and current on the traversable and thus modifies the state. Maybe current($traversable); does as-well?

The pointer might point to other elements. However the first element should be checked to avoid changing what the function actually does. Getting the first element without resetting the pointer would actually be the first proposed solution in #3624 with array_key_first.

mhsdesign · 2023-11-12T08:14:53Z

Ill close this pr for now. See #3624 (comment)

…eferences EEL can only operate on an array of nodes if the `[0]` item is a node (simple duck typing) Instead of fixing eel like proposed here neos#3946 with this fix we avoid returning non 0 indexed arrays here: ``` ${q(node).property("someReference")} ``` Currently, 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 neos@87804e1 ^^

…ences EEL can only operate on an array of nodes if the `[0]` item is a node (simple duck typing) Instead of fixing eel like proposed here neos/neos-development-collection#3946 with this fix we avoid returning non 0 indexed arrays here: ``` ${q(node).property("someReference")} ``` Currently, 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/8dc96a201dc9602d96c6d06b0c6090222cd5a198/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 neos/neos-development-collection@da3f4df ^^

crydotsnake and others added 12 commits November 2, 2022 11:22

TASK: prevent first array element from being accessed via $context

e0fa10d

TASK: fix styleci

7f7f71f

TASK: switch to TraversableNodeInterface class in mockNode

8ca43bd

TASK: set condition to default

2fb441d

TASK: set conditions back to default

f99caa0

TASK: set conditions back to default

935db18

TASK: Centralize canEvaluate method in ContentRepository FlowQuery op…

b9233c2

…erations The new implementation checks that the first item is a NodeInterface regardless wether the array key is 0 or the item is inside a traversable.

TASK: Verify every item for find operation canEvaluate and remove unn…

3be6ae6

…eeded use statements

TASK: Adjust can evaluate method in flowQuery find operation

dddd215

The check now make a distinction between the nodeInterface and the TraversableNodeInterface

TASK: Exclude find from generic flowQuery operations test

3fe49dc

This is needed because find operates on the old NodeInterface which is not compatible with the current mocks.

TASK: apply changes from styleci

5ea2c41

TASK: Add return types

938d8c5

mhsdesign mentioned this pull request Nov 2, 2022

BUGFIX: Have FlowQuery operations reliably detect nodes during canEvaluate #3838

Closed

kdambekalns and others added 2 commits November 2, 2022 11:50

TASK: Reapply: FlowQueryOperations fix some errors found by Doctum

3f13492

Reapplies 928cc4a As i oversaw those changes in a merge conflict

TASK: FlowQueryOperations fix error found by Doctum and cleanup doc c…

fbe29aa

…omments

mhsdesign commented Nov 2, 2022

View reviewed changes

mhsdesign assigned crydotsnake Nov 2, 2022

markusguenther added Bug 7.3 labels Feb 22, 2023

crydotsnake added 2 commits May 7, 2023 11:49

TASK: cleanup

9f20b83

TASK: apply suggestions from code review

3fe0683

crydotsnake force-pushed the bugfix/flowQueryCanEvaluateNode branch from 82df5c4 to 3fe0683 Compare May 7, 2023 09:51

crydotsnake linked an issue May 7, 2023 that may be closed by this pull request

FlowQuery operations: incompatible check on first array element in canEvaluate #3624

Open

crydotsnake requested review from kdambekalns and mficzel May 7, 2023 13:03

crydotsnake requested a review from kitsunet May 23, 2023 12:02

Sebobo requested changes Jun 28, 2023

View reviewed changes

mhsdesign mentioned this pull request Nov 12, 2023

FlowQuery operations: incompatible check on first array element in canEvaluate #3624

Open

mhsdesign closed this Nov 12, 2023

mhsdesign mentioned this pull request Nov 12, 2023

BUGFIX: #3624 Node::getProperty does not always return list for references #4731

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUGFIX: Have FlowQuery operations reliably detect nodes during canEvaluate #3946

BUGFIX: Have FlowQuery operations reliably detect nodes during canEvaluate #3946

mhsdesign commented Nov 2, 2022

mhsdesign commented Nov 2, 2022

mhsdesign left a comment

mhsdesign Nov 2, 2022 •

edited

kdambekalns Jun 28, 2023

mhsdesign Nov 2, 2022

patricekaufmann commented Feb 17, 2023

Sebobo Jun 28, 2023

mhsdesign Jun 28, 2023

Sebobo Jun 28, 2023 •

edited

mhsdesign commented Jun 28, 2023

mficzel commented Jun 28, 2023

patricekaufmann commented Jun 28, 2023

mhsdesign commented Nov 12, 2023

BUGFIX: Have FlowQuery operations reliably detect nodes during canEvaluate #3946

BUGFIX: Have FlowQuery operations reliably detect nodes during canEvaluate #3946

Conversation

mhsdesign commented Nov 2, 2022

mhsdesign commented Nov 2, 2022

mhsdesign left a comment

Choose a reason for hiding this comment

mhsdesign Nov 2, 2022 • edited

Choose a reason for hiding this comment

kdambekalns Jun 28, 2023

Choose a reason for hiding this comment

mhsdesign Nov 2, 2022

Choose a reason for hiding this comment

patricekaufmann commented Feb 17, 2023

Sebobo Jun 28, 2023

Choose a reason for hiding this comment

mhsdesign Jun 28, 2023

Choose a reason for hiding this comment

Sebobo Jun 28, 2023 • edited

Choose a reason for hiding this comment

mhsdesign commented Jun 28, 2023

mficzel commented Jun 28, 2023

patricekaufmann commented Jun 28, 2023

mhsdesign commented Nov 12, 2023

mhsdesign Nov 2, 2022 •

edited

Sebobo Jun 28, 2023 •

edited