BUGFIX: Respect removed-state while reducing NodeData results

[Nikdro]

(Hopefully) fixes #3651

Please read Issue #3651, to get the problem.

By adding the removed-state as 3rd priority while reducing NodeData results, we should always get the not-removed NodeData, if there are 2 (because of Node-move).

I tested this fix on our projects, and it worked. I can't find something breaking. But since this is deep inside NodeData, I've no idea, if we could get unexpected side-effects on it.

[kdambekalns]

👍

By reading the issue and the change, this seems reasonable and should do no harm. Since I'd love to have some more reviews, I'll just leave this as a comment for now…

[bwaidelich]

Can we come up with a unit or functional test for this?

[Nikdro]

It's hard for me, to get the current state here? Do we need additional Tests here?
I've never done something with tests on Neos core. So right now I'm a bit without a plan as to how to proceed.
Is there anything I can/must do so that we can get the PR in master?

[kitsunet]

I guess this must be a behat test then, we should probably have all tools for it in the trait, so it's just a question of coming up with a behat description that fails without the change and succeeds with it. Given that sorting is semi random in those queries (maybe persistence identifier as primary key has influence?) that might be a tricky part for the test but at least there should be something.

In general I like the change, it makes a lot of sense, but somewhere in the back of my head a little alarm is saying this existed before and had some other unwanted effect, BUT I couldn't find any evidence of that so I guess that worry is unfounded.

[markusguenther]

Yesterday I stumbled over a similar issue and after checking the code in the Neos-UI @kitsunet led me to the NodeData and NodeDataRepository. As @Nikdro already described, when we move e.g. a node into the own parent page, we create a shadow node that is marked as removed and we have the regular node that is moved and not marked as removed.

For some reason, the NodeDataRepository chooses the shadow node as all are filtered by identifier. The change from Niklas changes the position and so we get the moved node and not the shadow node. It would be better here to check in the UI if the target node is also the parent node and so not create a shadow node, but as this can also have implications these bugfixes seem to be a nice approach.

Behavoir before this change:

Move-node-into-same-parent-with-bug.-.01.mp4

Behavoir after this change:

Move-node-into-same-parent-with-fix.1.-.01.mp4

The only thing I could find was an issue when you move the node into the parent, publish it, and then move the node into the same parent again, without reloading the page. This does not happen when you reload the page after publishing.
So it seems that we have something wrong in the UI.

Move-node-into-same-parent-with-fix-exception-before-reload.1.-.01.mp4

I will rebase this change to Neos 8.3, and then we can hopefully continue with the discussion here. Seems that the only concern was the missing test :) With Neos 9.0, this should be different anyways.

[markusguenther]

I’ll close this PR in favor of the copycat for Neos 8.3. ;)