From 2090119474c73e589ad6cfae97eca136532ebd8e Mon Sep 17 00:00:00 2001 From: Sylvester Damgaard Date: Thu, 27 Aug 2026 09:38:25 +0200 Subject: [PATCH] fix(entries): satisfy required slug/date rules from the entry's own values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #27 fix removed the slug from the validated payload before blueprint validation, so a blueprint whose slug field is required — Statamic's default — could never be satisfied: every update failed with "The Slug field is required" no matter what the caller sent (#39). Slug and date are entry properties, not data keys, so the merged payload never contains them on its own. Inject the entry's effective values into the validation payload and resolve the UniqueEntryValue({collection}, {id}, {site}) placeholders via withReplacements() on both FieldsValidator invocations (including the TypeError fallback), matching how the CP's EntriesController validates. With the entry's own id excluded, the re-validated slug cannot trip the uniqueness rule — the false positive that #27 was about — while a slug owned by another entry is still rejected. Also from the #39 report: - date no longer has to be resent on every update of a dated collection; the entry's current date satisfies a required rule when omitted - create's slug argument is now declared in the tool schema (it was read but impossible to send), and data.slug is accepted as an alias Fixes #39 --- CHANGELOG.md | 3 + src/Mcp/Tools/Routers/EntriesRouter.php | 74 ++++-- tests/Feature/Routers/EntriesRouterTest.php | 281 ++++++++++++++++++++ 3 files changed, 340 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6606451..2cb84b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **`SECURITY.md`** — Private vulnerability reporting via GitHub, an explicit split between what this addon secures and what the operator does, and a limitations section stating plainly that the audit log is append-only by convention with no hash chain (neither tamper-proof nor tamper-evident), that confirmation tokens are replayable within their window, and that `require_https` falls back to off when the published config predates the key ### Fixed +- **Entry updates no longer fail on blueprints with a required slug** (#39) — The #27 fix removed the slug from the validated payload entirely, so Statamic's default slug field (`validate: [required, UniqueEntryValue…]`) could never be satisfied: every update failed with "The Slug field is required", whether the caller omitted the slug or resent the current one. The entry's effective slug is now injected back into the validation payload, and both `FieldsValidator` invocations (including the `TypeError` fallback) resolve the `UniqueEntryValue({collection}, {id}, {site})` placeholders via `withReplacements()`, so the rule excludes the entry being updated — the false positive #27 was about — while a slug owned by another entry is still rejected +- **`date` no longer has to be resent on every update of a dated collection** — Like the slug, the date is an entry property absent from the merged data payload, so a blueprint with a required date field failed any update that did not repeat a date the caller never meant to change. The entry's current date now satisfies the rule when the payload omits it +- **Explicit slug on create actually works** — `createEntry()` read `$arguments['slug']`, but the tool schema never declared the parameter, so no client could send it and the slug was always derived from the title. The schema now declares `slug`, and create also accepts it as `data.slug` — the shape update uses — storing it as an entry property in both cases, never as a data key - **PHPStan level 9 is no longer partly disabled** — The config carried blanket `ignoreErrors` patterns (`#Method .* should return .* but returns mixed#`, `#Cannot call method .* on .*\|null#`, `#Parameter .* expects .*, mixed given#`, and four more) that suppressed whole error classes across `src/`, so "level 9 clean" meant considerably less than it sounded. Removing them surfaced 54 real errors — almost all method calls on `Blueprint|null`, `Entry|null`, or `GlobalSet|null` after a lookup, because `requireResource()` returned an error array without narrowing the variable. Every site now checks for null explicitly (identical messages, identical behaviour, and the analyser can see it), the untyped Statamic/Eloquent return values are narrowed rather than cast, and the four `@phpstan-ignore` annotations on `abort()` calls are gone. `requireResource()` itself is removed, having no callers left - **CI verifies formatting instead of rewriting it** — The Tests workflow ran Pint in fix mode, committed the result, and pushed it back to the branch; it now runs `pint --test` and fails on violations, matching what the release workflow already does - **CI exercises both Laravel majors** — `composer.json` claims Laravel 12 and 13 via `orchestra/testbench: ^10.0 || ^11.0`, but the matrix only varied PHP, so every job resolved testbench 11 and the Laravel 12 claim was never verified. The matrix now spans both. The suite passes on Laravel 12 diff --git a/src/Mcp/Tools/Routers/EntriesRouter.php b/src/Mcp/Tools/Routers/EntriesRouter.php index 07137e2..ffd6d78 100644 --- a/src/Mcp/Tools/Routers/EntriesRouter.php +++ b/src/Mcp/Tools/Routers/EntriesRouter.php @@ -75,6 +75,9 @@ protected function defineSchema(JsonSchemaContract $schema): array . 'blueprint handle to see required fields, types, and nesting before sending data.' ), + 'slug' => JsonSchema::string() + ->description('Entry slug for the create action. Defaults to a slug generated from the title. For update, pass "slug" inside data instead'), + 'filters' => JsonSchema::object() ->description('Filter conditions as key-value pairs. Keys are field handles from the blueprint. Example: {"status": "published"}'), @@ -350,9 +353,13 @@ private function createEntry(array $arguments): array ->collection($collection) ->locale($site); - // Set slug from arguments or generate from title - if (! empty($arguments['slug'])) { - $requestedSlug = $arguments['slug']; + // Set slug from arguments (or data, for callers that send it as a + // field value) or generate from title. Slug is an entry property, + // not a data key, so it never stays in $data. + $requestedSlug = $arguments['slug'] ?? $data['slug'] ?? null; + unset($data['slug']); + + if (! empty($requestedSlug)) { $requestedSlug = is_string($requestedSlug) ? $requestedSlug : ''; // Use Statamic's built-in validation for unique slugs @@ -421,6 +428,11 @@ private function createEntry(array $arguments): array try { $fields = $blueprint->fields()->addValues($dataWithSlug); + // Resolve blueprint-level rule placeholders — Statamic's + // default slug field carries + // `new UniqueEntryValue({collection}, {id}, {site})`, + // which needs {collection} and {site} filled in ({id} + // resolves to null: nothing to exclude on create). (new FieldsValidator) ->fields($fields) ->withContext([ @@ -428,6 +440,10 @@ private function createEntry(array $arguments): array 'collection' => $collection, 'site' => $site, ]) + ->withReplacements([ + 'collection' => $collection->handle(), + 'site' => $site, + ]) ->validate(); // Process through fieldtypes (Terms strips prefixes, @@ -530,14 +546,14 @@ 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. + // Handle slug separately *before* blueprint validation. Slug is + // an entry property, not a data key, so a requested change is + // applied to the entry object and removed from $data. This + // dedicated check passes the entry's id as the $except argument, + // so it gives a precise error without rejecting the entry's own + // slug as "already taken" (issue #27). The blueprint validation + // below re-validates the applied slug with resolved rule + // placeholders. if (array_key_exists('slug', $data)) { $newSlug = is_string($data['slug']) ? $data['slug'] : ''; $slugValidator = Validator::make(['slug' => $newSlug], [ @@ -592,26 +608,47 @@ 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 = $this->sanitizeStoredFieldDataForValidation($blueprint, $mergedData); + // Slug and date are entry properties, not data keys, so the + // merged payload never contains them on its own. Statamic's + // default blueprint declares slug as `required`, so validation + // must see the entry's effective values or every update fails + // with "The Slug field is required" (issue #39). Safe to + // re-validate the slug: withReplacements() below resolves + // `UniqueEntryValue({collection}, {id}, {site})` with this + // entry's id, excluding it from the uniqueness check — the + // exact false positive #27 was about. + $entryPropertyValues = []; + $entrySlug = $entry->slug(); + if (is_string($entrySlug) && $entrySlug !== '') { + $entryPropertyValues['slug'] = $entrySlug; + } + $entryDate = $entry->date(); + if (! array_key_exists('date', $data) && $entry->collection()->dated() && $entryDate !== null) { + $entryPropertyValues['date'] = $entryDate->format('Y-m-d\TH:i:s.v\Z'); + } + $mergedData = array_merge($mergedData, $entryPropertyValues); + $validationContext = [ 'entry' => $entry, 'collection' => $entry->collection(), 'site' => $site, ]; + $ruleReplacements = [ + 'id' => $entry->id(), + 'collection' => $entry->collectionHandle(), + 'site' => $site, + ]; + try { (new FieldsValidator) ->fields($blueprint->fields()->addValues($mergedData)) ->withContext($validationContext) + ->withReplacements($ruleReplacements) ->validate(); } catch (ValidationException $e) { return $this->formatValidationError($e); @@ -623,8 +660,9 @@ private function updateEntry(array $arguments): array // already valid when it was saved. try { (new FieldsValidator) - ->fields($blueprint->fields()->addValues($data)) + ->fields($blueprint->fields()->addValues(array_merge($data, $entryPropertyValues))) ->withContext($validationContext) + ->withReplacements($ruleReplacements) ->validate(); } catch (ValidationException $inner) { return $this->formatValidationError($inner); diff --git a/tests/Feature/Routers/EntriesRouterTest.php b/tests/Feature/Routers/EntriesRouterTest.php index 8884333..9fa232a 100644 --- a/tests/Feature/Routers/EntriesRouterTest.php +++ b/tests/Feature/Routers/EntriesRouterTest.php @@ -6,7 +6,9 @@ use Cboxdk\StatamicMcp\Mcp\Tools\Routers\EntriesRouter; use Cboxdk\StatamicMcp\Tests\TestCase; +use Illuminate\Support\Carbon; use Illuminate\Support\Facades\Storage; +use Statamic\Facades\Blueprint; use Statamic\Facades\Collection; use Statamic\Facades\Entry; use Statamic\Facades\Stache; @@ -321,6 +323,285 @@ public function test_update_entry_to_existing_slug_returns_error(): void ); } + /** + * Give the test collection the blueprint Statamic scaffolds by default: + * a slug field that is `required` and unique via placeholder-based + * UniqueEntryValue. Issue #39 only reproduces against this blueprint. + */ + private function createBlueprintWithRequiredSlug(): void + { + Blueprint::make($this->collectionHandle) + ->setNamespace("collections.{$this->collectionHandle}") + ->setContents([ + 'tabs' => [ + 'main' => [ + 'sections' => [ + [ + 'fields' => [ + ['handle' => 'title', 'field' => ['type' => 'text', 'validate' => ['required']]], + ['handle' => 'slug', 'field' => [ + 'type' => 'slug', + 'localizable' => true, + 'validate' => [ + 'required', + 'new \Statamic\Rules\UniqueEntryValue({collection}, {id}, {site})', + ], + ]], + ], + ], + ], + ], + ], + ]) + ->save(); + } + + /** + * Regression for https://github.com/cboxdk/statamic-mcp/issues/39. + * + * The #27 fix removed the slug from the validated payload entirely, + * so a blueprint whose slug field is `required` (Statamic's default) + * could never be satisfied — every update failed with + * "The Slug field is required" no matter what the caller sent. + */ + public function test_update_with_required_slug_blueprint_succeeds_without_slug(): void + { + $this->createBlueprintWithRequiredSlug(); + + $entry = Entry::make() + ->collection($this->collectionHandle) + ->slug("required-slug-omitted-{$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'], + 'update must satisfy a required slug rule from the entry\'s current slug; got: ' + . json_encode($result['errors'] ?? []), + ); + + $reloaded = Entry::find($entry->id()); + $this->assertSame('Updated Title', $reloaded->get('title')); + $this->assertSame("required-slug-omitted-{$this->testId}", $reloaded->slug()); + } + + /** + * Companion regression for issue #39: resending the current slug must + * not re-trigger the #27 false positive. The blueprint-level + * UniqueEntryValue({collection}, {id}, {site}) placeholders must be + * resolved so the rule excludes the entry being updated. + */ + public function test_update_with_required_slug_blueprint_succeeds_with_unchanged_slug(): void + { + $this->createBlueprintWithRequiredSlug(); + + $slug = "required-slug-resent-{$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 current slug must pass the blueprint uniqueness rule; got: ' + . json_encode($result['errors'] ?? []), + ); + + $reloaded = Entry::find($entry->id()); + $this->assertSame($slug, $reloaded->slug()); + $this->assertSame('Updated', $reloaded->get('title')); + } + + /** + * Resolving the uniqueness placeholders must not weaken the rule: + * a slug owned by another entry is still rejected. + */ + public function test_update_with_required_slug_blueprint_rejects_colliding_slug(): void + { + $this->createBlueprintWithRequiredSlug(); + + $other = Entry::make() + ->collection($this->collectionHandle) + ->slug("required-slug-owner-{$this->testId}") + ->data(['title' => 'Other']); + $other->save(); + + $entry = Entry::make() + ->collection($this->collectionHandle) + ->slug("required-slug-victim-{$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'], + 'a slug owned by another entry must still be rejected', + ); + } + + /** + * Issue #39, date facet: on dated collections the date is an entry + * property, so it is absent from the merged data payload. A blueprint + * with a required date field failed every update that did not resend + * the date the caller never meant to change. + */ + public function test_update_dated_collection_without_date_succeeds(): void + { + $datedHandle = "dated-{$this->testId}"; + Collection::make($datedHandle) + ->title('Dated Posts') + ->dated(true) + ->save(); + + Blueprint::make($datedHandle) + ->setNamespace("collections.{$datedHandle}") + ->setContents([ + 'tabs' => [ + 'main' => [ + 'sections' => [ + [ + 'fields' => [ + ['handle' => 'title', 'field' => ['type' => 'text', 'validate' => ['required']]], + ['handle' => 'date', 'field' => ['type' => 'date', 'validate' => ['required']]], + ], + ], + ], + ], + ], + ]) + ->save(); + + $entry = Entry::make() + ->collection($datedHandle) + ->slug("dated-entry-{$this->testId}") + ->date(Carbon::parse('2025-01-15')) + ->data(['title' => 'Dated Original']); + $entry->save(); + + $result = $this->router->execute([ + 'action' => 'update', + 'collection' => $datedHandle, + 'id' => $entry->id(), + 'data' => [ + 'title' => 'Dated Updated', + ], + ]); + + $this->assertTrue( + $result['success'], + 'update must satisfy a required date rule from the entry\'s current date; got: ' + . json_encode($result['errors'] ?? []), + ); + + $reloaded = Entry::find($entry->id()); + $this->assertSame('Dated Updated', $reloaded->get('title')); + $this->assertSame('2025-01-15', $reloaded->date()->format('Y-m-d')); + } + + /** + * Issue #39, create facet: the slug argument was read by createEntry() + * but never declared in the tool schema, so an explicit slug on create + * was impossible. + */ + public function test_create_entry_with_explicit_slug_argument(): void + { + $result = $this->router->execute([ + 'action' => 'create', + 'collection' => $this->collectionHandle, + 'slug' => "explicit-slug-{$this->testId}", + 'data' => [ + 'title' => 'Explicit Slug Test', + ], + ]); + + $this->assertTrue( + $result['success'], + 'create with an explicit slug must succeed; got: ' + . json_encode($result['errors'] ?? []), + ); + $this->assertSame("explicit-slug-{$this->testId}", $result['data']['entry']['slug']); + } + + /** + * Callers that send the slug as a data field (the shape update uses) + * get the same result on create — and the slug is stored as an entry + * property, never as a data key. + */ + public function test_create_entry_with_slug_in_data(): void + { + $result = $this->router->execute([ + 'action' => 'create', + 'collection' => $this->collectionHandle, + 'data' => [ + 'title' => 'Data Slug Test', + 'slug' => "data-slug-{$this->testId}", + ], + ]); + + $this->assertTrue( + $result['success'], + 'create with slug inside data must succeed; got: ' + . json_encode($result['errors'] ?? []), + ); + $this->assertSame("data-slug-{$this->testId}", $result['data']['entry']['slug']); + + $reloaded = Entry::find($result['data']['entry']['id']); + $this->assertArrayNotHasKey('slug', $reloaded->data()->all()); + } + + /** + * Creating against the default required-slug blueprint exercises the + * blueprint-level UniqueEntryValue placeholders on the create path. + */ + public function test_create_with_required_slug_blueprint_succeeds(): void + { + $this->createBlueprintWithRequiredSlug(); + + $result = $this->router->execute([ + 'action' => 'create', + 'collection' => $this->collectionHandle, + 'slug' => "required-create-{$this->testId}", + 'data' => [ + 'title' => 'Required Blueprint Create', + ], + ]); + + $this->assertTrue( + $result['success'], + 'create must pass the default required+unique slug rules; got: ' + . json_encode($result['errors'] ?? []), + ); + $this->assertSame("required-create-{$this->testId}", $result['data']['entry']['slug']); + } + public function test_update_nonexistent_entry_returns_error(): void { $result = $this->router->execute([