BUGFIX: make moving of nodes more robust

Moving of nodes did not work when the *rendered parent node* in DOM was not the parent node in the node tree. In this case, the following happened: - the rendered parent node was transmitted as "parentDomAddress.contextPath" and made available in AbstractChange as getParentNode() - in AbstractMove::canApply(), the "getParentNode()" method was called, and taken into account for constraint checks -- so if the rendered parent node was a different node than the actual parent (e.g. the grandparent), the constraints might not have matched and the move could not succeed. To fix this, I changed the following: - removed AbstractStructuralChange::getParentNode as this is not really the parent node, but the closest rendered node. - pushed canApply() down; not relying on getParentNode() anymore - for the Copy/Move INTO cases, add a specific property "parentContextPath" to the change; which contains the parent node without further processing. - we ensure the renderedParentNode === parentNode (==is the ContentCollection), otherwise out of band rendering can not work (and we need to reload the full page instead).
theDex · Mar 6, 2018 · 63b79f5 · 63b79f5
1 parent 420db69
commit 63b79f5
Show file tree

Hide file tree

Showing 11 changed files with 179 additions and 52 deletions.
diff --git a/Classes/Neos/Neos/Ui/Domain/Model/Changes/AbstractCopy.php b/Classes/Neos/Neos/Ui/Domain/Model/Changes/AbstractCopy.php
@@ -23,18 +23,6 @@ abstract class AbstractCopy extends AbstractStructuralChange
      */
     protected $contentRepositoryNodeService;
 
-    /**
-     * Checks whether this change can be applied to the subject
-     *
-     * @return boolean
-     */
-    public function canApply()
-    {
-        $nodeType = $this->getSubject()->getNodeType();
-
-        return $this->getParentNode()->isNodeTypeAllowedAsChildNode($nodeType);
-    }
-
     /**
      * Generate a unique node name for the copied node
      *

diff --git a/Classes/Neos/Neos/Ui/Domain/Model/Changes/AbstractMove.php b/Classes/Neos/Neos/Ui/Domain/Model/Changes/AbstractMove.php
@@ -17,17 +17,6 @@
 
 abstract class AbstractMove extends AbstractStructuralChange
 {
-    /**
-     * Checks whether this change can be applied to the subject
-     *
-     * @return boolean
-     */
-    public function canApply()
-    {
-        $nodeType = $this->getSubject()->getNodeType();
-
-        return $this->getParentNode()->isNodeTypeAllowedAsChildNode($nodeType);
-    }
 
     /**
      * Perform finish tasks - needs to be called from inheriting class on `apply`
@@ -42,6 +31,7 @@ protected function finish(NodeInterface $node)
 
         $this->feedbackCollection->add($removeNode);
 
+        // $this->getSubject() is the moved node at the NEW location!
         parent::finish($this->getSubject());
     }
 

diff --git a/Classes/Neos/Neos/Ui/Domain/Model/Changes/AbstractStructuralChange.php b/Classes/Neos/Neos/Ui/Domain/Model/Changes/AbstractStructuralChange.php
@@ -45,11 +45,6 @@ abstract class AbstractStructuralChange extends AbstractChange
      */
     protected $nodeService;
 
-    /**
-     * @var NodeInterface
-     */
-    protected $cachedParentNode = null;
-
     /**
      * @var NodeInterface
      */
@@ -74,7 +69,9 @@ public function setParentDomAddress(RenderedNodeDomAddress $parentDomAddress = n
     }
 
     /**
-     * Get the parent node dom address
+     * Get the DOM address of the closest RENDERED node in the DOM tree.
+     *
+     * DOES NOT HAVE TO BE THE PARENT NODE!
      *
      * @return RenderedNodeDomAddress
      */
@@ -83,26 +80,6 @@ public function getParentDomAddress()
         return $this->parentDomAddress;
     }
 
