[5.x] Fix CP nav ordering for when preferences are stored in JSON SQL columns (#10809)

Author: jesseleite

src/CP/Navigation/NavPreferencesNormalizer.php

@@ -2,6 +2,7 @@
 
 namespace Statamic\CP\Navigation;
 
+use Illuminate\Support\Collection;
 use Statamic\Support\Arr;
 use Statamic\Support\Str;
 
@@ -72,11 +73,12 @@ protected function normalize()
     {
         $navConfig = collect($this->preferences);
 
-        $normalized = collect()->put('reorder', $reorder = $navConfig->get('reorder', false));
+        $normalized = collect()->put('reorder', (bool) $reorder = $navConfig->get('reorder', false));
 
         $sections = collect($navConfig->get('sections') ?? $navConfig->except('reorder'));
 
-        $sections = $sections
+        $sections = $this
+            ->normalizeToInheritsFromReorder($sections, $reorder)
             ->prepend($sections->pull('top_level') ?? '@inherit', 'top_level')
             ->map(fn ($config, $section) => $this->normalizeSectionConfig($config, $section))
             ->reject(fn ($config) => $config['action'] === '@inherit' && ! $reorder)
@@ -115,15 +117,16 @@ protected function normalizeSectionConfig($sectionConfig, $sectionKey)
 
         $normalized->put('display', $sectionConfig->get('display', false));
 
-        $normalized->put('reorder', $reorder = $sectionConfig->get('reorder', false));
+        $normalized->put('reorder', (bool) $reorder = $sectionConfig->get('reorder', false));
 
         $items = collect($sectionConfig->get('items') ?? $sectionConfig->except([
             'action',
             'display',
             'reorder',
         ]));
 
-        $items = $items
+        $items = $this
+            ->normalizeToInheritsFromReorder($items, $reorder)
             ->map(fn ($config, $itemId) => $this->normalizeItemConfig($itemId, $config, $sectionKey))
             ->keyBy(fn ($config, $itemId) => $this->normalizeItemId($itemId, $config))
             ->filter()
@@ -187,14 +190,23 @@ protected function normalizeItemConfig($itemId, $itemConfig, $sectionKey, $remov
             }
         }
 
-        // If item has children, normalize those items as well.
-        if ($children = $normalized->get('children')) {
-            $normalized->put('children', collect($children)
-                ->map(fn ($childConfig, $childId) => $this->normalizeChildItemConfig($childId, $childConfig, $sectionKey))
-                ->keyBy(fn ($childConfig, $childId) => $this->normalizeItemId($childId, $childConfig))
-                ->all());
+        // Normalize `reorder` bool.
+        if ($reorder = $normalized->get('reorder', false)) {
+            $normalized->put('reorder', (bool) $reorder);
         }
 
+        // Normalize `children`.
+        $children = $this
+            ->normalizeToInheritsFromReorder($normalized->get('children', []), $reorder)
+            ->map(fn ($childConfig, $childId) => $this->normalizeChildItemConfig($childId, $childConfig, $sectionKey))
+            ->keyBy(fn ($childConfig, $childId) => $this->normalizeItemId($childId, $childConfig))
+            ->all();
+
+        // Only output `children` in normalized output if there are any.
+        $children
+            ? $normalized->put('children', $children)
+            : $normalized->forget('children');
+
         $allowedKeys = array_merge(['action'], static::ALLOWED_NAV_ITEM_MODIFICATIONS);
 
         return $normalized->only($allowedKeys)->all();
@@ -234,11 +246,14 @@ protected function normalizeChildItemConfig($itemId, $itemConfig, $sectionKey)
             ];
         }
 
