Prevent cascading container conversion on resource move (#16001)

* Fix issue where moving resource above another would make the "target" a container

* Reformat code according to code style

* Return failures

Co-authored-by: Mark Hamstra <[email protected]>

Co-authored-by: Jason Coward <[email protected]>
Co-authored-by: Mark Hamstra <[email protected]>

Author: 3 people

core/src/Revolution/Processors/Resource/Sort.php

@@ -1,4 +1,5 @@
 <?php
+
 /*
  * This file is part of MODX Revolution.
  *
@@ -10,7 +11,6 @@
 
 namespace MODX\Revolution\Processors\Resource;
 
-
 use MODX\Revolution\modContext;
 use MODX\Revolution\Processors\Processor;
 use MODX\Revolution\modResource;
@@ -53,9 +53,13 @@ public function process()
 
         // Because of BC
         $data = urldecode($this->getProperty('data', ''));
-        if (empty($data)) $this->failure($this->modx->lexicon('invalid_data'));
+        if (empty($data)) {
+            return $this->failure($this->modx->lexicon('invalid_data'));
+        }
         $data = $this->modx->fromJSON($data);
-        if (empty($data)) $this->failure($this->modx->lexicon('invalid_data'));
+        if (empty($data)) {
+            return $this->failure($this->modx->lexicon('invalid_data'));
+        }
 
         // Because of BC
         $this->getNodesFormatted($data, $this->getProperty('parent', 0));
@@ -66,7 +70,7 @@ public function process()
         $source = $this->getProperty('source', '');
         $point = $this->getProperty('point', '');
 
-        if (empty ($target)) {
+        if (empty($target)) {
             return $this->failure('Target not set');
         }
 
@@ -83,7 +87,9 @@ public function process()
         $this->parseNodes($source, $target);
 
         $sorted = $this->sort();
-        if ($sorted !== true) return $this->failure($sorted);
+        if ($sorted !== true) {
+            return $this->failure($sorted);
+        }
 
         $this->fireAfterSort();
 
@@ -151,7 +157,6 @@ public function fireAfterSort()
             'contextsAffected' => &$this->contextsAffected,
             'modifiedNodes' => &$this->nodesAffected, /* backward compat */
         ]);
-
     }
 
     public function clearCache()
@@ -174,8 +179,12 @@ public function fixParents($node)
         /** @var modResource $newParent */
         $newParent = $this->modx->getObject(modResource::class, $this->target->id);
 
-        if (empty($oldParent) && empty($newParent)) return;
-        if ($oldParent->id == $newParent->id) return;
+        if (empty($oldParent) && empty($newParent)) {
+            return;
+        }
+        if ($oldParent->id == $newParent->id) {
+            return;
+        }
 
         if (!empty($oldParent)) {
             $oldParentChildrenCount = $this->modx->getCount(modResource::class, ['parent' => $oldParent->get('id'), 'id:!=' => $node->id]);
@@ -189,7 +198,7 @@ public function fixParents($node)
             }
         }
 
-        if (!empty($newParent)) {
+        if (!empty($newParent) && $this->point === 'append') {
             $newParent->set('isfolder', true);
             $newParent->save();
 
@@ -343,10 +352,14 @@ public function moveResource($type, $rank)
     public function moveResourceAbove($lastRank)
     {
         $moved = $this->moveResource('source', $lastRank);
-        if ($moved !== true) return $moved;
+        if ($moved !== true) {
+            return $moved;
+        }
 
         $moved = $this->moveResource('target', $lastRank + 1);
-        if ($moved !== true) return $moved;
+        if ($moved !== true) {
+            return $moved;
+        }
 
         return $this->moveAffectedResources($lastRank);
     }