Bug description
This is basically a duplicate of #11209, but I wanted to create a new bug report since:
a) I've actually been able to reproduce this issue
b) I sent the the clankers after it and they seem to have found the underlying issue
I hate to slop-grenade but it's an issue affecting quite a few people, including myself, so figure the extra context is better than nothing.
Root cause
Three things combine:
1. Tree::tree() memoises the healed tree under a key that ignores the entry set.
|
public function tree($tree = null) |
|
{ |
|
return $this->fluentlyGetOrSet('tree') |
|
->getter(function ($tree) { |
|
$key = "structure-{$this->handle()}-{$this->locale()}-".md5(json_encode($tree)); |
|
|
|
return Blink::once($key, function () use ($tree) { |
|
return $this->structure()->validateTree($tree, $this->locale()); |
|
}); |
|
}) |
|
->setter(function ($tree) { |
|
return $this->removeNullItems($tree); |
|
}) |
|
->args(func_get_args()); |
|
} |
CollectionStructure::validateTree() appends entries that aren't in the stored tree (L77-L81), so its result depends on both the stored tree and the collection's entries. The cache key (L52) only covers the stored tree. Creating an entry changes the entry set without changing the stored tree, so the key is unchanged and the stale healed tree — the one that predates the new entry — is served for the rest of the request.
2. Orderable collections never persist new entries to the tree file.
|
private function addToStructure($site, $parent = null): ?Closure |
|
{ |
|
// If it's orderable (linear - a max depth of 1) then don't add it. |
|
if ($this->collection()->orderable()) { |
|
return null; |
|
} |
So the healing in (1) is the only thing that puts these entries in the tree, which makes the staleness in (1) directly observable.
3. Entry::order() degrades to 1 instead of failing.
|
public function order() |
|
{ |
|
if (! $this->hasStructure()) { |
|
return $this->value('order'); |
|
} |
|
|
|
return $this->structure()->in($this->locale())->entryOrder($this->id) + 1; |
|
} |
entryOrder() returns null when the reference isn't in the tree, and null + 1 === 1. So an
entry the tree can't find is indexed as the first entry rather than erroring or returning null.
Because order is 1-based, this is a collision, not merely a low value: the entry legitimately at tree index 0 also has order = 1. The two tie, and the tie breaks in favour of the entry already in the index, so the new entry surfaces in row 2 of the listing.
Putting it together: during $entry->save(), the order index calls $entry>order(). If anything earlier in that request already read the tree — which any real CP request does — step 1 serves a healed tree without the new entry, step 3 turns that into order = 1, and with the watcher off nothing ever recomputes it.
Then the reorder corrupts the tree:
|
$contents |
|
->keys() |
|
->forPage($request->page, $request->perPage) |
|
->zip($reorderPayload) |
|
->each(function ($operation) use ($contents, &$reorderedEntries) { |
|
$reorderedEntries->put( |
|
$operation[0], |
|
$contents->get($operation[1]) |
|
); |
|
}); |
This is only safe when the posted ids and the tree slice are the same set. When they aren't, put() writes a value that already exists elsewhere (→ duplicate) over a key whose own value is never written back (→ silently deleted).
How to reproduce
This is a repo for the error reproduction:
https://github.com/JonKaric/reorderbug
You'll need to set:
.env STATAMIC_STACHE_WATCHER must be set to false
The repo already sets:
The collection must be orderable (tested with max_depth: 1)
Must have more entries than the default pagination size.
To reproduce:
- Press the reorder button and save without actually reordering (might be optional, but the stache needs to have the up-to-date tree)
- Create an entry
- You'll be sent back to the listings
- Press Reorder
- Press save OR Reorder the new entry within the same pagination page
Logs
Environment
Environment
Laravel Version: 13.23.0
PHP Version: 8.5.3
Composer Version: 2.9.5
Environment: local
Debug Mode: ENABLED
Maintenance Mode: OFF
Timezone: UTC
Locale: en
Cache
Config: NOT CACHED
Events: NOT CACHED
Routes: NOT CACHED
Views: CACHED
Drivers
Broadcasting: log
Cache: file
Database: sqlite
Logs: stack / single
Mail: log
Queue: sync
Session: file
Storage
public/storage: NOT LINKED
Statamic
Addons: 0
License Key: Not set
Sites: 1
Stache Watcher: Disabled
Static Caching: Disabled
Version: 6.26.0 PRO
Installation
Fresh statamic/statamic site via CLI
Additional details
No response
Bug description
This is basically a duplicate of #11209, but I wanted to create a new bug report since:
a) I've actually been able to reproduce this issue
b) I sent the the clankers after it and they seem to have found the underlying issue
I hate to slop-grenade but it's an issue affecting quite a few people, including myself, so figure the extra context is better than nothing.
Root cause
Three things combine:
1.
Tree::tree()memoises the healed tree under a key that ignores the entry set.cms/src/Structures/Tree.php
Lines 48 to 62 in d20dd36
CollectionStructure::validateTree()appends entries that aren't in the stored tree (L77-L81), so its result depends on both the stored tree and the collection's entries. The cache key (L52) only covers the stored tree. Creating an entry changes the entry set without changing the stored tree, so the key is unchanged and the stale healed tree — the one that predates the new entry — is served for the rest of the request.2. Orderable collections never persist new entries to the tree file.
cms/src/Entries/Entry.php
Lines 915 to 920 in d20dd36
So the healing in (1) is the only thing that puts these entries in the tree, which makes the staleness in (1) directly observable.
3.
Entry::order()degrades to1instead of failing.cms/src/Entries/Entry.php
Lines 558 to 565 in d20dd36
entryOrder()returnsnullwhen the reference isn't in the tree, andnull + 1 === 1. So anentry the tree can't find is indexed as the first entry rather than erroring or returning null.
Because
orderis 1-based, this is a collision, not merely a low value: the entry legitimately at tree index0also hasorder = 1. The two tie, and the tie breaks in favour of the entry already in the index, so the new entry surfaces in row 2 of the listing.Putting it together: during
$entry->save(), theorderindex calls$entry>order(). If anything earlier in that request already read the tree — which any real CP request does — step 1 serves a healed tree without the new entry, step 3 turns that intoorder = 1, and with the watcher off nothing ever recomputes it.Then the reorder corrupts the tree:
cms/src/Http/Controllers/CP/Collections/ReorderEntriesController.php
Lines 33 to 42 in d20dd36
This is only safe when the posted ids and the tree slice are the same set. When they aren't,
put()writes a value that already exists elsewhere (→ duplicate) over a key whose own value is never written back (→ silently deleted).How to reproduce
This is a repo for the error reproduction:
https://github.com/JonKaric/reorderbug
You'll need to set:
.env
STATAMIC_STACHE_WATCHERmust be set tofalseThe repo already sets:
The collection must be orderable (tested with
max_depth: 1)Must have more entries than the default pagination size.
To reproduce:
Logs
Environment
Installation
Fresh statamic/statamic site via CLI
Additional details
No response