Coerce string query/route scalars against constructor scalar types#29
Conversation
HTTP query strings deliver every value as a string, so any list command typed `int $page` / `int $limit` / `float`/ `bool` returned 400 Bad Request when the client sent `?page=1&limit=25` — Symfony's ObjectNormalizer refuses to coerce `"1"` to `int 1` and the bundle surfaces a NotNormalizableValueException as a problem+json response. CommandValueResolver now reflects on the target class's constructor parameters and applies `filter_var(..., FILTER_NULL_ON_FAILURE)` to string payload values whose corresponding parameter is typed `int`/`float`/`bool` (ReflectionNamedType only — unions and intersections fall through untouched, preserving the existing `int|string` workaround pattern). On invalid input (`?page=foo`), the value is left as a string so the denormalizer surfaces its precise "expected int, got string" error rather than silently casting to 0 — keeping the schema-driven strict-validation promise of the bundle. JSON body values pass through untouched via the `is_string($value)` guard — JSON has real types and doesn't need coercion. Coverage: eight data-provider scenarios (int/float/bool happy paths, invalid-int leaves string, string-typed param untouched, union-typed skip, missing key, route-attribute coercion) plus one test for the no-constructor edge case (stdClass). Behavior fix only — no BC break, since GET pagination DTOs simply did not work before this commit.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR enhances ChangesScalar type coercion for query parameters
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Unit/Controller/ArgumentResolver/CommandValueResolverTest.php (1)
248-294: ⚡ Quick winAdd test cases for zero and negative coercion to guard the strict comparison.
The implementation correctly uses
!== nullto handle falsy coerced values (0,0.0,false), but the test suite doesn't verify'0'→0(int) or'0.0'→0.0(float). These edge cases would catch regressions if someone refactors the null check.🧪 Suggested additional test cases
yield 'string "false" coerces to bool false for typed bool parameter' => [ new Request(query: ['archived' => 'false']), ['archived' => false], ]; + yield 'string "0" coerces to int 0 for typed int parameter' => [ + new Request(query: ['page' => '0']), + ['page' => 0], + ]; + + yield 'string "0.0" coerces to float 0.0 for typed float parameter' => [ + new Request(query: ['ratio' => '0.0']), + ['ratio' => 0.0], + ]; + + yield 'negative int string coerces to negative int' => [ + new Request(query: ['page' => '-5']), + ['page' => -5], + ]; + yield 'invalid int string stays as string so validator surfaces its error' => [ new Request(query: ['page' => 'not-a-number']), ['page' => 'not-a-number'], ];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/Controller/ArgumentResolver/CommandValueResolverTest.php` around lines 248 - 294, Add explicit test cases in provideQueryParameterCoercionCases to cover zero and negative coercion edge cases so the strict !== null checks are exercised: add yields that pass Request(query: ['page' => '0']) expecting ['page' => 0], Request(query: ['ratio' => '0.0']) expecting ['ratio' => 0.0], and a negative case like Request(query: ['page' => '-1']) expecting ['page' => -1]; update any equivalent route-attribute cases (Request(attributes: [...])) as well so CommandValueResolverTest verifies both query and attribute coercion for 0/negative values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/Unit/Controller/ArgumentResolver/CommandValueResolverTest.php`:
- Around line 248-294: Add explicit test cases in
provideQueryParameterCoercionCases to cover zero and negative coercion edge
cases so the strict !== null checks are exercised: add yields that pass
Request(query: ['page' => '0']) expecting ['page' => 0], Request(query: ['ratio'
=> '0.0']) expecting ['ratio' => 0.0], and a negative case like Request(query:
['page' => '-1']) expecting ['page' => -1]; update any equivalent
route-attribute cases (Request(attributes: [...])) as well so
CommandValueResolverTest verifies both query and attribute coercion for
0/negative values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d290d30c-8ec3-4f80-92c6-02f7fc374200
📒 Files selected for processing (2)
src/Controller/ArgumentResolver/CommandValueResolver.phptests/Unit/Controller/ArgumentResolver/CommandValueResolverTest.php
Docblock on coerceScalarsAgainstConstructor was a six-line paragraph re-stating what the method name and implementation already convey. Reduced to two sentences capturing the non-obvious why (HTTP query parameters always arrive as strings; invalid input deliberately stays as a string). Added five test cases to provideQueryParameterCoercionCases to lock down the strict !== null guard against future regressions to a truthy check: ?page=0 → int 0, ?ratio=0.0 → float 0.0, ?page=-1 → int -1, plus route-attribute equivalents for 0 and -3. These would all silently fall through if the coerced-value check were ever changed to a truthy test (PHP int 0 is falsy).
1 participant
Summary
CommandValueResolvernow coerces string scalars in the merged route+query+body payload to the typed scalar (int/float/bool) declared on the target command's constructor parameter — before delegating to the Symfony Serializer'sdenormalize(). Fixes the long-standing 400 Bad Request on any list endpoint typedint $page/int $limitwhen the client sends?page=1&limit=25(which all HTTP clients do — query strings are always strings on the wire).Why
The bundle's identity is schema-driven request validation (per
composer.jsondescription and the README's "Two-Layer Validation" feature). The previous behavior —?page=1400ing against a typedint $pagebecauseObjectNormalizerwon't cast — silently broke the canonical OpenAPI pagination shape. Consumers were working around it withint|string $pageunion types, which is hand-rolled boilerplate the bundle exists to eliminate.Alternative considered and rejected:
AbstractNormalizer::DISABLE_TYPE_ENFORCEMENTcontext. One-liner, but lossy —?page=foowould silently cast toint 0via PHP's(int)cast, and validation messages would reference the post-cast value rather than the original input. That contradicts the bundle's strict-validation promise. The reflection-based approach (this PR) leaves invalid input as a string so the denormalizer'sNotNormalizableValueExceptionsurfaces the precise type problem.How
coerceScalarsAgainstConstructor()reflects on the target class's constructor:is_string()and whose type is aReflectionNamedTypeofint/float/bool, appliesfilter_var(..., FILTER_VALIDATE_*, FILTER_NULL_ON_FAILURE).int|string) and intersection types fall through to the existing behavior —ReflectionNamedTypeguard makes them no-ops, so the consumer's existing workaround pattern stays valid.stdClass) all short-circuit cleanly.This is the same algorithm Symfony's
MapQueryParameterattribute uses internally, lifted to the whole DTO scope.JSON request bodies pass through untouched (the
is_string($value)guard catches that — JSON has real types).Test plan
composer check— cs-fixer + PHPStan clean (0 errors over 96 files).composer test— full suite (191 tests, 576 assertions) passes including the new data-provider cases.CommandValueResolverTestcoverage: int/float/bool happy paths, invalid input stays as string, string-typed parameter untouched, union-typed parameter skipped, missing key handled, route-attribute coercion path, no-constructor (stdClass) edge case.composer update stixx/openapi-command-bundle. Will verify before tagging 0.8.1.Compatibility
Behavior fix only — no BC break.
?page=1against anint $pagepreviously returned400 Bad Request; no consumer can have relied on that. Existingint|stringunion workarounds in consumer apps continue to work (union types skip the coercion branch by design).