diff --git a/src/Http/Controllers/CP/Collections/CollectionTreeController.php b/src/Http/Controllers/CP/Collections/CollectionTreeController.php index 8bec1e94bc8..7e53c6ddcbf 100644 --- a/src/Http/Controllers/CP/Collections/CollectionTreeController.php +++ b/src/Http/Controllers/CP/Collections/CollectionTreeController.php @@ -76,21 +76,22 @@ private function validateUniqueUris($tree) return; } + $siblingUris = []; + foreach ($tree->diff()->moved() as $id) { $page = $tree->find($id); $parent = $page->parent(); + $isTopLevel = ! $parent || $parent->isRoot(); + $key = $isTopLevel ? '' : $parent->id(); - $siblings = (! $parent || $parent->isRoot()) - ? $tree->pages()->all()->slice(1) - : $page->parent()->pages()->all(); - - $siblings = $siblings->reject(function ($sibling) use ($id) { - return $sibling->reference() === $id; - }); + // Moved pages under the same parent share siblings, so build that list once. + $siblingUris[$key] ??= ($isTopLevel ? $tree->pages()->all()->slice(1) : $parent->pages()->all()) + ->mapWithKeys(fn ($sibling) => [$sibling->reference() => $sibling->uri()]); - $uris = $siblings->map->uri(); + // The root is sliced off the top-level siblings, so it won't be in the list. + $uri = $siblingUris[$key][$id] ?? $page->uri(); - if ($uris->contains($uri = $page->uri())) { + if ($siblingUris[$key]->except($id)->contains($uri)) { throw ValidationException::withMessages(['uri' => trans('statamic::validation.duplicate_uri', ['value' => $uri])]); } } diff --git a/src/Structures/Page.php b/src/Structures/Page.php index d434f84caa4..8d21c5f4e82 100644 --- a/src/Structures/Page.php +++ b/src/Structures/Page.php @@ -49,6 +49,7 @@ class Page implements Arrayable, ArrayAccess, Augmentable, BulkAugmentable, Cont private $routeData; private $status; private $structure; + private $uri; public function __construct() { @@ -120,6 +121,7 @@ public function isRedirect() public function setDepth($depth) { $this->depth = $depth; + $this->uri = null; return $this; } @@ -170,6 +172,7 @@ public function setEntry($reference): self $this->reference = $reference; $this->routeData = null; + $this->uri = null; return $this; } @@ -205,6 +208,7 @@ public function parent(): ?self public function setParent(?self $parent): self { $this->parent = $parent; + $this->uri = null; return $this; } @@ -212,6 +216,7 @@ public function setParent(?self $parent): self public function setRoute(?string $route): self { $this->route = $route; + $this->uri = null; return $this; } @@ -247,14 +252,20 @@ public function uri() return $cached; } + // With the shared cache off, a page still keeps its own URI, + // so siblings don't each rebuild their parent's. + if (! $this->tree->uriCacheEnabled() && ! is_null($this->uri)) { + return $this->uri; + } + if (! $this->structure() instanceof CollectionStructure) { - return $uris[$this->reference] = $this->entry()->uri(); + return $this->uri = $uris[$this->reference] = $this->entry()->uri(); } $parentUri = $this->parent && ! $this->parent->isRoot() ? $this->parent->uri() : ''; $parentUri = Str::replace(['.html', '.htm'], '', $parentUri); - return $uris[$this->reference] = app(UrlBuilder::class) + return $this->uri = $uris[$this->reference] = app(UrlBuilder::class) ->content($this) ->merge([ 'parent_uri' => $parentUri, @@ -299,6 +310,7 @@ public function isRoot() public function setTree($tree) { $this->tree = $tree; + $this->uri = null; return $this; } @@ -306,6 +318,7 @@ public function setTree($tree) public function setRoot(bool $isRoot) { $this->isRoot = $isRoot; + $this->uri = null; return $this; } diff --git a/tests/Data/Structures/PageTest.php b/tests/Data/Structures/PageTest.php index 63cd7d1b2ca..41df8b301f2 100644 --- a/tests/Data/Structures/PageTest.php +++ b/tests/Data/Structures/PageTest.php @@ -188,6 +188,44 @@ public function slug($slug = null) $this->assertFalse($page->hasCustomUrl()); } + #[Test] + public function it_rebuilds_the_uri_when_its_route_changes_while_the_uri_cache_is_disabled() + { + $entry = new class extends Entry + { + public function id($id = null) + { + return 'a'; + } + + public function slug($slug = null) + { + return 'entry-slug'; + } + }; + $collection = tap(\Statamic\Facades\Collection::make('test'))->save(); + $entry->collection('test'); + + $parent = Mockery::mock(Page::class); + $parent->shouldReceive('uri')->andReturn('/first/parent'); + $parent->shouldReceive('isRoot')->andReturnFalse(); + + $tree = $this->newTree()->disableUriCache()->setStructure( + $this->mock(CollectionStructure::class)->shouldReceive('collection')->andReturn($collection)->getMock() + ); + + $page = (new Page) + ->setTree($tree) + ->setRoute('{parent_uri}/{slug}') + ->setParent($parent) + ->setEntry($entry); + + $this->assertEquals('/first/parent/entry-slug', $page->uri()); + + $page->setRoute('/foo/{parent_uri}/{slug}'); + $this->assertEquals('/foo/first/parent/entry-slug', $page->uri()); + } + #[Test] #[DataProvider('stripExtensionFromParentUriProvider')] public function it_builds_a_uri_and_strips_out_file_extensions_from_parent_uri($ext) diff --git a/tests/Feature/Collections/UpdateCollectionTreeTest.php b/tests/Feature/Collections/UpdateCollectionTreeTest.php index 60d4417e386..79c96de8c5f 100644 --- a/tests/Feature/Collections/UpdateCollectionTreeTest.php +++ b/tests/Feature/Collections/UpdateCollectionTreeTest.php @@ -5,9 +5,12 @@ use Facades\Tests\Factories\EntryFactory; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; +use Statamic\Contracts\Routing\UrlBuilder as UrlBuilderContract; use Statamic\Facades\Collection; use Statamic\Facades\Entry; use Statamic\Facades\User; +use Statamic\Routing\UrlBuilder; +use Statamic\Structures\Page; use Tests\FakesRoles; use Tests\PreventSavingStacheItemsToDisk; use Tests\TestCase; @@ -126,6 +129,56 @@ private function duplicateUriTest($expectsRoot) ], $collection->structure()->in('en')->tree()); } + #[Test] + public function it_builds_each_siblings_uri_once_when_validating_moved_pages() + { + $user = tap(User::make()->makeSuper())->save(); + $collection = tap(Collection::make('test')->routes('{parent_uri}/{slug}'))->save(); + EntryFactory::id('e1')->collection($collection)->slug('alfa')->create(); + EntryFactory::id('e2')->collection($collection)->slug('bravo')->create(); + EntryFactory::id('e3')->collection($collection)->slug('charlie')->create(); + $collection->structureContents(['root' => false])->save(); + $collection->structure()->in('en')->tree([ + ['entry' => 'e1', 'children' => [ + ['entry' => 'e2'], + ['entry' => 'e3'], + ]], + ])->save(); + + // Saving leaves the tree's pages cached in memory, where a request would load it fresh. + $collection->structure()->in('en')->flushCache(); + + $built = collect(); + + $this->app->bind(UrlBuilderContract::class, fn () => new class($built) extends UrlBuilder + { + public function __construct(private $built) + { + } + + public function build($route) + { + if ($this->content instanceof Page) { + $this->built->push($this->content->reference()); + } + + return parent::build($route); + } + }); + + $this + ->actingAs($user) + ->update($collection, ['pages' => [ + ['id' => 'e1', 'children' => [ + ['id' => 'e3', 'children' => []], + ['id' => 'e2', 'children' => []], + ]], + ]]) + ->assertOk(); + + $this->assertEquals(['e1' => 1, 'e2' => 1, 'e3' => 1], $built->countBy()->sortKeys()->all()); + } + #[Test] public function it_deletes_entries_scheduled_for_deletion() {