[5.x] CP nav reordering fixes

src/CP/Navigation/NavBuilder.php

@@ -3,6 +3,7 @@
 namespace Statamic\CP\Navigation;
 
 use Exception;
+use Illuminate\Support\Collection;
 use Illuminate\Support\Facades\Cache;
 use Statamic\Facades\Blink;
 use Statamic\Facades\Preference;
@@ -251,8 +252,8 @@ protected function applyPreferenceOverrides($preferences = null)
             ->each(fn ($overrides) => $this->createPendingItemsForSection($overrides))
             ->each(fn ($overrides) => $this->applyPreferenceOverridesForSection($overrides));
 
-        if ($navPreferencesConfig['reorder']) {
-            $this->setSectionOrder($sections);
+        if ($reorder = $navPreferencesConfig['reorder']) {
+            $this->setSectionOrder($reorder);
         }
 
         return $this;
@@ -317,8 +318,8 @@ protected function applyPreferenceOverridesForSection($sectionNav)
             ->filter()
             ->each(fn ($item) => $item->isChild(false));
 
-        if ($sectionNav['reorder']) {
-            $this->setSectionItemOrder($section, $sectionNav['items']);
+        if ($reorder = $sectionNav['reorder']) {
+            $this->setSectionItemOrder($section, $sectionNav['items'], $reorder);
         }
     }
 
@@ -441,62 +442,76 @@ protected function renameSection($sectionKey)
 
     /**
      * Set section order.
-     *
-     * @param  array  $sections
      */
-    protected function setSectionOrder($sections)
+    protected function setSectionOrder(array $reorder): void
     {
-        // Get conconfigured core sections...
+        // Get unconfigured core sections...
         $unconfiguredCoreSections = $this->sections;
 
         // Get unconfigured sections...
-        $unconfiguredRegisteredSections = collect($this->items)->map->section()->filter()->unique();
+        $unconfiguredRegisteredSections = collect($this->items)
+            ->mapWithKeys(fn ($item) => [NavItem::snakeCase($item->section()) => $item->section()])
+            ->filter()
+            ->unique();
 
-        // Merge unconfigured sections onto the end of the list and map their order...
-        $this->sectionsOrder = collect($sections)
-            ->pluck('display')
-            ->merge($unconfiguredRegisteredSections)
+        // Get merged unique list of sections...
+        $order = collect()
             ->merge($unconfiguredCoreSections)
-            ->unique()
+            ->merge($unconfiguredRegisteredSections)
+            ->unique();
+
+        // Reorder to match `$reorder` config...
+        collect($reorder)
+            ->reverse()
+            ->each(fn ($key) => $order->prepend($order->pull($key), $key));
+
+        // Ensure `top_level` is always first...
+        $order->prepend($order->pull('top_level'), 'top_level');
+
+        $this->sectionsOrder = $order
             ->values()
             ->mapWithKeys(fn ($section, $index) => [$section => $index + 1])
             ->all();
     }
 
     /**
      * Set section item order.
-     *
-     * @param  string  $section
-     * @param  array  $items
      */
-    protected function setSectionItemOrder($section, $items)
+    protected function setSectionItemOrder(string $section, array $items, array $reorder): void
     {
+        // Get unconfigured item IDs...
+        $unconfiguredItemIds = collect($this->items)
+            ->filter(fn ($item) => $item->section() === $section)
+            ->map
+            ->id();
+
         // Generate IDs for newly created items...
-        $itemIds = collect($items)
+        $createdItemIds = collect($items)
             ->map(function ($item, $id) use ($section, $items) {
                 return $items[$id]['action'] === '@create'
                     ? $this->generateNewItemId($section, $items[$id]['display'])
                     : $id;
             })
             ->values();
 
-        // Get unconfigured item IDs...
-        $unconfiguredItemIds = collect($this->items)
-            ->filter(fn ($item) => $item->section() === $section)
-            ->map
-            ->id();
-
         // Merge unconfigured items into the end of the list...
-        $itemIds = $itemIds
-            ->values()
+        $itemIds = collect()
             ->merge($unconfiguredItemIds)
+            ->merge($createdItemIds)
             ->unique()
-            ->values();
+            ->flip();
+
+        // Reorder to match `$reorder` config...
+        collect($reorder)
+            ->reverse()
+            ->each(fn ($key) => $itemIds->prepend($itemIds->pull($key), $key));
 
         // Set an explicit order value on each item...
         $itemIds
+            ->flip()
             ->map(fn ($id) => $this->findItem($id, false))
             ->filter()
+            ->values()
             ->each(fn ($item, $index) => $item->order($index + 1));
 
         // Inform builder that section items should be ordered...
@@ -668,29 +683,29 @@ protected function userModifyItem($item, $config, $section)
             ->reject(fn ($value, $setter) => in_array($setter, ['children', 'reorder']))
             ->each(fn ($value, $setter) => $item->{$setter}($value));
 
-        if ($children = $config->get('children')) {
-            $this->userModifyItemChildren($item, $children, $section, $config->get('reorder'));
+        $childrenConfig = $config->get('children');
+        $reorder = $config->get('reorder');
+
+        if ($childrenConfig || $reorder) {
+            $this->userModifyItemChildren($item, $childrenConfig, $section, $reorder);
         }
 
         return $item;
     }
 
     /**
      * Modify NavItem children.
-     *
-     * @param  \Statamic\CP\Navigation\NavItem  $item
-     * @param  array  $childrenOverrides
-     * @param  string  $section
-     * @return \Illuminate\Support\Collection
      */
-    protected function userModifyItemChildren($item, $childrenOverrides, $section, $reorder)
+    protected function userModifyItemChildren(NavItem $item, ?array $childrenConfig, string $section, ?array $reorder): void
     {
+        // Get original item children...
         $itemChildren = collect($item->original()->resolveChildren()->children())
             ->each(fn ($item, $index) => $item->order($index + 1000))
             ->keyBy
             ->id();
 
-        collect($childrenOverrides)
+        // Apply children preferences from config...
+        collect($childrenConfig)
             ->map(fn ($config, $key) => $this->userModifyChild($config, $section, $key, $item))
             ->each(function ($item, $key) use (&$itemChildren) {
                 $item
@@ -701,15 +716,20 @@ protected function userModifyItemChildren($item, $childrenOverrides, $section, $
             ->values()
             ->each(fn ($item, $index) => $item->order($index + 1));
 
-        $newChildren = $reorder
-            ? $itemChildren->sortBy(fn ($item) => $item->order())->values()
-            : $itemChildren->values();
-
-        $newChildren->each(fn ($item, $index) => $item->order($index + 1));
+        // Reorder to match `$reorder` config...
+        if ($reorder) {
+            collect($reorder)
+                ->reverse()
+                ->each(fn ($key) => $itemChildren->prepend($itemChildren->pull($key), $key));
+        }
 
-        $item->children($newChildren, false);
+        // Update final `order`...
+        $itemChildren
+            ->sortBy(fn ($item) => $item->order())->values()
+            ->values()
+            ->each(fn ($item, $index) => $item->order($index + 1));
 
-        return $newChildren;
+        $item->children($itemChildren, false);
     }
 
     /**

src/CP/Navigation/NavPreferencesNormalizer.php

@@ -73,19 +73,20 @@ protected function normalize()
     {
         $navConfig = collect($this->preferences);
 
-        $normalized = collect()->put('reorder', (bool) $reorder = $navConfig->get('reorder', false));
-
         $sections = collect($navConfig->get('sections') ?? $navConfig->except('reorder'));
 
-        $sections = $this
-            ->normalizeToInheritsFromReorder($sections, $reorder)
+        $normalized = collect();
+
+        $sections = collect($sections)
             ->prepend($sections->pull('top_level') ?? '@inherit', 'top_level')
-            ->map(fn ($config, $section) => $this->normalizeSectionConfig($config, $section))
-            ->reject(fn ($config) => $config['action'] === '@inherit' && ! $reorder)
-            ->map(fn ($config) => $this->removeInheritFromConfig($config))
-            ->all();
+            ->map(fn ($config, $section) => $this->normalizeSectionConfig($config, $section));
+
+        $normalized->put('reorder', $this->normalizeReorder(
+            $navConfig->get('reorder', false),
+            $sections->reject(fn ($section, $key) => $key === 'top_level'),
+        ));
 
-        $normalized->put('sections', $sections);
+        $normalized->put('sections', $this->rejectInherits($sections)->all());
 
         $allowedKeys = ['reorder', 'sections'];
 
@@ -117,44 +118,26 @@ protected function normalizeSectionConfig($sectionConfig, $sectionKey)
 
         $normalized->put('display', $sectionConfig->get('display', false));
 
-        $normalized->put('reorder', (bool) $reorder = $sectionConfig->get('reorder', false));
-
         $items = collect($sectionConfig->get('items') ?? $sectionConfig->except([
             'action',
             'display',
             'reorder',
         ]));
 
-        $items = $this
-            ->normalizeToInheritsFromReorder($items, $reorder)
+        $items = $items
             ->map(fn ($config, $itemId) => $this->normalizeItemConfig($itemId, $config, $sectionKey))
             ->keyBy(fn ($config, $itemId) => $this->normalizeItemId($itemId, $config))
-            ->filter()
-            ->reject(fn ($config) => $config['action'] === '@inherit' && ! $reorder)
-            ->all();
+            ->filter();
+
+        $normalized->put('reorder', $this->normalizeReorder($sectionConfig->get('reorder', false), $items));
 
-        $normalized->put('items', $items);
+        $normalized->put('items', $this->rejectInherits($items)->all());
 
         $allowedKeys = array_merge(['action'], static::ALLOWED_NAV_SECTION_MODIFICATIONS);
 
         return $normalized->only($allowedKeys)->all();
     }
 
-    /**
-     * Remove inherit action from config.
-     *
-     * @param  array  $config
-     * @return array
-     */
-    protected function removeInheritFromConfig($config)
-    {
-        if ($config['action'] === '@inherit') {
-            $config['action'] = false;
-        }
-
-        return $config;
-    }
-
     /**
      * Normalize item config.
      *
@@ -190,21 +173,19 @@ protected function normalizeItemConfig($itemId, $itemConfig, $sectionKey, $remov
             }
         }
 
-        // Normalize `reorder` bool.
-        if ($reorder = $normalized->get('reorder', false)) {
-            $normalized->put('reorder', (bool) $reorder);
-        }
-
         // Normalize `children`.
-        $children = $this
-            ->normalizeToInheritsFromReorder($normalized->get('children', []), $reorder)
+        $children = collect($normalized->get('children', []))
             ->map(fn ($childConfig, $childId) => $this->normalizeChildItemConfig($childId, $childConfig, $sectionKey))
-            ->keyBy(fn ($childConfig, $childId) => $this->normalizeItemId($childId, $childConfig))
-            ->all();
+            ->keyBy(fn ($childConfig, $childId) => $this->normalizeItemId($childId, $childConfig));
+
+        // Only output `reorder` if there are any `children`.
+        if ($children->isNotEmpty() && $reorder = $normalized->get('reorder', false)) {
+            $normalized->put('reorder', $this->normalizeReorder($reorder, $children));
+        }
 
-        // Only output `children` in normalized output if there are any.
-        $children
-            ? $normalized->put('children', $children)
+        // Only output `children` if there are any.
+        $children->isNotEmpty()
+            ? $normalized->put('children', $this->rejectInherits($children)->all())
             : $normalized->forget('children');
 
         $allowedKeys = array_merge(['action'], static::ALLOWED_NAV_ITEM_MODIFICATIONS);
@@ -268,9 +249,7 @@ protected function itemIsModified($config)
             return false;
         }
 
-        $possibleModifications = array_merge(static::ALLOWED_NAV_ITEM_MODIFICATIONS);
-
-        return collect($possibleModifications)
+        return collect(static::ALLOWED_NAV_ITEM_MODIFICATIONS)
             ->intersect(array_keys($config))
             ->isNotEmpty();
     }
@@ -288,18 +267,23 @@ protected function itemIsInOriginalSection($itemId, $currentSectionKey)
     }
 
     /**
-     * Normalize to legacy style inherits from new `reorder: []` array schema, introduced to sidestep ordering issues in SQL.
+     * Normalize legacy `reorder: true` boolean to array, introduced to sidestep ordering issues in SQL.
      */
-    protected function normalizeToInheritsFromReorder(array|Collection $items, array|bool $reorder): Collection
+    protected function normalizeReorder(array|bool $reorder, array|Collection $items): array|bool
     {
-        if (! is_array($reorder)) {
-            return collect($items);
+        if ($reorder === true) {
+            $reorder = collect($items)->keys()->all();
         }
 
-        return collect($reorder)
-            ->flip()
-            ->map(fn () => '@inherit')
-            ->merge($items);
+        return $reorder;
+    }
+
+    /**
+     * Reject items with inherit actions.
+     */
+    protected function rejectInherits(Collection $items): Collection
+    {
+        return $items->reject(fn ($item) => Arr::get($item, 'action') === '@inherit');
     }
 
     /**