From e70af0bec172ed915d5657e1dc391460683c8392 Mon Sep 17 00:00:00 2001 From: SAY-5 Date: Tue, 28 Apr 2026 04:01:56 -0700 Subject: [PATCH] fix(entries): exclude current entry from slug uniqueness on update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #27. `updateEntry()` always injected the entry's *current* slug into the FieldsValidator payload: $mergedData = array_merge($entry->data()->all(), $data); $mergedData['slug'] = $entry->slug(); // <-- always added `UniqueEntryValue` then ran against that slug without an exclusion, so Statamic found the slug on the entry being updated and rejected it as 'already taken'. Every update through the MCP tool failed, regardless of whether the caller passed a slug at all. Mirror the slug handling already used in `createEntry()`, with one small addition: pass `$entry->id()` as the `$except` argument so `UniqueEntryValue` excludes the current entry from the uniqueness check. Slug rename, idempotent slug, and slug-not-supplied paths are now all handled before blueprint validation. The line that re-injected the slug into `mergedData` is removed so `FieldsValidator` never sees the slug column. Tests: * `test_update_entry_without_slug_in_data_succeeds` — exact reproduction from #27: update without slug now succeeds. * `test_update_entry_with_unchanged_slug_succeeds` — resending the current slug is idempotent. * `test_update_entry_with_new_unique_slug_succeeds` — renaming to a unique slug works. * `test_update_entry_to_existing_slug_returns_error` — colliding with another entry's slug still surfaces the validation error. --- src/Mcp/Tools/Routers/EntriesRouter.php | 37 +++++- tests/Feature/Routers/EntriesRouterTest.php | 137 ++++++++++++++++++++ 2 files changed, 173 insertions(+), 1 deletion(-) diff --git a/src/Mcp/Tools/Routers/EntriesRouter.php b/src/Mcp/Tools/Routers/EntriesRouter.php index d92d251..710c293 100644 --- a/src/Mcp/Tools/Routers/EntriesRouter.php +++ b/src/Mcp/Tools/Routers/EntriesRouter.php @@ -455,6 +455,36 @@ private function updateEntry(array $arguments): array unset($data['published']); } + // Handle slug separately *before* blueprint validation so the + // FieldsValidator never sees the slug column. Letting the merged + // payload include the slug forces UniqueEntryValue to compare the + // current slug against the entry being updated and reject it as + // "already taken" — even when the caller never asked to change + // the slug. Mirrors the createEntry() flow but passes the + // entry's id as the $except argument so the rule excludes the + // current entry from the uniqueness check. + if (array_key_exists('slug', $data)) { + $newSlug = is_string($data['slug']) ? $data['slug'] : ''; + $slugValidator = Validator::make(['slug' => $newSlug], [ + 'slug' => [ + 'required', + 'string', + new UniqueEntryValue($entry->collectionHandle(), $entry->id(), $site), + ], + ]); + + if ($slugValidator->fails()) { + $errors = $slugValidator->errors()->get('slug'); + /** @var array $flatErrors */ + $flatErrors = array_map(fn ($error) => is_string($error) ? $error : implode(', ', (array) $error), $errors); + + return $this->createErrorResponse('Slug validation failed: ' . implode(', ', $flatErrors))->toArray(); + } + + $entry->slug($newSlug); + unset($data['slug']); + } + // For dated collections, parse the date and set it on the entry. // Keep a normalized copy in data so the FieldsValidator sees the required field. if (array_key_exists('date', $data) && $entry->collection()->dated()) { @@ -487,9 +517,14 @@ private function updateEntry(array $arguments): array // TypeError (common with third-party fieldtypes like SEO Pro // whose preProcessValidatable can't handle stored data formats), // we fall back to validating only the incoming fields. + // NOTE: do NOT inject slug into mergedData. Any slug change + // was already applied to the entry object above; the + // FieldsValidator does not need the slug column and including + // it forces UniqueEntryValue to reject the current entry's + // own slug. See the slug-handling block earlier in this + // method (issue #27). /** @var array $mergedData */ $mergedData = array_merge($entry->data()->all(), $data); - $mergedData['slug'] = $entry->slug(); $mergedData = $this->sanitizeStoredFieldDataForValidation($blueprint, $mergedData); $validationContext = [ diff --git a/tests/Feature/Routers/EntriesRouterTest.php b/tests/Feature/Routers/EntriesRouterTest.php index 64de092..8884333 100644 --- a/tests/Feature/Routers/EntriesRouterTest.php +++ b/tests/Feature/Routers/EntriesRouterTest.php @@ -184,6 +184,143 @@ public function test_create_entry_without_slug_generates_from_title(): void $this->assertEquals('auto-slug-generation-test', $result['data']['entry']['slug']); } + /** + * Regression for https://github.com/cboxdk/statamic-mcp/issues/27. + * + * The previous updateEntry() flow injected the entry's current + * slug into the FieldsValidator payload, which made + * UniqueEntryValue compare the slug against the entry being + * updated and reject it as "already taken" — even when the caller + * never passed a slug at all. + */ + public function test_update_entry_without_slug_in_data_succeeds(): void + { + $entry = Entry::make() + ->collection($this->collectionHandle) + ->slug("update-no-slug-{$this->testId}") + ->data(['title' => 'Original Title']); + $entry->save(); + + $result = $this->router->execute([ + 'action' => 'update', + 'collection' => $this->collectionHandle, + 'id' => $entry->id(), + 'data' => [ + 'title' => 'Updated Title', + ], + ]); + + $this->assertTrue( + $result['success'], + 'updating an entry without changing the slug must succeed; got: ' + . json_encode($result['errors'] ?? []), + ); + + $reloaded = Entry::find($entry->id()); + $this->assertSame('Updated Title', $reloaded->get('title')); + $this->assertSame("update-no-slug-{$this->testId}", $reloaded->slug()); + } + + /** + * Resending the entry's *current* slug as part of `data` is + * idempotent and must not trigger UniqueEntryValue against the + * entry itself. + */ + public function test_update_entry_with_unchanged_slug_succeeds(): void + { + $slug = "update-same-slug-{$this->testId}"; + $entry = Entry::make() + ->collection($this->collectionHandle) + ->slug($slug) + ->data(['title' => 'Original']); + $entry->save(); + + $result = $this->router->execute([ + 'action' => 'update', + 'collection' => $this->collectionHandle, + 'id' => $entry->id(), + 'data' => [ + 'title' => 'Updated', + 'slug' => $slug, + ], + ]); + + $this->assertTrue( + $result['success'], + 'resending the existing slug must be idempotent; got: ' + . json_encode($result['errors'] ?? []), + ); + + $reloaded = Entry::find($entry->id()); + $this->assertSame($slug, $reloaded->slug()); + $this->assertSame('Updated', $reloaded->get('title')); + } + + /** + * Renaming an entry to a slug that does not collide with any + * other entry in the collection succeeds. + */ + public function test_update_entry_with_new_unique_slug_succeeds(): void + { + $entry = Entry::make() + ->collection($this->collectionHandle) + ->slug("update-rename-from-{$this->testId}") + ->data(['title' => 'Will Be Renamed']); + $entry->save(); + + $newSlug = "update-rename-to-{$this->testId}"; + $result = $this->router->execute([ + 'action' => 'update', + 'collection' => $this->collectionHandle, + 'id' => $entry->id(), + 'data' => [ + 'slug' => $newSlug, + ], + ]); + + $this->assertTrue( + $result['success'], + 'updating to a unique new slug must succeed; got: ' + . json_encode($result['errors'] ?? []), + ); + + $reloaded = Entry::find($entry->id()); + $this->assertSame($newSlug, $reloaded->slug()); + } + + /** + * Renaming an entry to a slug that another entry already owns + * must surface the validation error. + */ + public function test_update_entry_to_existing_slug_returns_error(): void + { + $other = Entry::make() + ->collection($this->collectionHandle) + ->slug("update-collision-{$this->testId}") + ->data(['title' => 'Other']); + $other->save(); + + $entry = Entry::make() + ->collection($this->collectionHandle) + ->slug("update-target-{$this->testId}") + ->data(['title' => 'Target']); + $entry->save(); + + $result = $this->router->execute([ + 'action' => 'update', + 'collection' => $this->collectionHandle, + 'id' => $entry->id(), + 'data' => [ + 'slug' => $other->slug(), + ], + ]); + + $this->assertFalse( + $result['success'], + 'updating to a slug already owned by another entry must fail', + ); + } + public function test_update_nonexistent_entry_returns_error(): void { $result = $this->router->execute([