-        if (is_array($itemConfig)) {
-            Arr::forget($itemConfig, 'children');
+        $normalized = $this->normalizeItemConfig($itemId, $itemConfig, $sectionKey, false);
+
+        if (is_array($normalized)) {
+            Arr::forget($normalized, 'reorder');
+            Arr::forget($normalized, 'children');
         }
 
-        return $this->normalizeItemConfig($itemId, $itemConfig, $sectionKey, false);
+        return $normalized;
     }
 
     /**
@@ -272,6 +287,21 @@ protected function itemIsInOriginalSection($itemId, $currentSectionKey)
         return Str::startsWith($itemId, "$currentSectionKey::");
     }
 
+    /**
+     * Normalize to legacy style inherits from new `reorder: []` array schema, introduced to sidestep ordering issues in SQL.
+     */
+    protected function normalizeToInheritsFromReorder(array|Collection $items, array|bool $reorder): Collection
+    {
+        if (! is_array($reorder)) {
+            return collect($items);
+        }
+
+        return collect($reorder)
+            ->flip()
+            ->map(fn () => '@inherit')
+            ->merge($items);
+    }
+
     /**
      * Get normalized preferences.
      *

src/CP/Navigation/NavTransformer.php

@@ -12,7 +12,6 @@ class NavTransformer
     protected $coreNav;
     protected $submitted;
     protected $config;
-    protected $reorderedMinimums;
 
     /**
      * Instantiate nav transformer.
@@ -59,10 +58,9 @@ protected function removeEmptyCustomSections($submitted)
      */
     protected function transform()
     {
-        $this->config['reorder'] = $this->itemsAreReordered(
-            $this->coreNav->pluck('display_original'),
-            collect($this->submitted)->pluck('display_original'),
-            'sections'
+        $this->config['reorder'] = $this->getReorderedItems(
+            $this->coreNav->map(fn ($section) => $this->transformSectionKey($section)),
+            collect($this->submitted)->map(fn ($section) => $this->transformSectionKey($section)),
         );
 
         $this->config['sections'] = collect($this->submitted)
@@ -110,10 +108,9 @@ protected function transformSection($section, $sectionKey)
 
         $items = Arr::get($section, 'items', []);
 
-        $transformed['reorder'] = $this->itemsAreReordered(
+        $transformed['reorder'] = $this->getReorderedItems(
             $this->coreNav->pluck('items', 'display_original')->get($displayOriginal, collect())->map->id(),
             collect($items)->pluck('id'),
-            $sectionKey
         );
 
         $transformed['items'] = $this->transformItems($items, $sectionKey);
@@ -134,7 +131,7 @@ protected function transformItems($items, $parentId, $transformingChildItems = f
         return collect($items)
             ->map(fn ($item) => array_merge($item, ['id' => $this->transformItemId($item, $item['id'], $parentId, $items)]))
             ->keyBy('id')
-            ->map(fn ($item, $itemId) => $this->transformItem($item, $itemId, $parentId, $transformingChildItems))
+            ->map(fn ($item, $itemId) => $this->transformItem($item, $itemId, $transformingChildItems))
             ->all();
     }
 
@@ -174,11 +171,10 @@ protected function transformItemId($item, $id, $parentId, $items)
      *
      * @param  array  $item
      * @param  string  $itemId
-     * @param  string  $parentId
      * @param  bool  $isChild
      * @return array
      */
-    protected function transformItem($item, $itemId, $parentId, $isChild = false)
+    protected function transformItem($item, $itemId, $isChild = false)
     {
         $transformed = Arr::get($item, 'manipulations', []);
 
@@ -210,10 +206,9 @@ protected function transformItem($item, $itemId, $parentId, $isChild = false)
         $transformed['reorder'] = false;
 
         if ($children && $originalHasChildren && ! in_array($transformed['action'], ['@alias', '@create'])) {
-            $transformed['reorder'] = $this->itemsAreReordered(
+            $transformed['reorder'] = $this->getReorderedItems(
                 $originalItem->resolveChildren()->children()->map->id()->all(),
                 collect($children)->keys()->all(),
-                $itemId
             );
         }
 
@@ -254,14 +249,12 @@ protected function transformItemUrl($url)
     }
 
     /**
-     * Check if items are being reordered.
+     * Check if items are being reordered and return minimum list of item keys required to replicate saved order.
      *
      * @param  array  $originalList
      * @param  array  $newList
-     * @param  string  $parentKey
-     * @return bool
      */
-    protected function itemsAreReordered($originalList, $newList, $parentKey)
+    protected function getReorderedItems($originalList, $newList): bool|array
     {
         $itemsAreReordered = collect($originalList)
             ->intersect($newList)
@@ -271,21 +264,22 @@ protected function itemsAreReordered($originalList, $newList, $parentKey)
             ->reject(fn ($pair) => $pair->first() === $pair->last())
             ->isNotEmpty();
 
-        if ($itemsAreReordered) {
-            $this->trackReorderedMinimums($originalList, $newList, $parentKey);
+        if (! $itemsAreReordered) {
+            return false;
         }
 
-        return $itemsAreReordered;
+        return collect($newList)
+            ->take($this->calculateMinimumItemsForReorder($originalList, $newList))
+            ->all();
     }
 
     /**
-     * Track minimum number of items needed for reorder config.
+     * Calculate minimum number of items needed for reorder config.
      *
      * @param  array  $originalList
      * @param  array  $newList
-     * @param  string  $parentKey
      */
-    protected function trackReorderedMinimums($originalList, $newList, $parentKey)
+    protected function calculateMinimumItemsForReorder($originalList, $newList): int
     {
         $continueRejecting = true;
 
@@ -301,7 +295,7 @@ protected function trackReorderedMinimums($originalList, $newList, $parentKey)
             })
             ->count();
 
-        $this->reorderedMinimums[$parentKey] = max(1, $minimumItemsCount - 1);
+        return max(1, $minimumItemsCount - 1);
     }
 
     /**
@@ -326,15 +320,18 @@ protected function findOriginalItem($id)
      */
     protected function minify()
     {
-        $this->config['sections'] = collect($this->config['sections'])
-            ->map(fn ($section, $key) => $this->minifySection($section, $key))
+        $sections = collect($this->config['sections'])
+            ->map(fn ($section) => $this->minifySection($section))
+            ->pipe(fn ($sections) => $this->rejectInherits($sections));
+
+        $reorder = collect(Arr::get($this->config, 'reorder') ?: [])
+            ->reject(fn ($section) => $section === 'top_level')
+            ->values()
             ->all();
 
-        if ($this->config['reorder'] === true) {
-            $this->config['sections'] = $this->rejectUnessessaryInherits($this->config['sections'], 'sections');
-        } else {
-            $this->config = $this->rejectAllInherits($this->config['sections']);
-        }
+        $this->config = $reorder
+            ? array_filter(compact('reorder', 'sections'))
+            : $sections;
 
         // If the config is completely null after minifying, ensure we save an empty array.
         // For example, if we're transforming this config for a user's nav preferences,
@@ -351,21 +348,17 @@ protected function minify()
      * Minify tranformed section.
      *
      * @param  array  $section
-     * @param  string  $sectionKey
      * @return mixed
      */
-    protected function minifySection($section, $sectionKey)
+    protected function minifySection($section)
     {
         $action = Arr::get($section, 'action');
 
         $section['items'] = collect($section['items'])
-            ->map(fn ($item, $key) => $this->minifyItem($item, $key))
-            ->all();
+            ->map(fn ($item) => $this->minifyItem($item))
+            ->pipe(fn ($items) => $this->rejectInherits($items));
 
-        if ($section['reorder'] === true) {
-            $section['items'] = $this->rejectUnessessaryInherits($section['items'], $sectionKey);
-        } else {
-            $section['items'] = $this->rejectAllInherits($section['items']);
+        if (! $section['reorder']) {
             Arr::forget($section, 'reorder');
         }
 
@@ -390,21 +383,16 @@ protected function minifySection($section, $sectionKey)
      * Minify tranformed item.
      *
      * @param  array  $item
-     * @param  string  $itemKey
+     * @param  bool  $isChild
      * @return array
      */
-    protected function minifyItem($item, $itemKey, $isChild = false)
+    protected function minifyItem($item, $isChild = false)
     {
-        $action = Arr::get($item, 'action');
-
         $item['children'] = collect($item['children'] ?? [])
-            ->map(fn ($item, $childId) => $this->minifyItem($item, $childId, true))
-            ->all();
+            ->map(fn ($item) => $this->minifyItem($item, true))
+            ->pipe(fn ($items) => $this->rejectInherits($items));
 
-        if ($item['reorder'] === true) {
-            $item['children'] = $this->rejectUnessessaryInherits($item['children'], $itemKey);
-        } else {
-            $item['children'] = $this->rejectAllInherits($item['children']);
+        if (! $item['reorder']) {
             Arr::forget($item, 'reorder');
         }
 
@@ -432,7 +420,7 @@ protected function minifyItem($item, $itemKey, $isChild = false)
      * @param  array  $items
      * @return array
      */
-    protected function rejectAllInherits($items)
+    protected function rejectInherits($items)
     {
         $items = collect($items)->reject(fn ($item) => $item === '@inherit');
 
@@ -443,37 +431,6 @@ protected function rejectAllInherits($items)
         return $items->all();
     }
 
-    /**
-     * Reject unessessary `@inherit`s at end of array.
-     *
-     * @param  array  $items
-     * @param  string  $parentKey
-     * @return array
-     */
-    protected function rejectUnessessaryInherits($items, $parentKey)
-    {
-        if (! $reorderedMinimum = $this->reorderedMinimums[$parentKey] ?? false) {
-            return $items;
-        }
-
-        $keyValuePairs = collect($items)
-            ->map(fn ($item, $key) => ['key' => $key, 'value' => $item])
-            ->values()
-            ->keyBy(fn ($keyValuePair, $index) => $index + 1);
-
-        $trailingInherits = $keyValuePairs
-            ->reverse()
-            ->takeUntil(fn ($item) => $item['value'] !== '@inherit');
-
-        $modifiedMinimum = $keyValuePairs->count() - $trailingInherits->count();
-
-        $actualMinimum = max($reorderedMinimum, $modifiedMinimum);
-
-        return collect($items)
-            ->take($actualMinimum)
-            ->all();
-    }
-
     /**
      * Get config.
      *