Neos-ui: Unpublished nodes can't be selected when moved

[bwaidelich]

Description

When a new (unpublished) document node is moved within the new UI it sometimes can't be selected/edited any longer

Steps to Reproduce

1. Create a new Document node (e.g. in a fresh Demo Site)
   
2. Drag the new (unpublished) document node into an existing (published) node
   
3. Try to select the moved node in the tree

Expected behavior

The iFrame should load the representation of the moved node and it should be editable in the Backend

Actual behavior

(Sometimes) the new node can't be selected. Instead the homepage is selected (and loaded in the iFrame)

Background

I pinned down the problem to be related to the /neos/ui-services/change AJAX request that returns an invalid uri for the moved nodes (sometimes):

It should be: "uri": "http:\/\/domain.tld\/en\/download\/new@user-admin;language=en_US.html",
but it is: "uri": "http:\/\/domain.tld\/en@user-admin;language=en_US.html",

I suspect some kind of race condition in the Routing because it is not always reproducible and never reproducible with disabled routing caches..

Affected Versions

Neos:

3.3+

[bwaidelich]

Maybe this is also related to https://github.com/neos/neos-development-collection/pull/1721

[bwaidelich]

After another debugging session with @kitsunet I'm still clueless about the source of this problem..
What I can say is that it is not related to the routing (cache) after all because it is reproducible even with disabled caches.

One part that made us very suspicious is the clone of the subject (= node to move) in \Neos\Neos\Ui\Domain\Model\Changes\MoveInto. Cloning nodes can have a lot of side effects due to runtime caches etc.. But even with removed clone the problem persists so this is at least not the (only) reason for the issue.

[skurfuerst]

@bwaidelich maybe the change on https://github.com/neos/neos-ui/pull/1645 helps to fix the issue? Removing the clone is definitely wrong; we need information about the old location in the tree.

[skurfuerst]

I might have been hit by a similarish issue; maybe it helps to track down the issue:

• create a new page A; do not publish it.
• create a new page B as child of A; do not publish it.
• publish B.
• in backend, B is correctly shown.
• *the link to the frontend starts to become active, but it points to /b.html (which should be /a/b.html; but as the parent is not yet published, the becoming-active does not work).
• even when publishing A, the problem is not fixed due to the routing cache being filled.

this worked reliably every time ;-)

[skurfuerst]

@bwaidelich can you test #1645 whether it fixes the bug?

[bwaidelich]

@skurfuerst yes, sorry for the delay and thanks for the reminder. On it now ;)

[bwaidelich]

I can confirm that #1645 fixes the issue described above! 🎉
The node still loses focus when it is moved, but that's just a minor problem compared to the original bug.

The bug @skurfuerst described in https://github.com/neos/neos-ui/issues/1523#issuecomment-367698026 is still reproducible though.

[dimaip]

I'll close this issue then. @skurfuerst could you please create a separate issue for "your" bug? :)

[bwaidelich]

Unfortunately #1645 did _not_ fix the bug apparently.. Currently trying to investigate the root cause with @hlubek

[bwaidelich]

It looks like this is related to https://github.com/neos/neos-development-collection/pull/1654#pullrequestreview-55227128 : We try to fetch a node from the context that might not be persisted yet.. Unfortunately it's a heisenbug that is not always reproducible..

[bwaidelich]

OK, here is (most probably) what happens:
In FrontendNodeRoutePartHandler::getRequestPathByNode() we do

$currentNode = $contextAllowingHiddenNodes->getNodeByIdentifier($node->getIdentifier());

so that nodes beneath hidden nodes can be resolved.
Internally that calls NodeDataRepository::findOneByIdentifier() which returned null in the case mentioned above.

And the reason..:

The query returns two NodeData instances. The one that has been moved and the new one.
NodeDataRepository::reduceNodeVariantsByWorkspacesAndDimensions() then removes one of those. And because they have the same specifity (as they reflect the same Node)

Now the crazy part:

Depending on the persistence_object_identifier that leaves us with the "Internal" NodeData (the one that was moved). And that is removed afterwards with

$foundNodes = $this->withoutRemovedNodes($foundNodes);

so $foundNodes is empty and the method returns null.

Possible solution

NodeDataRepository::reduceNodeVariantsByWorkspacesAndDimensions() should in this case always prefer the NodeData that was not moved. Adding

$dimensionPositions[] = $node->isInternal() ? 0 : 1;

should ensure that..

But we came across other parts that already filter internal NodeData on the database level, namely in: findRawNodesByPath(), findByWorkspace(), filterNodeDataByBestMatchInContext().

To solve this consistently, we should probably remove those DB constraints and always filter internal NodeData instances on the PHP level. NodeFactory::createFromNodeData() already returns null for those.

[bwaidelich]

Forget the possible solution. @hlubek found a better way: https://github.com/neos/neos-development-collection/pull/1987

[hlubek]

Actually I also thought the reduce thing would work, but it happens that some Behat scenarios for nested workspaces break with that. Adding the movedTo filter also to findOneByIdentifier seems pretty safe to do.

[bwaidelich]

I think this is solved by neos/neos-development-collection#1987