-    /**
-     * Get the parent node
-     *
-     * @return NodeInterface
-     */
-    public function getParentNode()
-    {
-        if ($this->parentDomAddress === null) {
-            return null;
-        }
-
-        if ($this->cachedParentNode === null) {
-            $this->cachedParentNode = $this->nodeService->getNodeFromContextPath(
-                $this->parentDomAddress->getContextPath()
-            );
-        }
-
-        return $this->cachedParentNode;
-    }
-
     /**
      * Set the sibling node dom address
      *
@@ -167,9 +144,16 @@ protected function finish(NodeInterface $node)
         $this->updateWorkspaceInfo();
 
         if ($node->getNodeType()->isOfType('Neos.Neos:Content') && ($this->getParentDomAddress() || $this->getSiblingDomAddress())) {
+
+            // we can ONLY render out of band if:
+            // 1) the parent of our new (or copied or moved) node is a ContentCollection; so we can directly update an element of this content collection
             if ($node->getParent()->getNodeType()->isOfType('Neos.Neos:ContentCollection') &&
+
+                // 2) the parent DOM address (i.e. the closest RENDERED node in DOM is actually the ContentCollection; and
+                //    no other node in between
                 $this->getParentDomAddress() &&
-                $this->getParentDomAddress()->getFusionPath()
+                $this->getParentDomAddress()->getFusionPath() &&
+                $this->getParentDomAddress()->getContextPath() === $node->getParent()->getContextPath()
             ) {
                 $renderContentOutOfBand = new RenderContentOutOfBand();
                 $renderContentOutOfBand->setNode($node);

diff --git a/Classes/Neos/Neos/Ui/Domain/Model/Changes/CopyAfter.php b/Classes/Neos/Neos/Ui/Domain/Model/Changes/CopyAfter.php
@@ -13,6 +13,20 @@
 
 class CopyAfter extends AbstractCopy
 {
+
+    /**
+     * "Subject" is the to-be-copied node; the "sibling" node is the node after which the "Subject" should be copied.
+     *
+     * @return boolean
+     */
+    public function canApply()
+    {
+        $nodeType = $this->getSubject()->getNodeType();
+
+        return $this->getSiblingNode()->getParent()->isNodeTypeAllowedAsChildNode($nodeType);
+    }
+
+
     public function getMode()
     {
         return 'after';
@@ -26,7 +40,7 @@ public function getMode()
     public function apply()
     {
         if ($this->canApply()) {
-            $nodeName = $this->generateUniqueNodeName($this->getParentNode());
+            $nodeName = $this->generateUniqueNodeName($this->getSiblingNode()->getParent());
             $node = $this->getSubject()->copyAfter($this->getSiblingNode(), $nodeName);
             $this->finish($node);
         }

diff --git a/Classes/Neos/Neos/Ui/Domain/Model/Changes/CopyBefore.php b/Classes/Neos/Neos/Ui/Domain/Model/Changes/CopyBefore.php
@@ -13,6 +13,19 @@
 
 class CopyBefore extends AbstractCopy
 {
+    /**
+     * "Subject" is the to-be-copied node; the "sibling" node is the node after which the "Subject" should be copied.
+     *
+     * @return boolean
+     */
+    public function canApply()
+    {
+        $nodeType = $this->getSubject()->getNodeType();
+
+        return $this->getSiblingNode()->getParent()->isNodeTypeAllowedAsChildNode($nodeType);
+    }
+
+
     public function getMode()
     {
         return 'before';
@@ -26,7 +39,7 @@ public function getMode()
     public function apply()
     {
         if ($this->canApply()) {
-            $nodeName = $this->generateUniqueNodeName($this->getParentNode());
+            $nodeName = $this->generateUniqueNodeName($this->getSiblingNode()->getParent());
             $node = $this->getSubject()->copyBefore($this->getSiblingNode(), $nodeName);
             $this->finish($node);
         }

diff --git a/Classes/Neos/Neos/Ui/Domain/Model/Changes/CopyInto.php b/Classes/Neos/Neos/Ui/Domain/Model/Changes/CopyInto.php
@@ -1,4 +1,5 @@
 <?php
+
 namespace Neos\Neos\Ui\Domain\Model\Changes;
 
 /*
@@ -11,8 +12,57 @@
  * source code.
  */
 
+use Neos\ContentRepository\Domain\Model\NodeInterface;
+
 class CopyInto extends AbstractCopy
 {
+
+
+    /**
+     * @var string
+     */
+    protected $parentContextPath;
+
+    /**
+     * @var NodeInterface
+     */
+    protected $cachedParentNode;
+
+    /**
+     * @param string $parentContextPath
+     */
+    public function setParentContextPath($parentContextPath)
+    {
+        $this->parentContextPath = $parentContextPath;
+    }
+
+    /**
+     * @return NodeInterface
+     */
+    public function getParentNode()
+    {
+        if ($this->cachedParentNode === null) {
+            $this->cachedParentNode = $this->nodeService->getNodeFromContextPath(
+                $this->parentContextPath
+            );
+        }
+
+        return $this->cachedParentNode;
+    }
+
+    /**
+     * "Subject" is the to-be-copied node; the "parent" node is the new parent
+     *
+     * @return boolean
+     */
+    public function canApply()
+    {
+        $nodeType = $this->getSubject()->getNodeType();
+
+        return $this->getParentNode()->isNodeTypeAllowedAsChildNode($nodeType);
+    }
+
+
     public function getMode()
     {
         return 'into';

diff --git a/Classes/Neos/Neos/Ui/Domain/Model/Changes/Create.php b/Classes/Neos/Neos/Ui/Domain/Model/Changes/Create.php
@@ -13,6 +13,15 @@
 
 class Create extends AbstractCreate
 {
+
+    /**
+     * @param string $parentContextPath
+     */
+    public function setParentContextPath($parentContextPath)
+    {
+        // this method needs to exist; otherwise the TypeConverter breaks.
+    }
+
     /**
      * Get the insertion mode (before|after|into) that is represented by this change
      *

diff --git a/Classes/Neos/Neos/Ui/Domain/Model/Changes/MoveAfter.php b/Classes/Neos/Neos/Ui/Domain/Model/Changes/MoveAfter.php
@@ -15,6 +15,19 @@
 
 class MoveAfter extends AbstractMove
 {
+    /**
+     * "Subject" is the to-be-moved node; the "sibling" node is the node after which the "Subject" should be copied.
+     *
+     * @return boolean
+     */
+    public function canApply()
+    {
+        $nodeType = $this->getSubject()->getNodeType();
+
+        return $this->getSiblingNode()->getParent()->isNodeTypeAllowedAsChildNode($nodeType);
+    }
+
+
     public function getMode()
     {
         return 'after';

diff --git a/Classes/Neos/Neos/Ui/Domain/Model/Changes/MoveBefore.php b/Classes/Neos/Neos/Ui/Domain/Model/Changes/MoveBefore.php
@@ -15,6 +15,19 @@
 
 class MoveBefore extends AbstractMove
 {
+
+    /**
+     * "Subject" is the to-be-moved node; the "sibling" node is the node after which the "Subject" should be copied.
+     *
+     * @return boolean
+     */
+    public function canApply()
+    {
+        $nodeType = $this->getSubject()->getNodeType();
+
+        return $this->getSiblingNode()->getParent()->isNodeTypeAllowedAsChildNode($nodeType);
+    }
+
     public function getMode()
     {
         return 'before';

diff --git a/Classes/Neos/Neos/Ui/Domain/Model/Changes/MoveInto.php b/Classes/Neos/Neos/Ui/Domain/Model/Changes/MoveInto.php
@@ -11,15 +11,67 @@
  * source code.
  */
 
+use Neos\ContentRepository\Domain\Model\NodeInterface;
 use Neos\Neos\Ui\Domain\Model\Feedback\Operations\UpdateNodeInfo;
 
 class MoveInto extends AbstractMove
 {
+
+    /**
+     * @var string
+     */
+    protected $parentContextPath;
+
+    /**
+     * @var NodeInterface
+     */
+    protected $cachedParentNode;
+
+    /**
+     * @param string $parentContextPath
+     */
+    public function setParentContextPath($parentContextPath)
+    {
+        $this->parentContextPath = $parentContextPath;
+    }
+
+
+    /**
+     * Get the insertion mode (before|after|into) that is represented by this change
+     *
+     * @return string
+     */
     public function getMode()
     {
         return 'into';
     }
 
+    /**
+     * @return NodeInterface
+     */
+    public function getParentNode()
+    {
+        if ($this->cachedParentNode === null) {
+            $this->cachedParentNode = $this->nodeService->getNodeFromContextPath(
+                $this->parentContextPath
+            );
+        }
+
+        return $this->cachedParentNode;
+    }
+
+    /**
+     * "Subject" is the to-be-copied node; the "parent" node is the new parent
+     *
+     * @return boolean
+     */
+    public function canApply()
+    {
+        $nodeType = $this->getSubject()->getNodeType();
+
+        return $this->getParentNode()->isNodeTypeAllowedAsChildNode($nodeType);
+    }
+
     /**
      * Applies this change
      *

diff --git a/packages/neos-ui/src/Sagas/CR/NodeOperations/helpers.js b/packages/neos-ui/src/Sagas/CR/NodeOperations/helpers.js
@@ -49,6 +49,7 @@ export const calculateDomAddressesFromMode = (mode, contextPath, fusionPath) =>
             const element = findNodeInGuestFrame(contextPath, fusionPath);
 
             return {
+                parentContextPath: contextPath,
                 parentDomAddress: {
                     contextPath: element ? element.getAttribute('data-__neos-node-contextpath') : contextPath,
                     fusionPath: element ? element.getAttribute('data-__neos-fusion-path') : fusionPath