From 0ba675f98b5caf5f93f110d2e8e96c9f5b683ec6 Mon Sep 17 00:00:00 2001 From: Sajeeb Ahamed Date: Fri, 13 Nov 2026 17:05:23 +0600 Subject: [PATCH] Speed up validation of large wildcard payloads Validating a product with 180 variants (5,956 keys after wildcard expansion) took ~950 ms in the CLI and ~2.6 s with Xdebug on. It now takes ~58 ms and ~88 ms. Three causes, all fixed: - RuleParser parsed every expanded wildcard key separately. Each rule entry is now parsed once; later keys get shallow clones, so each key still owns its instances (parse: 577 ms -> 17 ms). - Validator re-ran every rule: validate() ran passes() twice, and a repeat passes()/fails() duplicated errors. The outcome is now stored and reused; set_value() and sometimes() (when it adds rules) reset it (validated() after passes(): 198 ms -> 27 ms). - Every rule check re-resolved default messages for all of the rule's constraints, each lookup rebuilding the MessagesBag table. MessagesBag::get() now reads the table built once, and sync_custom_messages() skips the work when no custom messages are in play (passes: 177 ms -> 14 ms). Also fixes MessagesBag merging config/messages.php shallowly: overriding one validator message wiped every other default in the group. It now merges recursively. Adds tests for wildcard parsing, run lifecycle, custom messages and the messages bag; a public GET /validation-stress route in example/ to exercise the validator; and the OpenSpec specs for validation-rule-parsing, validation-run-lifecycle and validation-messages. Timings: CLI, PHP 8.4, Xdebug off unless stated, mean of 2 runs, main measured at 15f25bd. Co-Authored-By: Claude Opus 5.5 --- .../app/Services/ValidationStressService.php | 205 ++++++++++++++++++ example/routes/api.php | 19 +- .../.openspec.yaml | 2 + .../design.md | 105 +++++++++ .../proposal.md | 63 ++++++ .../specs/validation-messages/spec.md | 60 +++++ .../tasks.md | 33 +++ .../.openspec.yaml | 2 + .../bench/ValidatorBenchTest.php | 46 ++++ .../2026-11-13-validator-parse-once/design.md | 121 +++++++++++ .../proposal.md | 68 ++++++ .../specs/validation-rule-parsing/spec.md | 58 +++++ .../specs/validation-run-lifecycle/spec.md | 41 ++++ .../2026-11-13-validator-parse-once/tasks.md | 44 ++++ openspec/specs/validation-messages/spec.md | 60 +++++ .../specs/validation-rule-parsing/spec.md | 58 +++++ .../specs/validation-run-lifecycle/spec.md | 41 ++++ src/Supports/MessagesBag.php | 10 +- src/Validation/RuleParser.php | 31 ++- src/Validation/ValidationRule.php | 8 + src/Validation/Validator.php | 56 ++++- .../Validation/CountingMessagesBag.php | 17 ++ tests/Unit/Supports/MessagesBagTest.php | 64 ++++++ tests/Unit/Validation/CustomMessagesTest.php | 119 ++++++++++ tests/Unit/Validation/RuleParsingTest.php | 138 ++++++++++++ .../Validation/ValidatorRunLifecycleTest.php | 88 ++++++++ 26 files changed, 1544 insertions(+), 13 deletions(-) create mode 100644 example/app/Services/ValidationStressService.php create mode 100644 openspec/changes/archive/2026-11-13-validator-message-lookups/.openspec.yaml create mode 100644 openspec/changes/archive/2026-11-13-validator-message-lookups/design.md create mode 100644 openspec/changes/archive/2026-11-13-validator-message-lookups/proposal.md create mode 100644 openspec/changes/archive/2026-11-13-validator-message-lookups/specs/validation-messages/spec.md create mode 100644 openspec/changes/archive/2026-11-13-validator-message-lookups/tasks.md create mode 100644 openspec/changes/archive/2026-11-13-validator-parse-once/.openspec.yaml create mode 100644 openspec/changes/archive/2026-11-13-validator-parse-once/bench/ValidatorBenchTest.php create mode 100644 openspec/changes/archive/2026-11-13-validator-parse-once/design.md create mode 100644 openspec/changes/archive/2026-11-13-validator-parse-once/proposal.md create mode 100644 openspec/changes/archive/2026-11-13-validator-parse-once/specs/validation-rule-parsing/spec.md create mode 100644 openspec/changes/archive/2026-11-13-validator-parse-once/specs/validation-run-lifecycle/spec.md create mode 100644 openspec/changes/archive/2026-11-13-validator-parse-once/tasks.md create mode 100644 openspec/specs/validation-messages/spec.md create mode 100644 openspec/specs/validation-rule-parsing/spec.md create mode 100644 openspec/specs/validation-run-lifecycle/spec.md create mode 100644 tests/Support/Validation/CountingMessagesBag.php create mode 100644 tests/Unit/Supports/MessagesBagTest.php create mode 100644 tests/Unit/Validation/CustomMessagesTest.php create mode 100644 tests/Unit/Validation/RuleParsingTest.php create mode 100644 tests/Unit/Validation/ValidatorRunLifecycleTest.php diff --git a/example/app/Services/ValidationStressService.php b/example/app/Services/ValidationStressService.php new file mode 100644 index 0000000..86c347b --- /dev/null +++ b/example/app/Services/ValidationStressService.php @@ -0,0 +1,205 @@ + 'required|array', + 'product.title' => 'required|string|min:3|max:120', + 'product.slug' => 'required|string|alpha_dash|max:120', + 'product.status' => 'required|in:draft,published,archived', + 'product.type' => 'required|in:simple,variable', + 'product.description' => 'nullable|string|max:5000', + 'product.vendor_email' => 'required|string|email', + 'product.website' => 'nullable|string|url', + 'product.published_at' => 'nullable|date', + 'product.featured' => 'required|boolean', + 'product.tax_rate' => 'required|numeric|min:0|max:100', + 'product.tags' => 'nullable|array|max:20', + 'product.tags.*' => 'required|string|max:30', + + // Variants. + 'variants' => 'required|array|min:1', + 'variants.*.sku' => 'required|string|alpha_dash|max:50', + 'variants.*.name' => 'required|string|min:2|max:100', + 'variants.*.price' => 'required|numeric|min:0', + 'variants.*.sale_price' => 'nullable|numeric|min:0', + 'variants.*.cost' => 'nullable|numeric|min:0', + 'variants.*.stock' => 'required|integer|min:0', + 'variants.*.low_stock_threshold' => 'nullable|integer|min:0', + 'variants.*.weight' => 'nullable|numeric|min:0', + 'variants.*.length' => 'nullable|numeric|min:0', + 'variants.*.width' => 'nullable|numeric|min:0', + 'variants.*.height' => 'nullable|numeric|min:0', + 'variants.*.color' => 'required|string|max:30', + 'variants.*.size' => 'required|in:xs,s,m,l,xl,xxl', + 'variants.*.material' => 'nullable|string|max:50', + 'variants.*.barcode' => 'nullable|string|alpha_num|max:20', + 'variants.*.is_active' => 'required|boolean', + 'variants.*.is_default' => 'required|boolean', + 'variants.*.backorder' => 'required|in:no,notify,yes', + 'variants.*.position' => 'required|integer|min:0', + 'variants.*.image_url' => 'nullable|string|url', + 'variants.*.available_from' => 'nullable|date', + 'variants.*.shipping_class' => 'nullable|string|max:50', + 'variants.*.tax_class' => 'nullable|string|max:50', + 'variants.*.note' => 'nullable|string|max:255', + 'variants.*.digital' => [ + Rule::when( + fn ($data) => ($data['product']['type'] ?? null) === 'variable', + 'required|boolean', + 'nullable|boolean' + ), + ], + 'variants.*.discount_percent' => [ + 'nullable', + function ($value, $key) { + return $value === null || ($value >= 0 && $value <= 90) + ? true + : "The {$key} must be between 0 and 90."; + }, + ], + + // Nested wildcard: every variant carries a list of attributes. + 'variants.*.attributes' => 'required|array|min:1', + 'variants.*.attributes.*.name' => 'required|string|max:50', + 'variants.*.attributes.*.value' => 'required|string|max:100', + ]; + } + + public function payload(int $variants, bool $invalid = false): array + { + $sizes = ['xs', 's', 'm', 'l', 'xl', 'xxl']; + $items = []; + + for ($i = 0; $i < $variants; $i++) { + $attributes = []; + + for ($a = 0; $a < static::ATTRIBUTES_PER_VARIANT; $a++) { + $attributes[] = ['name' => "attribute-{$a}", 'value' => "value-{$i}-{$a}"]; + } + + $items[] = [ + 'sku' => "SKU-{$i}", + 'name' => "Variant {$i}", + 'price' => 19.99 + $i, + 'sale_price' => 17.99 + $i, + 'cost' => 9.5, + 'stock' => 100 + $i, + 'low_stock_threshold' => 5, + 'weight' => 0.4, + 'length' => 30, + 'width' => 20, + 'height' => 2, + 'color' => 'black', + 'size' => $sizes[$i % count($sizes)], + 'material' => 'cotton', + 'barcode' => "BC{$i}", + 'is_active' => true, + 'is_default' => $i === 0, + 'backorder' => 'notify', + 'position' => $i, + 'image_url' => "https://example.test/images/{$i}.jpg", + 'available_from' => '2026-11-13', + 'shipping_class' => 'standard', + 'tax_class' => 'reduced', + 'note' => null, + 'digital' => false, + 'discount_percent' => 10, + 'attributes' => $attributes, + ]; + } + + // One broken variant in every ten, each breaking a different kind of rule. + if ($invalid) { + foreach ($items as $i => &$item) { + if ($i % 10 !== 0) { + continue; + } + + $item['sku'] = 'bad sku!'; + $item['price'] = -5; + $item['stock'] = 'many'; + $item['size'] = 'huge'; + $item['image_url'] = 'not-a-url'; + $item['discount_percent'] = 95; + $item['attributes'][0]['value'] = ''; + } + unset($item); + } + + return [ + 'product' => [ + 'title' => 'Stress Test Tee', + 'slug' => 'stress-test-tee', + 'status' => 'published', + 'type' => 'variable', + 'description' => 'A product with a very large number of variants.', + 'vendor_email' => 'vendor@example.test', + 'website' => 'https://example.test', + 'published_at' => '2026-11-13', + 'featured' => true, + 'tax_rate' => 7.5, + 'tags' => ['apparel', 'cotton', 'stress-test'], + ], + 'variants' => $items, + ]; + } + + /** + * Validate the payload and report timings, key counts and errors. + */ + public function run(int $variants, bool $invalid = false): array + { + $rules = $this->rules(); + $data = $this->payload($variants, $invalid); + + $started = microtime(true); + $validator = Validator::make($data, $rules); + $parsed = microtime(true); + $passes = $validator->passes(); + $checked = microtime(true); + $validated = $passes ? $validator->validated() : null; + $finished = microtime(true); + + $errors = $validator->errors(); + $errors_by_field = []; + + foreach (array_keys($errors) as $key) { + $field = preg_replace('/\.\d+(?=\.|$)/', '.*', $key); + $errors_by_field[$field] = ($errors_by_field[$field] ?? 0) + 1; + } + + return [ + 'variants' => $variants, + 'invalid' => $invalid, + 'rule_entries' => count($rules), + 'expanded_keys' => count($validator->get_rules()), + 'passes' => $passes, + 'error_keys' => count($errors), + 'errors_by_field' => $errors_by_field, + 'sample_errors' => array_slice($errors, 0, 10, true), + 'validated_variants' => $validated === null ? null : count($validated['variants'] ?? []), + 'timings_ms' => [ + 'parse' => round(($parsed - $started) * 1000, 2), + 'passes' => round(($checked - $parsed) * 1000, 2), + 'validated' => round(($finished - $checked) * 1000, 2), + 'total' => round(($finished - $started) * 1000, 2), + ], + 'peak_memory_mb' => round(memory_get_peak_usage(true) / 1048576, 1), + ]; + } +} diff --git a/example/routes/api.php b/example/routes/api.php index 1f1e36d..0a2e7e4 100644 --- a/example/routes/api.php +++ b/example/routes/api.php @@ -10,6 +10,7 @@ use Example\App\Models\Blog; use Example\App\Models\Event; use Example\App\Resources\TestResource; +use Example\App\Services\ValidationStressService; use Framework\Http\Request; use Framework\Middlewares\AdminMiddleware; use Framework\Middlewares\AuthMiddleware; @@ -93,4 +94,20 @@ })->throttle(5, 1); -Route::get('/new-route', [TestController::class, 'get']); \ No newline at end of file +Route::get('/new-route', [TestController::class, 'get']); + +/* + * Validator stress test. Public, no middleware. + * + * Validates a product with many variants against a wide rule set that uses + * wildcard and nested wildcard keys, and reports timings and errors. + * + * GET /wp-json/framework/v1/validation-stress?variants=180 + * GET /wp-json/framework/v1/validation-stress?variants=500&invalid=1 + */ +Route::get('/validation-stress', function (Request $request, ValidationStressService $stress) { + $variants = max(1, min(2000, (int) $request->get('variants', 180, 'int'))); + $invalid = (bool) $request->get('invalid', false, 'bool'); + + return response()->json($stress->run($variants, $invalid)); +}); diff --git a/openspec/changes/archive/2026-11-13-validator-message-lookups/.openspec.yaml b/openspec/changes/archive/2026-11-13-validator-message-lookups/.openspec.yaml new file mode 100644 index 0000000..22ca07d --- /dev/null +++ b/openspec/changes/archive/2026-11-13-validator-message-lookups/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-11-13 diff --git a/openspec/changes/archive/2026-11-13-validator-message-lookups/design.md b/openspec/changes/archive/2026-11-13-validator-message-lookups/design.md new file mode 100644 index 0000000..31b42fa --- /dev/null +++ b/openspec/changes/archive/2026-11-13-validator-message-lookups/design.md @@ -0,0 +1,105 @@ +## Context + +See proposal.md (Why) for the measurements. + +How messages flow today: + +- `ValidationRule::__construct()` calls `process_default_messages($this->constraints)`. For + `default` and each supported constraint, it takes the custom message if there is one + (`get_custom_message()`, which reads `$this->custom_messages` at `"$name.$key"`), otherwise + `message('validator..')`. The result goes into `$this->default_messages`, which is + a plain `protected array`. About 21 call sites in `Rules/` read `$this->default_messages[$key]` + directly, and a consumer's own rule subclass may do the same. +- `Validator::apply()` → `sync_custom_messages($this->custom_messages)` → + `sync_default_messages_with_custom_messages()` → `process_default_messages()` again, for + every rule on every key. This has to stay per key **when there are custom messages**, because + `get_custom_message()` uses `$this->name`, and a closure message also gets `$this->value` and + `$this->data`. +- `message()` → `app()->make(MessagesBag::class)->get()` → `Arr::get($this->messages(), …)`. + `messages()` rebuilds `defaults()` (a large array literal, not cached while opcache is off) and + merges in `config('messages')`. `config()` already caches each file in a function-static + `$cache` for the request. + +## Goals / Non-Goals + +**Goals:** +- No message-table lookups while checking keys when the validator has no custom messages. +- Cheaper lookups everywhere else. +- Byte-for-byte the same messages as today in every case (spec: validation-messages). + +**Non-Goals:** +- Lazy default messages. They would need `$default_messages` to become an accessor or + `ArrayAccess` object, which breaks subclasses that read the array. Not needed for the + numbers above. +- Per-field indexing of custom messages to skip unrelated fields. +- Changes to `Validator`, `RuleParser`, `RuleFactory` or the rule classes. + +## Decisions + +### D1. `MessagesBag::get()` reads `$this->messages` + +Replace `Arr::get($this->messages(), $key, '')` with `Arr::get($this->messages, $key, '')`. The +constructor already fills `$this->messages`, and the bag is a container singleton. Its only +dynamic input, `config('messages')`, is cached per request by `config()`, so a lookup cannot +return a different message than it does today within one request. In tests, `TestCase` resets the +container, so each test gets a fresh bag. + +*Alternative:* keep `messages()` but memoise it. Rejected: that duplicates what the constructor +already does. + +### D2. `sync_custom_messages()` returns early when no custom messages are involved + +```php +if ($messages === [] && $this->custom_messages === []) { + return $this; +} +``` + +Why this is exact: +- With `$this->custom_messages === []`, every `get_custom_message()` returns `null` whatever + `$this->name` or `$this->value` is, so `process_default_messages()` would produce only the + `message()` defaults. +- Those defaults are what `$default_messages` already holds. The constructor computed them with + empty custom messages, and clones copy them (`validator-parse-once`). +- The `$this->custom_messages === []` half covers a rule object that carried custom messages from + an earlier validator. It recomputes once and drops back to defaults (spec: shared rule object). + +The guard goes in `sync_custom_messages()`, not `sync_default_messages_with_custom_messages()`, +so a subclass that overrides the latter still behaves the same whenever custom messages exist. + +*Alternatives:* +- **Lazy `$default_messages`.** Rejected (see Non-Goals). +- **Per-class static cache of defaults.** Rejected: it would survive container resets in tests + and long-running processes, and D2 already removes the per-key lookups. + +### D3. Counting test double for "no lookups" + +`tests/Support/Validation/CountingMessagesBag.php` extends `MessagesBag`, counts `get()` calls, and +is bound as the `MessagesBag` singleton inside the test. The "no lookups" scenario asserts the +count stays 0 during `passes()` on a passing payload. Custom-message tests are written first and +must pass on today's code. They lock in current behaviour, because none exist yet. + +## Risks / Trade-offs + +- [A consumer rule subclass overrides `get_default_message()` or `process_default_messages()` to + depend on `$this->name`, `$this->value` or `$this->data`] → With no custom messages, its + defaults would no longer be recomputed per key. Mitigation: document this in the + `sync_custom_messages()` docblock. Such a rule can override `sync_custom_messages()`. No rule + in `src/` does this. +- [Code changes `config('messages')` partway through a request] → Not possible today because + `config()` caches. Unchanged by D1. + +## Migration Plan + +No signature changes; ships with the next release. Rollback is a revert of two lines and a guard. + +## Benchmark + +`XDEBUG_MODE=off composer test:unit -- openspec/changes/validator-parse-once/bench/ValidatorBenchTest.php` +plus the stress payload from `example/app/Services/ValidationStressService.php` (180 variants). + +| `passes()` (stress payload) | Before | After | +|---|---|---| +| CLI, Xdebug off | 187 ms | 12 ms | +| CLI, Xdebug on | 460 ms | 28 ms | +| Web, playground | 1,238 ms | _pending: re-run `GET /validation-stress?variants=180`_ | diff --git a/openspec/changes/archive/2026-11-13-validator-message-lookups/proposal.md b/openspec/changes/archive/2026-11-13-validator-message-lookups/proposal.md new file mode 100644 index 0000000..16158bb --- /dev/null +++ b/openspec/changes/archive/2026-11-13-validator-message-lookups/proposal.md @@ -0,0 +1,63 @@ +## Why + +After `validator-parse-once`, parsing a 5,956-key payload takes about 10 ms, but `passes()` still takes +180 ms in the CLI, 484 ms with Xdebug on, and 1,238 ms in the playground web request (from +`GET /validation-stress?variants=180`). Almost all of that time goes to message lookups for messages +that are never shown: + +- Before every rule check, `Validator::apply()` calls `sync_custom_messages()`, which looks up a + default message for **every constraint the rule class supports**: 18 for `StringRule`, 10 for + `NumericRule`. This happens even when the value passes. For the stress payload that is + **74,357 `message()` calls**. +- Every `message()` call makes `MessagesBag::get()` rebuild its whole message table (the + `defaults()` literal merged with `config('messages')`). The constructor already built and stored + that table in `$this->messages`, but `get()` never reads it. + +Prototype measured on the stress payload (180 variants, 5,956 keys; the full unit suite passes): + +| `passes()` | Today | Prototype | +|---|---|---| +| CLI, Xdebug off | 180 ms | 12 ms | +| CLI, Xdebug on | 484 ms | 29 ms | + +## What Changes + +- `MessagesBag::get()` reads the table its constructor stored instead of rebuilding it on every + call. `config()` already caches each config file for the request, so the messages returned + stay the same. +- `ValidationRule::sync_custom_messages()` skips recomputing the default messages when the + validator has no custom messages and the rule was not carrying any before. The defaults the + constructor computed are already the ones that call would produce. Whenever there are custom + messages, or the rule had custom messages from an earlier sync, it recomputes exactly as today. +- No public signatures change, and `$default_messages` stays a fully filled array, so rule + subclasses that read it directly keep working. No **BREAKING** change is intended. + +Out of scope: +- Speeding up validators that do have custom messages beyond what the `MessagesBag` fix gives. + They still recompute per key, because a custom message depends on the field name and a + closure message also gets the value. +- The `invalid_rule` message that `RuleFactory::build_rules_chain()` builds for every rule + piece while parsing. After `validator-parse-once` this runs only once per rule entry + (about 100 calls per validator), so it is not worth a change. +- The playground's `XDEBUG_MODE=debug` / `opcache.enable=0` settings, which belong to the + environment, not the library. + +## Capabilities + +### New Capabilities +- `validation-messages`: which message a failing rule reports (custom message, closure custom + message, or the default from the message table), and that a passing value never needs its + messages looked up. + +### Modified Capabilities + + +## Impact + +- Code: `src/Supports/MessagesBag.php` (`get()`), `src/Validation/ValidationRule.php` + (`sync_custom_messages()`). +- Unchanged: `Validator`, `RuleParser`, `RuleFactory`, the classes in `src/Validation/Rules/`. +- Tests: new tests for custom messages, which have no coverage today; they must pass before and + after the change. +- Depends on `validator-parse-once`, which is implemented but not yet archived. Its numbers are + the "Today" column above. diff --git a/openspec/changes/archive/2026-11-13-validator-message-lookups/specs/validation-messages/spec.md b/openspec/changes/archive/2026-11-13-validator-message-lookups/specs/validation-messages/spec.md new file mode 100644 index 0000000..3f6d6c9 --- /dev/null +++ b/openspec/changes/archive/2026-11-13-validator-message-lookups/specs/validation-messages/spec.md @@ -0,0 +1,60 @@ +## Purpose + +Defines which message a failing validation rule reports, whether it comes from the caller's custom messages +or the framework's message table, and that values which pass do not pay for looking up messages. + +## ADDED Requirements + +### Requirement: Custom messages override the defaults per field +When a validator is given custom messages, a failing rule SHALL report the custom message registered for +that field and rule (`.`) or for that field and constraint (`.`). Keys +may be written flat (`'name.required'`) or nested (`['name' => ['required' => ...]]`). Placeholders in a +custom message SHALL be replaced as in default messages. A closure custom message SHALL be called with +the field name, the value, and the data, and its return value SHALL be reported. A field without a custom +message SHALL report the default message. + +#### Scenario: Flat custom message +- **WHEN** `name => 'required'` fails with custom messages `['name.required' => 'Flat']` +- **THEN** the error for `name` is `Flat` + +#### Scenario: Nested custom message +- **WHEN** `name => 'required'` fails with custom messages `['name' => ['required' => 'Nested']]` +- **THEN** the error for `name` is `Nested` + +#### Scenario: Constraint message with a placeholder +- **WHEN** `name => 'string|max:3'` fails on `'abcdef'` with custom messages `['name.max' => 'At most {max}']` +- **THEN** the error for `name` is `At most 3` + +#### Scenario: Closure custom message +- **WHEN** `name => 'string'` fails on `5` with custom message `name.string` set to a closure returning `"Closure {$name}=" . $value` +- **THEN** the error for `name` is `Closure name=5` + +#### Scenario: Custom message for one expanded wildcard key +- **WHEN** `v.*.s => 'string|max:3'` fails for both items with custom messages `['v.1.s.max' => 'Second only']` +- **THEN** `v.1.s` reports `Second only`, and `v.0.s` reports the default max message + +#### Scenario: Custom message for another field +- **WHEN** `name => 'string|max:3'` fails with custom messages that only contain `other.max` +- **THEN** `name` reports the default max message + +### Requirement: A rule reused across validators does not keep custom messages +A rule object passed to more than one validator SHALL report the messages of the validator that is +running it. A validator without custom messages SHALL report default messages even if the same rule +object reported a custom message in an earlier validator. + +#### Scenario: Shared rule object, custom then default +- **WHEN** one `Rule::string()->max(3)` object fails in validator A with custom messages `['name.max' => 'Custom A']`, and then fails in validator B without custom messages +- **THEN** A reports `Custom A`, and B reports the default max message + +### Requirement: Passing values do not need message lookups +When a validator has no custom messages, checking a value that passes SHALL NOT look up messages in the +message table. Default messages SHALL be looked up when rule instances are built, not again for each +key that is checked. + +#### Scenario: Large passing payload without custom messages +- **WHEN** a validator without custom messages checks a passing payload of 5,956 expanded keys +- **THEN** `passes()` makes no message-table lookups, and returns `true` + +#### Scenario: Default messages still reported +- **WHEN** a validator without custom messages checks `name => 'required'` against an empty value +- **THEN** the error for `name` is the default required message diff --git a/openspec/changes/archive/2026-11-13-validator-message-lookups/tasks.md b/openspec/changes/archive/2026-11-13-validator-message-lookups/tasks.md new file mode 100644 index 0000000..bdfb21f --- /dev/null +++ b/openspec/changes/archive/2026-11-13-validator-message-lookups/tasks.md @@ -0,0 +1,33 @@ +## 1. Baseline + +- [x] 1.1 Run `composer phpcs && composer test:unit` and record the result (expect the three existing line-length warnings only) +- [x] 1.2 Record the "before" `passes()` time for the stress payload (180 variants) with `XDEBUG_MODE=off` and `XDEBUG_MODE=debug` in `design.md` + +## 2. Custom message tests (lock in current behaviour; must pass before any src change) + +- [x] 2.1 Add `tests/Unit/Validation/CustomMessagesTest.php`: flat `name.required` and nested `['name' => ['required' => ...]]` custom messages +- [x] 2.2 Test: `name.max` custom message with `{max}` placeholder is reported as `At most 3` +- [x] 2.3 Test: a closure custom message receives the name and value (`Closure name=5`) +- [x] 2.4 Test: a custom message for `v.1.s.max` applies to that key only; `v.0.s` reports the default +- [x] 2.5 Test: custom messages for another field leave `name` on the default message +- [x] 2.6 Test: one rule object used in validator A (custom `name.max`) and then validator B (no custom messages) reports `Custom A` and then the default +- [x] 2.7 Test: a required field with no custom messages reports the default required message +- [x] 2.8 Run the new tests on the unchanged code and confirm all pass + +## 3. No-lookup test (red) + +- [x] 3.1 Add `tests/Support/Validation/CountingMessagesBag.php`, a `MessagesBag` subclass that counts `get()` calls +- [x] 3.2 Test: with the counting bag bound as the singleton, `passes()` on a passing wildcard payload without custom messages makes 0 lookups; confirm it fails today + +## 4. Implementation (green) + +- [x] 4.1 `MessagesBag::get()` reads `$this->messages` instead of calling `messages()` +- [x] 4.2 `ValidationRule::sync_custom_messages()` returns early when both the incoming and current custom messages are empty; the docblock explains why this is exact and the subclass caveat +- [x] 4.3 Run `composer test:unit -- --filter 'CustomMessagesTest|ValidatorTest|RuleParsingTest|ValidatorRunLifecycleTest'` and confirm all pass + +## 5. Verification + +- [x] 5.1 `composer phpcs && composer test:unit` pass (no new warnings) +- [x] 5.2 Record the "after" `passes()` times (Xdebug off and on) in `design.md` +- [ ] 5.3 Ask the user to re-run `GET /validation-stress?variants=180` in the playground and add the web number +- [x] 5.4 Confirm the diff under `src/` touches only `MessagesBag.php` and `ValidationRule.php` diff --git a/openspec/changes/archive/2026-11-13-validator-parse-once/.openspec.yaml b/openspec/changes/archive/2026-11-13-validator-parse-once/.openspec.yaml new file mode 100644 index 0000000..22ca07d --- /dev/null +++ b/openspec/changes/archive/2026-11-13-validator-parse-once/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-11-13 diff --git a/openspec/changes/archive/2026-11-13-validator-parse-once/bench/ValidatorBenchTest.php b/openspec/changes/archive/2026-11-13-validator-parse-once/bench/ValidatorBenchTest.php new file mode 100644 index 0000000..212e7e9 --- /dev/null +++ b/openspec/changes/archive/2026-11-13-validator-parse-once/bench/ValidatorBenchTest.php @@ -0,0 +1,46 @@ + 'required|array']; + foreach ($fields as $name => $r) { $rules["variants.*.$name"] = $r; } + $variant = []; + foreach ($fields as $name => $r) { $variant[$name] = str_contains($r, 'string') ? 'abc' : 5; } + $data = ['variants' => array_fill(0, 180, $variant)]; + + $t = microtime(true); + $v = Validator::make($data, $rules); + $parse = microtime(true) - $t; + $t = microtime(true); $v->passes(); $passes = microtime(true) - $t; + $t = microtime(true); $v->validated(); $validated = microtime(true) - $t; + + // Isolate: RuleFactory::make per expanded key vs once per raw rule + $t = microtime(true); + foreach ($v->get_rules() as $k => $_) { $raw = $rules[preg_replace('/\.\d+\./', '.*.', $k)]; RuleFactory::make(explode('|', $raw)); } + $per_key = microtime(true) - $t; + $t = microtime(true); + foreach ($rules as $raw) { RuleFactory::make(explode('|', $raw)); } + $per_rule = microtime(true) - $t; + + $t = microtime(true); (new RuleParser($data, $rules))->explode_wildcards(); $explode = microtime(true) - $t; + + $srules = []; foreach ($fields as $name => $r) { $srules["variants.*.$name"] = 'text'; } + $t = microtime(true); Sanitizer::make($data, $srules); $san = microtime(true) - $t; + + fwrite(STDERR, sprintf("\nkeys=%d\nValidator::make (parse) %.1f ms\n of which explode_wildcards %.1f ms\npasses() %.1f ms\nvalidated() %.1f ms\nRuleFactory per expanded key %.1f ms\nRuleFactory once per raw rule %.2f ms\nSanitizer 4680 keys %.1f ms\n", + count($v->get_rules()), $parse*1e3, $explode*1e3, $passes*1e3, $validated*1e3, $per_key*1e3, $per_rule*1e3, $san*1e3)); + $t = microtime(true); for ($i = 0; $i < 4681 * 3; $i++) { \Framework\Validation\Rule::string(); } fwrite(STDERR, sprintf("new rule x14043 (constructor+message lookups) %.1f ms\n", (microtime(true)-$t)*1e3)); $this->assertTrue(true); + } +} diff --git a/openspec/changes/archive/2026-11-13-validator-parse-once/design.md b/openspec/changes/archive/2026-11-13-validator-parse-once/design.md new file mode 100644 index 0000000..1bab68b --- /dev/null +++ b/openspec/changes/archive/2026-11-13-validator-parse-once/design.md @@ -0,0 +1,121 @@ +## Context + +See proposal.md (Why) for the measurements. + +`Validator::__construct()` → `parse_rules()` → `RuleParser::parse()` runs two passes: + +1. `explode_wildcards()` replaces each `*` key with its concrete keys. Every concrete key is given + the same raw rule value (string, array, object or closure). The source key is not kept. +2. `process_rules()` loops over the concrete keys and, for each one, calls + `RuleFactory::make(spread(make_array($rules)))`. + +The output of step 2 depends only on the raw value and `$this->data`. `spread()` calls +`ConditionalRule::passes($this->data)` with the whole dataset, not with the key. So for a +given parse, every concrete key that comes from one source entry produces the same output. + +Rule instances are stateful but only in a resettable way. `Validator::apply()` calls `with_data`, +`with_name`, `with_value`, `clear_error_messages` and `sync_custom_messages` before every +`passed()`. Every property on `ValidationRule`, `Fluent` and the classes in `Rules/` is a scalar +or an array, so a shallow `clone` produces a fully independent copy. + +`Validator::passes()` never resets `$errors`. `validated()` calls `passes()` again whenever +`$errors` is empty, which is always the case after a successful `fails()`. + +## Goals / Non-Goals + +**Goals:** +- One `RuleFactory::make()` call per source entry, scoped to a single `RuleParser`. +- One run of the rules per `Validator` state. +- Same pass/fail results, messages and `validated()` output as today. + +**Non-Goals:** +- No change to `RuleFactory`, the rule classes, message resolution, or `Sanitizer`. +- No static or cross-validator cache. +- No change to the quadratic `preg_grep` in `validated()` (noted, separate). +- No change to wildcard-expansion semantics (`array_merge` re-indexing, empty-array handling). + +## Decisions + +### D1. RuleParser records the source key for each concrete key + +`explode_wildcards()` fills a new `protected array $sources` (concrete key → source key). It +records each key from `explode_array_rule()` and maps every non-wildcard key to itself. +`process_rules()` keeps a local `$parsed_by_source` map: + +- first concrete key of a source → `RuleFactory::make(...)`, stored, and assigned as-is; +- every later key of the same source → `array_map(fn ($rule) => clone $rule, $stored)`. + +*Alternatives:* +- **Memoise by rule string.** Rejected: it only works for string rules, because arrays + holding objects or closures have no stable key, and it would save nothing over 27 parses. +- **Static cache in `RuleFactory`.** Rejected: `Rule::when()` outcomes depend on the data, + and in long-running processes it would carry stale state between validators. +- **Re-derive the source from the concrete key.** Rejected: `0` cannot be told apart from + `*`, and it is fragile under nested wildcards. + +### D2. The first key keeps the parsed instances; later keys get clones + +Giving the first key the originals keeps today's identity for non-wildcard keys: a rule object +passed for a plain key is still the same object in `get_rules()`. A shallow clone is enough (see +Context) and skips the constructor, so its message lookups do not run again. Closures are +cloned too; `clone $closure` is valid on PHP 7.4. + +*Alternative:* share one set of instances across all keys. Rejected in grilling: siblings +would become aliases of each other through the public `get_rules()`. + +### D3. Validator stores its outcome in a nullable flag + +Add `protected ?bool $passed = null;`. Typed properties already appear in `Validator`, so +this matches the class. + +- `passes()`: if `$this->passed !== null`, return it. Otherwise run the loop as today and set + `$this->passed = empty($this->errors)`. +- `fails()`: unchanged (`!$this->passes()`), so it inherits the cache. +- `validated()`: replace "run `passes()` if errors are empty, then check errors" with "throw + if `!$this->passes()`". `validate()` needs no change. +- New `protected function reset_outcome()` sets `$passed = null` and `$errors = []`. + `set_value()` always calls it, and `sometimes()` calls it inside its `if`, after the merge. + +*Alternative:* a separate `bool $has_run` alongside the errors. Rejected: the nullable flag +holds both "has run" and "result" in one field. + +### D4. Tests are behavioural, the benchmark stays out of the suite + +The tests go in `tests/Unit/Validation/` and follow the spec scenarios. A closure that counts +its calls proves a single run. `assertNotSame` / `assertEquals` on `get_rules()` prove cloning. +There is no timing assertion; the before/after numbers come from `bench/ValidatorBenchTest.php` in this change and are +recorded when the work is applied. + +## Risks / Trade-offs + +- [A caller relied on one rule object being shared across wildcard keys, for example by + mutating it after `make()`] → Unlikely and undocumented. The proposal mentions the change, and + plain keys keep their identity (D2). +- [A future rule class gains an object-valued property that must not be shared] → Shallow + clone would share it. Mitigation: a note in the `process_rules()` docblock. A rule like that + can add `__clone()`. +- [Code that changes `$data` other than through `set_value()` would see a stale outcome] → + `$data` is protected, and `set_value()` is the only public mutator. Subclasses that write + `$this->data` directly must call `reset_outcome()`, which its docblock says. +- [`stop_on_first_failure` changes after a run would be ignored] → It has no public setter + today, so this cannot happen. Revisit if one is added. + +## Migration Plan + +Internal change with no signature changes; ships in the next framework release. Rollback is a +revert of the two files. + +## Benchmark + +`XDEBUG_MODE=off composer test:unit -- openspec/changes/validator-parse-once/bench/ValidatorBenchTest.php` +(PHP 8.4, 180 variants × 26 fields = 4,681 keys). + +Baseline gate: `composer test:unit` OK (953 tests, 2252 assertions). `composer phpcs` exits 1 on three +pre-existing line-length **warnings** (`src/View/TemplateEngine.php:276,282`, +`src/Validation/ValidationRule.php:396`), 0 errors — not touched by this change. + +| Step | Before | After | +|---|---|---| +| `Validator::make()` (parse) | 510.7 ms | 8.7 ms | +| `passes()` | 176.1 ms | 165.7 ms (unchanged: per-key message lookups, out of scope) | +| `validated()` after `passes()` | 170.6 ms | 4.3 ms (builds the result only) | diff --git a/openspec/changes/archive/2026-11-13-validator-parse-once/proposal.md b/openspec/changes/archive/2026-11-13-validator-parse-once/proposal.md new file mode 100644 index 0000000..b692e26 --- /dev/null +++ b/openspec/changes/archive/2026-11-13-validator-parse-once/proposal.md @@ -0,0 +1,68 @@ +## Why + +The validator rebuilds the rule instances for every concrete key a wildcard rule expands to. A product +with 180 variants and 26 `variants.*.` rules expands to 4,681 keys. `RuleParser::process_rules()` +calls `RuleFactory::make()` once per key, so each rule string is parsed 180 times, and each parse builds +about five rule objects. Every object's constructor looks up translated messages. On top of that, +`validate()` runs every rule twice, because `validated()` calls `passes()` again after `fails()` already +did. + +Measured on PHP 8.4 with Xdebug off and the 4,681-key payload above: + +| Step | Today | +|---|---| +| `Validator::make()` (rule parsing) | 522 ms | +| …of which `RuleFactory::make` per expanded key | 520 ms | +| …the same work done once per raw rule (27 entries) | 2.8 ms | +| `passes()` | 171 ms | +| `validated()` (runs `passes()` again) | 173 ms | + +The sanitizer has no parse step: it dispatches on a string per key and takes 2 ms on the same +payload. It is not part of this change. + +## What Changes + +- `RuleParser` parses each entry of the rules array once, however many concrete keys its wildcard + expands to. Each expanded key gets its own shallow `clone` of the parsed instances. This includes + closures and rule objects the caller passes in, which today are shared by every expanded key. The + cache lasts for one parse only; nothing is cached statically, because a `Rule::when()` outcome + depends on the data being validated. +- `Validator` remembers whether it has run and what the outcome was: + - A second call to `passes()` or `fails()` returns the stored outcome instead of validating + again. This fixes a current bug: calling `fails()` and then `passes()` on invalid data + reports every error message twice. + - `validated()` and `validate()` call `passes()` only when validation has not run yet. + - `sometimes()` (when it adds rules) and `set_value()` throw away the stored outcome and the + errors, so the next call validates again. +- No public method signatures change, and no **BREAKING** change is intended. Two effects are + visible to callers: + - Calling `get_rules()` on a wildcard rule now returns separate cloned instances for each + key, where a rule object you passed in used to come back as the same object for every key. + - A repeat `passes()` call no longer appends duplicate errors. + +Out of scope, for a separate change: caching the default-message lookups that every rule constructor +and `sync_custom_messages()` performs, and removing the throwaway probe instance that +`RuleFactory::group_under_base_rule()` builds for each piece of a rule string. + +## Capabilities + +### New Capabilities +- `validation-rule-parsing`: how wildcard rule keys are expanded and turned into rule instances, + including the guarantee that each source rule is parsed once and that every expanded key gets + its own instances. +- `validation-run-lifecycle`: when a `Validator` runs its rules, when it reuses a stored outcome, + and which calls throw that outcome away. + +### Modified Capabilities + + +## Impact + +- Code: `src/Validation/RuleParser.php` (wildcard expansion keeps the source key; `process_rules()` + parses once per source entry and clones), `src/Validation/Validator.php` (run flag, stored + outcome, invalidation in `sometimes()` / `set_value()`). +- `RuleFactory`, the rule classes, `Sanitizer`: unchanged. +- Tests: new behaviour tests under `tests/Unit/Validation/`. +- Public API: no signature changes; the facade-free `Validator` keeps its methods. +- Docs: covered by the two new capability specs and method docblocks; no `docs/validation.md` in + this change. diff --git a/openspec/changes/archive/2026-11-13-validator-parse-once/specs/validation-rule-parsing/spec.md b/openspec/changes/archive/2026-11-13-validator-parse-once/specs/validation-rule-parsing/spec.md new file mode 100644 index 0000000..5177992 --- /dev/null +++ b/openspec/changes/archive/2026-11-13-validator-parse-once/specs/validation-rule-parsing/spec.md @@ -0,0 +1,58 @@ +## Purpose + +Defines how a validator turns its rules array into rule instances when a rule key uses `*` wildcards, +so that large payloads cost one parse per rule entry rather than one per concrete key. + +## ADDED Requirements + +### Requirement: Wildcard keys expand against the data +A rule key containing `*` SHALL expand into one concrete dot-notated key for every matching path in +the data being validated. A wildcard that matches no data SHALL produce no keys. A key without `*` +SHALL be kept as given. + +#### Scenario: Wildcard expands per item +- **WHEN** the data holds three `variants` items and the rules contain `variants.*.price => 'required|numeric'` +- **THEN** the validator's rules contain `variants.0.price`, `variants.1.price` and `variants.2.price` + +#### Scenario: Wildcard over an empty or missing array +- **WHEN** the rules contain `variants.*.price` and the data has no `variants` items +- **THEN** no `variants.*.price` key is produced + +### Requirement: Each rule entry is parsed once per validator +The validator SHALL turn each entry of the rules array into rule instances once, however many concrete +keys that entry's wildcard expands to. The parsed result SHALL be valid only for the validator that +produced it, and SHALL NOT be reused by another validator instance. + +#### Scenario: Large wildcard payload +- **WHEN** a validator is made for 180 `variants` items with 26 `variants.*.` rule entries +- **THEN** the rules are parsed 27 times in total (one per entry, including `variants`), not once per each of the 4,681 expanded keys, and every expanded key carries the rules of its entry + +#### Scenario: Conditional rule outcome is not reused across validators +- **WHEN** two validators are made with the same `Rule::when()` wildcard rule but data that makes the condition true for one and false for the other +- **THEN** each validator applies the branch its own data selects + +### Requirement: Each expanded key owns its rule instances +Every concrete key produced from one wildcard entry SHALL hold its own rule instances. Changing a +rule instance obtained for one key SHALL NOT change the rules of any other key. This SHALL hold for +string rules, rule objects passed by the caller, closures, and the rules that `Rule::when()` / +`Rule::unless()` select. + +#### Scenario: String rule instances are distinct per key +- **WHEN** `variants.*.sku => 'required|string|max:50'` expands to `variants.0.sku` and `variants.1.sku` +- **THEN** the rule instances for the two keys are equal in type and constraints but are not the same objects + +#### Scenario: Caller-supplied rule object is distinct per key +- **WHEN** the rules contain `variants.*.sku => [Rule::string()]` and the data holds two items +- **THEN** the two expanded keys hold different `StringRule` instances, and adding a constraint to one key's instance leaves the other key's instance unchanged + +#### Scenario: Closure rule runs for every key +- **WHEN** the rules contain `variants.*.sku => [function ($value) { ... }]` and the data holds three items +- **THEN** the closure is called once for each of the three keys, with that key's value + +### Requirement: Validation results are unchanged by parsing once +Parsing once SHALL NOT change which keys pass or fail, the error messages reported for each key, or +the data returned by `validated()`, compared with parsing every expanded key separately. + +#### Scenario: Errors are reported per expanded key +- **WHEN** `variants.*.price => 'required|numeric|min:0'` is validated against items whose prices are `5`, `-1` and `'abc'` +- **THEN** `variants.0.price` has no error, and `variants.1.price` and `variants.2.price` each carry their own error messages diff --git a/openspec/changes/archive/2026-11-13-validator-parse-once/specs/validation-run-lifecycle/spec.md b/openspec/changes/archive/2026-11-13-validator-parse-once/specs/validation-run-lifecycle/spec.md new file mode 100644 index 0000000..11a8993 --- /dev/null +++ b/openspec/changes/archive/2026-11-13-validator-parse-once/specs/validation-run-lifecycle/spec.md @@ -0,0 +1,41 @@ +## Purpose + +Defines when a validator actually runs its rules, when it reuses the outcome of an earlier run, and which +calls invalidate that outcome, so that every rule runs once per run and errors are never reported twice. + +## ADDED Requirements + +### Requirement: A validator runs its rules once per state +A validator SHALL run its rules the first time its outcome is needed, by `passes()`, `fails()`, +`validate()` or `validated()`. Later calls SHALL reuse the stored outcome and errors, and SHALL NOT run +the rules again until the stored outcome is invalidated. + +#### Scenario: validate() runs each rule once +- **WHEN** a closure rule that counts its calls is validated through `validate()` on data that passes +- **THEN** the closure is called once per key, and `validate()` returns the validated data + +#### Scenario: passes() followed by validated() +- **WHEN** `passes()` returns `true` and `validated()` is called afterwards +- **THEN** the rules are not run a second time, and `validated()` returns the validated data + +#### Scenario: Repeat calls do not duplicate errors +- **WHEN** `fails()` is called on invalid data and then `passes()` is called on the same validator +- **THEN** `passes()` returns `false` and `errors()` holds each error message once, as it did after `fails()` + +### Requirement: Changing rules or data invalidates the outcome +A call that changes what a validator checks SHALL clear the stored outcome and errors, so the next +`passes()`, `fails()`, `validate()` or `validated()` runs the rules again against the current rules and +data. `set_value()` SHALL always invalidate. `sometimes()` SHALL invalidate only when its callback +returns true and rules are added. + +#### Scenario: sometimes() adds a rule after a run +- **WHEN** a validator passes, then `sometimes()` adds a rule that the data violates, and `passes()` is called again +- **THEN** `passes()` returns `false` and `errors()` contains the new rule's error + +#### Scenario: sometimes() with a false callback +- **WHEN** a validator has run and `sometimes()` is called with a callback that returns false +- **THEN** the stored outcome is kept and the rules are not run again by the next `passes()` + +#### Scenario: set_value() after a failed run +- **WHEN** a validator fails because a required field is empty, then `set_value()` fills that field, and `passes()` is called again +- **THEN** `passes()` returns `true` and `errors()` is empty diff --git a/openspec/changes/archive/2026-11-13-validator-parse-once/tasks.md b/openspec/changes/archive/2026-11-13-validator-parse-once/tasks.md new file mode 100644 index 0000000..e8dcfe7 --- /dev/null +++ b/openspec/changes/archive/2026-11-13-validator-parse-once/tasks.md @@ -0,0 +1,44 @@ +## 1. Baseline + +- [x] 1.1 Run `composer phpcs && composer test:unit` on a clean tree and record the result +- [x] 1.2 Run `XDEBUG_MODE=off composer test:unit -- openspec/changes/validator-parse-once/bench/ValidatorBenchTest.php` (180 variants × 26 fields) and record the "before" numbers for `Validator::make`, `passes()` and `validated()` + +## 2. Rule parsing tests (red) + +- [x] 2.1 Add `tests/Unit/Validation/RuleParsingTest.php`: wildcard expands per item, and an empty or missing array produces no keys +- [x] 2.2 Test: string-rule instances for two expanded keys are `assertEquals` but `assertNotSame` +- [x] 2.3 Test: a caller-supplied `Rule::string()` on a wildcard key gives each key its own instance, and changing one leaves the other unchanged +- [x] 2.4 Test: a closure rule on a wildcard key is called once per expanded key with that key's value +- [x] 2.5 Test: `Rule::when()` on a wildcard key selects its branch from each validator's own data (two validators, opposite conditions) +- [x] 2.6 Test: `variants.*.price => 'required|numeric|min:0'` with prices `5`, `-1`, `'abc'` reports errors only for keys 1 and 2 +- [x] 2.7 Test: a non-wildcard key given a rule object keeps that same object in `get_rules()` + +## 3. Rule parsing implementation (green) + +- [x] 3.1 Add `protected array $sources` to `RuleParser` (docblock, `@since 1.0.0`), filled by `explode_wildcards()` with concrete key → source key, and with plain keys mapped to themselves +- [x] 3.2 Change `process_rules()` to parse once per source key: the first concrete key gets the parsed instances and later keys get shallow clones; explain in the docblock why a shallow clone is enough +- [x] 3.3 Run `composer test:unit -- --filter 'RuleParsingTest|ValidatorTest'` and confirm the tests pass + +## 4. Run lifecycle tests (red) + +- [x] 4.1 Add `tests/Unit/Validation/ValidatorRunLifecycleTest.php`: a counting closure shows `validate()` runs each key once on passing data +- [x] 4.2 Test: `passes()` then `validated()` does not run the rules again +- [x] 4.3 Test: `fails()` then `passes()` on invalid data returns `false`, and each error appears once +- [x] 4.4 Test: after a passing run, `sometimes()` with a true callback adds a failing rule, and `passes()` then returns `false` with the new error +- [x] 4.5 Test: `sometimes()` with a false callback keeps the stored outcome (the counter does not go up) +- [x] 4.6 Test: after a failing run, `set_value()` fills the required field, and `passes()` then returns `true` with no errors + +## 5. Run lifecycle implementation (green) + +- [x] 5.1 Add `protected ?bool $passed = null` to `Validator` (docblock, `@since 1.0.0`) +- [x] 5.2 `passes()` returns the stored outcome when it is set; otherwise it runs and stores `empty($this->errors)` +- [x] 5.3 `validated()` throws when `!$this->passes()` and no longer runs `passes()` a second time +- [x] 5.4 Add `protected function reset_outcome()` that clears `$passed` and `$errors`, with a docblock noting that subclasses writing `$this->data` must call it +- [x] 5.5 Call `reset_outcome()` from `set_value()`, and from `sometimes()` after its rules are merged +- [x] 5.6 Update the docblocks of `passes()`, `fails()`, `validated()`, `set_value()` and `sometimes()` to describe the caching and invalidation + +## 6. Verification + +- [x] 6.1 `composer phpcs && composer test:unit` pass +- [x] 6.2 Re-run the benchmark in `bench/` and record the "after" numbers next to the baseline in `design.md`, aiming for `Validator::make` of about 10 ms and `validated()` of about 0 ms after `passes()` +- [x] 6.3 Confirm `RuleFactory`, the classes in `src/Validation/Rules/` and `src/Sanitizer.php` are unchanged in the diff diff --git a/openspec/specs/validation-messages/spec.md b/openspec/specs/validation-messages/spec.md new file mode 100644 index 0000000..f40a965 --- /dev/null +++ b/openspec/specs/validation-messages/spec.md @@ -0,0 +1,60 @@ +## Purpose + +Defines which message a failing validation rule reports, whether it comes from the caller's custom messages +or the framework's message table, and that values which pass do not pay for looking up messages. + +## Requirements + +### Requirement: Custom messages override the defaults per field +When a validator is given custom messages, a failing rule SHALL report the custom message registered for +that field and rule (`.`) or for that field and constraint (`.`). Keys +may be written flat (`'name.required'`) or nested (`['name' => ['required' => ...]]`). Placeholders in a +custom message SHALL be replaced as in default messages. A closure custom message SHALL be called with +the field name, the value, and the data, and its return value SHALL be reported. A field without a custom +message SHALL report the default message. + +#### Scenario: Flat custom message +- **WHEN** `name => 'required'` fails with custom messages `['name.required' => 'Flat']` +- **THEN** the error for `name` is `Flat` + +#### Scenario: Nested custom message +- **WHEN** `name => 'required'` fails with custom messages `['name' => ['required' => 'Nested']]` +- **THEN** the error for `name` is `Nested` + +#### Scenario: Constraint message with a placeholder +- **WHEN** `name => 'string|max:3'` fails on `'abcdef'` with custom messages `['name.max' => 'At most {max}']` +- **THEN** the error for `name` is `At most 3` + +#### Scenario: Closure custom message +- **WHEN** `name => 'string'` fails on `5` with custom message `name.string` set to a closure returning `"Closure {$name}=" . $value` +- **THEN** the error for `name` is `Closure name=5` + +#### Scenario: Custom message for one expanded wildcard key +- **WHEN** `v.*.s => 'string|max:3'` fails for both items with custom messages `['v.1.s.max' => 'Second only']` +- **THEN** `v.1.s` reports `Second only`, and `v.0.s` reports the default max message + +#### Scenario: Custom message for another field +- **WHEN** `name => 'string|max:3'` fails with custom messages that only contain `other.max` +- **THEN** `name` reports the default max message + +### Requirement: A rule reused across validators does not keep custom messages +A rule object passed to more than one validator SHALL report the messages of the validator that is +running it. A validator without custom messages SHALL report default messages even if the same rule +object reported a custom message in an earlier validator. + +#### Scenario: Shared rule object, custom then default +- **WHEN** one `Rule::string()->max(3)` object fails in validator A with custom messages `['name.max' => 'Custom A']`, and then fails in validator B without custom messages +- **THEN** A reports `Custom A`, and B reports the default max message + +### Requirement: Passing values do not need message lookups +When a validator has no custom messages, checking a value that passes SHALL NOT look up messages in the +message table. Default messages SHALL be looked up when rule instances are built, not again for each +key that is checked. + +#### Scenario: Large passing payload without custom messages +- **WHEN** a validator without custom messages checks a passing payload of 5,956 expanded keys +- **THEN** `passes()` makes no message-table lookups, and returns `true` + +#### Scenario: Default messages still reported +- **WHEN** a validator without custom messages checks `name => 'required'` against an empty value +- **THEN** the error for `name` is the default required message diff --git a/openspec/specs/validation-rule-parsing/spec.md b/openspec/specs/validation-rule-parsing/spec.md new file mode 100644 index 0000000..01db6aa --- /dev/null +++ b/openspec/specs/validation-rule-parsing/spec.md @@ -0,0 +1,58 @@ +## Purpose + +Defines how a validator turns its rules array into rule instances when a rule key uses `*` wildcards, +so that large payloads cost one parse per rule entry rather than one per concrete key. + +## Requirements + +### Requirement: Wildcard keys expand against the data +A rule key containing `*` SHALL expand into one concrete dot-notated key for every matching path in +the data being validated. A wildcard that matches no data SHALL produce no keys. A key without `*` +SHALL be kept as given. + +#### Scenario: Wildcard expands per item +- **WHEN** the data holds three `variants` items and the rules contain `variants.*.price => 'required|numeric'` +- **THEN** the validator's rules contain `variants.0.price`, `variants.1.price` and `variants.2.price` + +#### Scenario: Wildcard over an empty or missing array +- **WHEN** the rules contain `variants.*.price` and the data has no `variants` items +- **THEN** no `variants.*.price` key is produced + +### Requirement: Each rule entry is parsed once per validator +The validator SHALL turn each entry of the rules array into rule instances once, however many concrete +keys that entry's wildcard expands to. The parsed result SHALL be valid only for the validator that +produced it, and SHALL NOT be reused by another validator instance. + +#### Scenario: Large wildcard payload +- **WHEN** a validator is made for 180 `variants` items with 26 `variants.*.` rule entries +- **THEN** the rules are parsed 27 times in total (one per entry, including `variants`), not once per each of the 4,681 expanded keys, and every expanded key carries the rules of its entry + +#### Scenario: Conditional rule outcome is not reused across validators +- **WHEN** two validators are made with the same `Rule::when()` wildcard rule but data that makes the condition true for one and false for the other +- **THEN** each validator applies the branch its own data selects + +### Requirement: Each expanded key owns its rule instances +Every concrete key produced from one wildcard entry SHALL hold its own rule instances. Changing a +rule instance obtained for one key SHALL NOT change the rules of any other key. This SHALL hold for +string rules, rule objects passed by the caller, closures, and the rules that `Rule::when()` / +`Rule::unless()` select. + +#### Scenario: String rule instances are distinct per key +- **WHEN** `variants.*.sku => 'required|string|max:50'` expands to `variants.0.sku` and `variants.1.sku` +- **THEN** the rule instances for the two keys are equal in type and constraints but are not the same objects + +#### Scenario: Caller-supplied rule object is distinct per key +- **WHEN** the rules contain `variants.*.sku => [Rule::string()]` and the data holds two items +- **THEN** the two expanded keys hold different `StringRule` instances, and adding a constraint to one key's instance leaves the other key's instance unchanged + +#### Scenario: Closure rule runs for every key +- **WHEN** the rules contain `variants.*.sku => [function ($value) { ... }]` and the data holds three items +- **THEN** the closure is called once for each of the three keys, with that key's value + +### Requirement: Validation results are unchanged by parsing once +Parsing once SHALL NOT change which keys pass or fail, the error messages reported for each key, or +the data returned by `validated()`, compared with parsing every expanded key separately. + +#### Scenario: Errors are reported per expanded key +- **WHEN** `variants.*.price => 'required|numeric|min:0'` is validated against items whose prices are `5`, `-1` and `'abc'` +- **THEN** `variants.0.price` has no error, and `variants.1.price` and `variants.2.price` each carry their own error messages diff --git a/openspec/specs/validation-run-lifecycle/spec.md b/openspec/specs/validation-run-lifecycle/spec.md new file mode 100644 index 0000000..e99ae17 --- /dev/null +++ b/openspec/specs/validation-run-lifecycle/spec.md @@ -0,0 +1,41 @@ +## Purpose + +Defines when a validator actually runs its rules, when it reuses the outcome of an earlier run, and which +calls invalidate that outcome, so that every rule runs once per run and errors are never reported twice. + +## Requirements + +### Requirement: A validator runs its rules once per state +A validator SHALL run its rules the first time its outcome is needed, by `passes()`, `fails()`, +`validate()` or `validated()`. Later calls SHALL reuse the stored outcome and errors, and SHALL NOT run +the rules again until the stored outcome is invalidated. + +#### Scenario: validate() runs each rule once +- **WHEN** a closure rule that counts its calls is validated through `validate()` on data that passes +- **THEN** the closure is called once per key, and `validate()` returns the validated data + +#### Scenario: passes() followed by validated() +- **WHEN** `passes()` returns `true` and `validated()` is called afterwards +- **THEN** the rules are not run a second time, and `validated()` returns the validated data + +#### Scenario: Repeat calls do not duplicate errors +- **WHEN** `fails()` is called on invalid data and then `passes()` is called on the same validator +- **THEN** `passes()` returns `false` and `errors()` holds each error message once, as it did after `fails()` + +### Requirement: Changing rules or data invalidates the outcome +A call that changes what a validator checks SHALL clear the stored outcome and errors, so the next +`passes()`, `fails()`, `validate()` or `validated()` runs the rules again against the current rules and +data. `set_value()` SHALL always invalidate. `sometimes()` SHALL invalidate only when its callback +returns true and rules are added. + +#### Scenario: sometimes() adds a rule after a run +- **WHEN** a validator passes, then `sometimes()` adds a rule that the data violates, and `passes()` is called again +- **THEN** `passes()` returns `false` and `errors()` contains the new rule's error + +#### Scenario: sometimes() with a false callback +- **WHEN** a validator has run and `sometimes()` is called with a callback that returns false +- **THEN** the stored outcome is kept and the rules are not run again by the next `passes()` + +#### Scenario: set_value() after a failed run +- **WHEN** a validator fails because a required field is empty, then `set_value()` fills that field, and `passes()` is called again +- **THEN** `passes()` returns `true` and `errors()` is empty diff --git a/src/Supports/MessagesBag.php b/src/Supports/MessagesBag.php index 11f2c08..f965e50 100644 --- a/src/Supports/MessagesBag.php +++ b/src/Supports/MessagesBag.php @@ -207,13 +207,16 @@ protected function app_defined_messages() /** * Get the messages. * + * App defined messages are merged recursively, so overriding one message keeps every default + * beside it instead of replacing its whole group. + * * @return array * * @since 1.0.0 */ protected function messages() { - return array_merge( + return array_replace_recursive( $this->defaults(), $this->app_defined_messages(), ); @@ -222,6 +225,9 @@ protected function messages() /** * Get a message by key. * + * Reads the table built by the constructor; the app-defined part comes from config(), which is + * already cached for the request, so rebuilding it per call could not change the result. + * * @param string $key The key. * @param mixed $args The arguments. * @@ -232,7 +238,7 @@ protected function messages() public function get(string $key, ...$args) { $args = !empty($args) ? Arr::flatten($args) : []; - $message = Arr::get($this->messages(), $key, ''); + $message = Arr::get($this->messages, $key, ''); throw_if(is_array($message), 'You may forget to define the full path of the message key.', InvalidArgumentException::class); diff --git a/src/Validation/RuleParser.php b/src/Validation/RuleParser.php index 1fa7b77..15162a7 100644 --- a/src/Validation/RuleParser.php +++ b/src/Validation/RuleParser.php @@ -36,6 +36,15 @@ class RuleParser */ protected array $rules; + /** + * The rule key each concrete key was expanded from, so a wildcard entry is parsed once. + * + * @var array + * + * @since 1.0.0 + */ + protected array $sources = []; + /** * Create a new rule parser instance. * @@ -87,13 +96,19 @@ public function all() public function explode_wildcards() { $exploded = []; + $this->sources = []; foreach ($this->rules as $key => $rules) { if (str_contains($key, '*')) { $array_rules = $this->explode_array_rule($key, $rules); $exploded = array_merge($exploded, $array_rules); + + foreach (array_keys($array_rules) as $concrete_key) { + $this->sources[$concrete_key] = $key; + } } else { $exploded[$key] = $rules; + $this->sources[$key] = $key; } } @@ -105,6 +120,12 @@ public function explode_wildcards() /** * Build rule instances for every parsed rule definition. * + * Every concrete key expanded from one wildcard entry would parse to the same result, so the + * entry is parsed once: its first key takes the built instances and each later key takes + * shallow clones. A shallow clone is a full copy because rule instances hold only scalars and + * arrays, and the validator resets their per-key state before each check. A rule that gains an + * object property which must not be shared should implement __clone(). + * * @return $this * * @since 1.0.0 @@ -112,9 +133,17 @@ public function explode_wildcards() public function process_rules() { $parsed = []; + $parsed_by_source = []; foreach ($this->rules as $key => $rules) { - $parsed[$key] = RuleFactory::make( + $source = $this->sources[$key] ?? $key; + + if (isset($parsed_by_source[$source])) { + $parsed[$key] = array_map(fn ($rule) => clone $rule, $parsed_by_source[$source]); + continue; + } + + $parsed[$key] = $parsed_by_source[$source] = RuleFactory::make( $this->spread( $this->make_array($rules) ) diff --git a/src/Validation/ValidationRule.php b/src/Validation/ValidationRule.php index d5bccc2..d8b4890 100644 --- a/src/Validation/ValidationRule.php +++ b/src/Validation/ValidationRule.php @@ -304,6 +304,10 @@ public function clear_error_messages() /** * Set the custom messages to return. * + * With no custom messages now or before, the default messages built by the constructor are + * exactly what recomputing would give, so the per-check lookups are skipped. A subclass whose + * default messages depend on the name, value, or data must override this method. + * * @param array $messages The custom messages to return. * * @return $this @@ -312,6 +316,10 @@ public function clear_error_messages() */ public function sync_custom_messages(array $messages) { + if ($messages === [] && $this->custom_messages === []) { + return $this; + } + $this->custom_messages = $messages; $this->sync_default_messages_with_custom_messages(); diff --git a/src/Validation/Validator.php b/src/Validation/Validator.php index 6beaa63..a9115bb 100644 --- a/src/Validation/Validator.php +++ b/src/Validation/Validator.php @@ -79,6 +79,15 @@ class Validator */ protected bool $stop_on_first_failure = false; + /** + * The outcome of the last run, or null when the rules have not run against the current state. + * + * @var bool|null + * + * @since 1.0.0 + */ + protected ?bool $passed = null; + /** * Create a new validator instance. * @@ -157,16 +166,34 @@ public function get_value($attribute) /** * Set the value of an attribute. * + * Discards the stored outcome and errors, so the next check validates the changed data. + * * @param string $attribute The attribute to set the value of. * @param mixed $value The value to set. * * @return void - * + * * @since 1.0.0 */ public function set_value($attribute, $value) { Arr::set($this->data, $attribute, $value); + $this->reset_outcome(); + } + + /** + * Discard the stored outcome and errors so the next check runs the rules again. + * + * A subclass that writes $this->data or $this->rules directly must call this. + * + * @return void + * + * @since 1.0.0 + */ + protected function reset_outcome() + { + $this->passed = null; + $this->errors = []; } /** @@ -196,12 +223,19 @@ public function get_data() /** * Validate the data. * + * The rules run once; later calls return the stored outcome until set_value() or sometimes() + * changes what is being checked. + * * @return bool - * + * * @since 1.0.0 */ public function passes() { + if ($this->passed !== null) { + return $this->passed; + } + foreach ($this->rules as $key => $rules) { if ($this->stop_on_first_failure && !empty($this->errors())) { break; @@ -210,7 +244,7 @@ public function passes() $this->validate_rules($key, $rules); } - return empty($this->errors()); + return $this->passed = empty($this->errors()); } /** @@ -323,6 +357,8 @@ protected function add_error(string $key, $messages) /** * Check if the validation failed. * + * Shares the stored outcome with passes(), so it does not run the rules a second time. + * * @return bool * * @since 1.0.0 @@ -363,17 +399,15 @@ public function validate() /** * Get the validated data. * + * Runs the rules only if they have not run against the current state. + * * @return array - * + * * @since 1.0.0 */ public function validated() { - if (empty($this->errors())) { - $this->passes(); - } - - if (!empty($this->errors())) { + if (!$this->passes()) { throw_exception(ValidationException::with_errors($this->errors())); } @@ -420,6 +454,9 @@ protected function has_array_rule(array $rules) /** * Sometimes the rules should be applied. * + * When the callback adds rules, the stored outcome and errors are discarded so the next check + * runs the full rule set. + * * @param string $key The key to apply the rules to. * @param array|string $rules The rules to apply. * @param callable $callback The callback to check if the rules should be applied. @@ -434,6 +471,7 @@ public function sometimes(string $key, $rules, $callback) $rules = [$key => $rules]; $rules = $this->parse_rules($rules); $this->rules = array_merge($this->rules, $rules); + $this->reset_outcome(); } } } diff --git a/tests/Support/Validation/CountingMessagesBag.php b/tests/Support/Validation/CountingMessagesBag.php new file mode 100644 index 0000000..fc35879 --- /dev/null +++ b/tests/Support/Validation/CountingMessagesBag.php @@ -0,0 +1,17 @@ +lookups++; + + return parent::get($key, ...$args); + } +} diff --git a/tests/Unit/Supports/MessagesBagTest.php b/tests/Unit/Supports/MessagesBagTest.php new file mode 100644 index 0000000..e7a6a9a --- /dev/null +++ b/tests/Unit/Supports/MessagesBagTest.php @@ -0,0 +1,64 @@ +app_messages = $app_messages; + + parent::__construct(); + } + + protected function app_defined_messages() + { + return $this->app_messages; + } + }; + } + + public function test_overriding_one_rule_message_keeps_the_other_rule_defaults(): void + { + $bag = $this->bag_with(['validator' => ['required' => ['default' => 'Please fill in {name}.']]]); + + $this->assertSame('Please fill in {name}.', $bag->get('validator.required.default')); + $this->assertSame('The {name} field must be a string.', $bag->get('validator.string.default')); + $this->assertSame('Validation failed!', $bag->get('validator.failed')); + } + + public function test_overriding_one_constraint_message_keeps_its_siblings(): void + { + $bag = $this->bag_with(['validator' => ['string' => ['max' => 'Too long.']]]); + + $this->assertSame('Too long.', $bag->get('validator.string.max')); + $this->assertSame('The {name} field must be at least {min} characters long.', $bag->get('validator.string.min')); + } + + public function test_app_messages_can_add_new_keys(): void + { + $bag = $this->bag_with([ + 'validator' => ['string' => ['slug' => 'Not a slug.']], + 'shop' => ['out_of_stock' => '%s is out of stock.'], + ]); + + $this->assertSame('Not a slug.', $bag->get('validator.string.slug')); + $this->assertSame('Tee is out of stock.', $bag->get('shop.out_of_stock', 'Tee')); + $this->assertSame('You have to be logged in.', $bag->get('auth.logged_in_required')); + } + + public function test_without_app_messages_the_defaults_are_used(): void + { + $bag = $this->bag_with([]); + + $this->assertSame('The {name} field is required.', $bag->get('validator.required.default')); + } +} diff --git a/tests/Unit/Validation/CustomMessagesTest.php b/tests/Unit/Validation/CustomMessagesTest.php new file mode 100644 index 0000000..c8aa0e7 --- /dev/null +++ b/tests/Unit/Validation/CustomMessagesTest.php @@ -0,0 +1,119 @@ +passes(); + + return $validator->errors(); + } + + public function test_flat_custom_message_is_reported(): void + { + $errors = $this->errors_for(['name' => ''], ['name' => 'required'], ['name.required' => 'Flat']); + + $this->assertSame(['name' => ['Flat']], $errors); + } + + public function test_nested_custom_message_is_reported(): void + { + $errors = $this->errors_for(['name' => ''], ['name' => 'required'], ['name' => ['required' => 'Nested']]); + + $this->assertSame(['name' => ['Nested']], $errors); + } + + public function test_constraint_custom_message_replaces_placeholders(): void + { + $errors = $this->errors_for( + ['name' => 'abcdef'], + ['name' => 'string|max:3'], + ['name.max' => 'At most {max}'] + ); + + $this->assertSame(['name' => ['At most 3']], $errors); + } + + public function test_closure_custom_message_receives_name_and_value(): void + { + $errors = $this->errors_for( + ['name' => 5], + ['name' => 'string'], + ['name.string' => fn ($name, $value) => "Closure {$name}=" . $value] + ); + + $this->assertSame(['name' => ['Closure name=5']], $errors); + } + + public function test_custom_message_for_one_expanded_key_applies_to_that_key_only(): void + { + $errors = $this->errors_for( + ['v' => [['s' => 'abcdef'], ['s' => 'abcdefg']]], + ['v.*.s' => 'string|max:3'], + ['v.1.s.max' => 'Second only'] + ); + + $this->assertSame(['v.0.s' => [static::DEFAULT_MAX], 'v.1.s' => ['Second only']], $errors); + } + + public function test_custom_message_for_another_field_leaves_the_default(): void + { + $errors = $this->errors_for(['name' => 'abcdef'], ['name' => 'string|max:3'], ['other.max' => 'Other']); + + $this->assertSame(['name' => [static::DEFAULT_MAX]], $errors); + } + + public function test_shared_rule_object_does_not_keep_custom_messages_across_validators(): void + { + $rule = Rule::string()->max(3); + + $with_custom = $this->errors_for(['name' => 'abcdef'], ['name' => [$rule]], ['name.max' => 'Custom A']); + $without_custom = $this->errors_for(['name' => 'abcdef'], ['name' => [$rule]]); + + $this->assertSame(['name' => ['Custom A']], $with_custom); + $this->assertSame(['name' => [static::DEFAULT_MAX]], $without_custom); + } + + public function test_default_message_is_reported_without_custom_messages(): void + { + $errors = $this->errors_for(['name' => ''], ['name' => 'required']); + + $this->assertSame(['name' => ['The field is required.']], $errors); + } + + public function test_passing_payload_without_custom_messages_makes_no_lookups(): void + { + $bag = new CountingMessagesBag(); + app()->instance(MessagesBag::class, $bag); + + $variants = array_fill(0, 50, ['sku' => 'SKU-1', 'price' => 10, 'stock' => 3]); + + $validator = Validator::make( + ['variants' => $variants], + [ + 'variants' => 'required|array|min:1', + 'variants.*.sku' => 'required|string|max:50', + 'variants.*.price' => 'required|numeric|min:0', + 'variants.*.stock' => 'nullable|integer|min:0', + ] + ); + + $bag->lookups = 0; + + $this->assertTrue($validator->passes()); + $this->assertSame(0, $bag->lookups); + } +} diff --git a/tests/Unit/Validation/RuleParsingTest.php b/tests/Unit/Validation/RuleParsingTest.php new file mode 100644 index 0000000..f3dec77 --- /dev/null +++ b/tests/Unit/Validation/RuleParsingTest.php @@ -0,0 +1,138 @@ + [['price' => 1], ['price' => 2], ['price' => 3]]], + ['variants.*.price' => 'required|numeric'] + ); + + $this->assertSame( + ['variants.0.price', 'variants.1.price', 'variants.2.price'], + array_keys($validator->get_rules()) + ); + } + + public function test_wildcard_over_empty_or_missing_array_produces_no_keys(): void + { + $empty = Validator::make(['variants' => []], ['variants.*.price' => 'required|numeric']); + $missing = Validator::make([], ['variants.*.price' => 'required|numeric']); + + $this->assertSame([], $empty->get_rules()); + $this->assertSame([], $missing->get_rules()); + } + + public function test_string_rule_instances_are_equal_but_distinct_per_key(): void + { + $rules = Validator::make( + ['variants' => [['sku' => 'a'], ['sku' => 'b']]], + ['variants.*.sku' => 'required|string|max:50'] + )->get_rules(); + + $first = $rules['variants.0.sku']; + $second = $rules['variants.1.sku']; + + $this->assertCount(count($first), $second); + + foreach ($first as $index => $rule) { + $this->assertEquals($rule, $second[$index]); + $this->assertNotSame($rule, $second[$index]); + } + } + + public function test_caller_supplied_rule_object_is_distinct_per_key(): void + { + $rules = Validator::make( + ['variants' => [['sku' => 'a'], ['sku' => 'b']]], + ['variants.*.sku' => [Rule::string()]] + )->get_rules(); + + $first = $rules['variants.0.sku'][0]; + $second = $rules['variants.1.sku'][0]; + + $this->assertInstanceOf(StringRule::class, $first); + $this->assertInstanceOf(StringRule::class, $second); + $this->assertNotSame($first, $second); + + $first->max(2); + + $this->assertSame(2, $first->get('max')); + $this->assertNull($second->get('max')); + } + + public function test_closure_rule_runs_once_per_expanded_key_with_its_value(): void + { + $seen = []; + + $validator = Validator::make( + ['variants' => [['sku' => 'a'], ['sku' => 'b'], ['sku' => 'c']]], + [ + 'variants.*.sku' => [ + function ($value, $key) use (&$seen) { + $seen[$key] = $value; + + return true; + }, + ], + ] + ); + + $this->assertTrue($validator->passes()); + $this->assertSame( + ['variants.0.sku' => 'a', 'variants.1.sku' => 'b', 'variants.2.sku' => 'c'], + $seen + ); + } + + public function test_conditional_rule_branch_follows_each_validators_own_data(): void + { + $rules = [ + 'variants.*.sku' => [ + Rule::when(fn ($data) => $data['strict'], 'required|string|max:3', 'string'), + ], + ]; + $variants = [['sku' => 'abcd'], ['sku' => 'abcde']]; + + $strict = Validator::make(['strict' => true, 'variants' => $variants], $rules); + $lenient = Validator::make(['strict' => false, 'variants' => $variants], $rules); + + $this->assertTrue($strict->fails()); + $this->assertArrayHasKey('variants.0.sku', $strict->errors()); + $this->assertArrayHasKey('variants.1.sku', $strict->errors()); + $this->assertTrue($lenient->passes()); + } + + public function test_errors_are_reported_per_expanded_key(): void + { + $validator = Validator::make( + ['variants' => [['price' => 5], ['price' => -1], ['price' => 'abc']]], + ['variants.*.price' => 'required|numeric|min:0'] + ); + + $this->assertTrue($validator->fails()); + + $errors = $validator->errors(); + + $this->assertArrayNotHasKey('variants.0.price', $errors); + $this->assertNotEmpty($errors['variants.1.price']); + $this->assertNotEmpty($errors['variants.2.price']); + } + + public function test_plain_key_keeps_the_caller_supplied_rule_object(): void + { + $rule = Rule::string(); + + $rules = Validator::make(['name' => 'Widget'], ['name' => [$rule]])->get_rules(); + + $this->assertSame($rule, $rules['name'][0]); + } +} diff --git a/tests/Unit/Validation/ValidatorRunLifecycleTest.php b/tests/Unit/Validation/ValidatorRunLifecycleTest.php new file mode 100644 index 0000000..af0748e --- /dev/null +++ b/tests/Unit/Validation/ValidatorRunLifecycleTest.php @@ -0,0 +1,88 @@ +calls++; + + return true; + }; + } + + public function test_validate_runs_each_key_once(): void + { + $validator = Validator::make( + ['name' => 'Widget', 'sku' => 'W-1'], + ['name' => [$this->counting_rule()], 'sku' => [$this->counting_rule()]] + ); + + $this->assertSame(['name' => 'Widget', 'sku' => 'W-1'], $validator->validate()); + $this->assertSame(2, $this->calls); + } + + public function test_validated_after_passes_does_not_run_the_rules_again(): void + { + $validator = Validator::make(['name' => 'Widget'], ['name' => [$this->counting_rule()]]); + + $this->assertTrue($validator->passes()); + $this->assertSame(['name' => 'Widget'], $validator->validated()); + $this->assertSame(1, $this->calls); + } + + public function test_repeat_calls_do_not_duplicate_errors(): void + { + $validator = Validator::make(['name' => ''], ['name' => 'required']); + + $this->assertTrue($validator->fails()); + $after_fails = $validator->errors(); + + $this->assertFalse($validator->passes()); + $this->assertSame($after_fails, $validator->errors()); + $this->assertCount(1, $validator->errors()['name']); + } + + public function test_sometimes_adding_a_failing_rule_after_a_run_validates_again(): void + { + $validator = Validator::make(['name' => 'Widget'], ['name' => 'string']); + + $this->assertTrue($validator->passes()); + + $validator->sometimes('name', [fn () => 'Name is reserved.'], fn () => true); + + $this->assertFalse($validator->passes()); + $this->assertSame(['Name is reserved.'], $validator->errors()['name']); + } + + public function test_sometimes_with_a_false_callback_keeps_the_stored_outcome(): void + { + $validator = Validator::make(['name' => 'Widget'], ['name' => [$this->counting_rule()]]); + + $this->assertTrue($validator->passes()); + + $validator->sometimes('name', [fn () => 'Name is reserved.'], fn () => false); + + $this->assertTrue($validator->passes()); + $this->assertSame(1, $this->calls); + } + + public function test_set_value_after_a_failed_run_validates_again(): void + { + $validator = Validator::make(['name' => ''], ['name' => 'required']); + + $this->assertTrue($validator->fails()); + + $validator->set_value('name', 'Widget'); + + $this->assertTrue($validator->passes()); + $this->assertSame([], $validator->errors()); + } +}