GH-77: Close the test-coverage gaps, and the bugs writing the tests surfaced - #131
Merged
Conversation
…ot cover Pursuing the coverage gaps this issue tracks turned up a reachable escape. The write is the one step the conversion pipeline does not decide: it converts against the type the resolver could DERIVE, and that is not always the type the target declares. An intersection is modelled by neither PropertyInfo nor the reflection fallback, so it resolves to nullable mixed, which accepts every payload and leaves the property to refuse it. Unguarded, that refusal arrived as a native TypeError, wrapped by Symfony's property accessor into its own InvalidTypeException, and escaped mapWithReport() past the report the caller was promised - breaking both "never let a native error escape" and "a rejected value is recorded exactly once". The write is now guarded and records a TypeMismatchException at the property's path. The expected type is the one the PROPERTY declares, not the one conversion ran against: that one accepted the value by definition, so reporting it would say "expected mixed, got array" and explain nothing. The declaration is assembled from the reflected parts rather than cast to string, because ReflectionType::__toString() is marked deprecated in favour of asking the concrete subclass. One fixture exercises all three parts - a (MarkerA&MarkerB)|null property is a union of an intersection and a named type. The hydrate() catch enumerates the two write failures rather than widening to MappingException, so a future step before the write cannot have its failure reported against the property path. Verified: composer ci:test green (386 tests, 1242 assertions; PHPStan max, Rector, CGL, jscpd all clean).
The issue names two tests that claim more than they check, plus one that reaches its subject through a private field. All three are about the same thing: a test whose name is a claim has to fail when the claim stops holding. itRejectsResolversReturningNonStrings wrote the resolver straight into ClassResolver's private classMap, so it never exercised add() - and could therefore have passed on a map shape the resolver does not actually accept. It registers through add() now. A closure's declared return type is documentation to PHP rather than a runtime check, so the contract violation under test is exactly how a consumer reaches this guard; the analyser's objection to it is scoped in phpstan.neon alongside the existing fixture entries. itAllowsScalarToObjectCastingWhenConfigured asserted only that no error was recorded and that an instance of Base came back - which would hold just as well if the property had been skipped. What the option does is lift a refusal, not perform a cast: the mapper builds the target from the scalar, and the scalar contributes nothing. That is now asserted, together with the discriminator that the same payload without the option is refused. itMapsLargeDatasetsWithinReasonableResources asserted no resource bound at all. A wall-clock or memory assertion would be flaky, and the resource property worth pinning is structural - the class's mapping shape is derived once rather than per element - which ClassMetadataReuseTest already owns. Renamed to what it does pin: that scale changes nothing about order or contents. The property reads go through get_object_vars() where the fixture's docblock declares a property non-nullable: a direct read is narrowed to that type, and every is-it-set assertion then becomes one the analyser proves in advance. Also adds the legacy-path pin the issue asks for: addType() is deprecated but public and documented as the escape hatch that outranks the built-in strategies, so ClosureTypeHandlerTest pins it together with the handler it wires - which covers the handler's decline-and-refuse contract at the same time. Verified: composer ci:test green (390 tests, 1256 assertions).
…ne that does not exist Covering the entry point's class validation, which this issue lists as a P1 gap, showed the guard was asking the wrong question. By the time it ran, the class resolver had already refused every name that resolves to nothing, so its existence check could only ever fire for the one thing class_exists() answers false to and the resolver accepts: an interface. It reported that as "Class [X] does not exist", which is not true of an interface and not a hint at what to do instead. Three shapes passed it and raised a native Error at `new $className` instead: an abstract class, an enum, and a class with a private constructor. Those escape error collection entirely, and their message names neither the mapper nor the argument that was wrong. The guard now asks whether the class can be built. All four shapes are refused with an InvalidArgumentException that names the class and points at addCustomClassMapEntry(), which is how an interface or an abstract base is meant to be mapped - the class map is consulted first, so a resolved interface has already become a concrete class before the check runs. The refused name is echoed only when it IS the name the call passed. A class-map entry may derive it from the payload, and this exception escapes past the report into whatever generic handler the consumer wrote, so echoing a resolver's output there would put a payload-chosen string into a response body - the rule the resolver's own echoName flag already follows. docs/recipes/error-handling.md gains the four shapes and the echo rule. Verified: composer ci:test green (401 tests, 1279 assertions).
A guard that the chain makes unreachable is not the same as a dead one. The chain decides the question before the guard sees it - NullValueConversionStrategy claims every null, supports() declines a type before convert() could be handed it, and every call site into the collection factory rules out a null payload - so the guard behind each of those is decoration today and load-bearing the moment the chain is reordered. These drive them individually, so that a future "this branch is never hit" cleanup meets a red test rather than a silent contract loss. Covered by direct invocation: the builtin strategy's nullable-identifier and literal-null-type arms, the object strategy's two shape guards, the guard trait's hand-back for a type it can identify no target for, the passthrough strategy that makes the converter's own no-strategy-matched guard unreachable, and the collection factory's no-collection answer for a null payload plus its refusal of a collection type whose wrapper names no class. Covered through the mapper, because the chain does reach them: a date payload that describes no instant, a number where an interval specification is expected, an unknown timezone written straight into the option bag, and a payload that cannot name a backed enum case. The timezone case also pins that the identifier is NOT echoed as the actual type - that slot is documented as the detected type of the value, and an extension point can route request-influenced data into the timezone. The docblock-driven collection resolver gains its own tests: a container with a docblock but no element tag and one with no docblock at all are two distinct paths through the reader, the memo answers a repeated class without reading it again, and an element tag carrying no readable type is skipped rather than dereferenced. StrategyDirectInvocationTest's rationale is corrected: the strategies became internal rather than a public extension point, so the case those guards defend is the chain changing, not a consumer calling one directly. Verified: composer ci:test green (426 tests, 1337 assertions).
…arget shapes A configuration arrives from two directions and the mapper holds them to different standards. A wither is a call the developer wrote, so a timezone it cannot use is a defect to raise on. An array handed to fromArray() is restored state - a session, a cache entry, a config file - and refusing to restore it would turn one stale key into a run that cannot start. It is sanitised to the same defaults an absent key produces, and the option bag gets the same treatment because an extension point can write to it without passing the configuration at all. Both directions are now pinned, together with the discriminator that a plain offset survives where an identifier list would have replaced it with UTC. A PSR-6 pool may refuse a key it will not accept. Resolution is the job and caching the optimisation, so both the read and the write swallow it - pinned by the extractor answering twice rather than by the absence of a crash, which a silently-cached wrong answer would also satisfy. ReplaceNullWithDefaultValue is checked twice per property and the two checks answer different questions: a null PAYLOAD before conversion, and a null RESULT after it. Only a registered type handler can produce the second, since the built-in strategies report a value they cannot convert rather than answering null for it - which is why the second check had no test. Without it the property is written with the handler's null, contradicting both its declared type and the attribute. Five target shapes the common path never produces are covered as well: a traversable payload, integer keys where property names are expected, a property reachable only through its setter, a constructor tail that consumes "the rest", and a property declaring the null type itself. Two dead branches go. describeType() checked for the null type after the builtin branch had already answered it with the same string - the check could never run. The guard trait's two nullability branches cannot run either, because Symfony expresses a nullable object as a NullableType wrapping an ObjectType, so the type they ask never carries nullability itself; those stay, with the reason written down, since a Symfony release could make the question live again. getRootInput() had no consumer at all. It is the only way a handler can reach the document its value came from - the context it receives has moved on to a nested path - so docs/API.md now documents the handler-facing context surface rather than leaving the method to look like an accident. Verified: composer ci:test green (451 tests, 1382 assertions). Uncovered statements in src/ are down from 100 to 30.
…cannot run Strict mode asks whether a property HAD to be supplied, and that is a question about the declaration. An intersection has no null that satisfies it - every member is a class or interface, and null implements none of them - so a payload omitting one leaves the property uninitialised and reading it back raises. The required check answered "no" for it, treating it like a type with a usable absent value, which is the opposite of what strict mode exists to say. It now answers yes, with the nullable-union case beside it as the discriminator. The class resolver's empty-name guard gains its test. The empty string is what a lookup that found nothing produces - an unset environment variable, a missing config key - and without its own branch it would be reported as a class that does not exist, sending the reader looking for a typo in a name there is none of. What is left uncovered in src/ is 27 statements, and every one of them is now a guard whose comment says what would have to change for it to run: the collection instantiator's no-arguments arm and wrapCollection()'s null arm (every call site rules the case out first), the reflection lookups in the metadata factory (the resolver refuses a name that resolves to nothing, and the entry point refuses one that cannot be instantiated), the type resolver's unknown-builtin and all-null-union arms (two vocabularies that do not currently disagree), the docblock package check (a hard require), and the guard trait's nullability pair (Symfony wraps a nullable object rather than marking the object type). Line coverage of src/ is 1263 of 1290 statements. Verified: composer ci:test green (453 tests, 1385 assertions), exit code 0.
Covering the name converter's edge cases, which this issue lists, showed one of them producing a name nothing can be mapped onto. The inflector reads a key written entirely in upper case as one already-capitalised word, so ADDRESS_LINE became aDDRESSLINE - no PHP property is declared under that, so the key mapped to nothing at all, and in strict mode was reported as unknown. Such a key states every word boundary with a separator and says nothing with its case, so the case is now discarded before camelizing: ADDRESS_LINE_1 becomes addressLine1 and ID becomes id. The whole key has to be caseless in this way. A key containing any lower-case letter states boundaries by case as well, and flattening it would destroy them - HTTPServer stays hTTPServer rather than becoming httpserver, because where a word ends inside an acronym is not something the key says. That case is pinned too, so the limit is a decision rather than a gap. The converter's other listed cases are pinned alongside it: digits that stay part of their segment, pascal case, the degenerate keys a payload can carry anyway - empty, single-letter, leading, trailing and repeated separators, no ASCII letters at all - and idempotency, run over the same provider so that no case is exercised by only one of the two rules. The remaining P2 gaps needed no production change, only pinning. The unknown-key collector and strict mode answer the same question in opposite directions and the collector wins: a key it took is no longer unknown, and the collector property itself is not reported as missing. Both come with the discriminator that the same key on a class without a collector still is reported. ReplaceProperty is repeatable, so one property can be reached by several legacy names - which is what an API that renamed a field twice leaves its consumers with; every spelling arrives at the same property, none is reported as unknown, and a payload carrying two of them lets the later win, the way PHP resolves a repeated key. docs/recipes/custom-name-converter.md now states what the built-in converter does with each of these shapes. Verified: composer ci:test green (498 tests, 1435 assertions), exit code 0.
…g it was reviewed into
The coverage work landed four production changes, and a full reviewer pass -
including an independent Codex read - found each of them incomplete. This fixes
all four; the findings converged from several reviewers onto the same causes.
Entry-point instantiability check (P1 regression + a payload-echo leak). Asserting
instantiability at the point a class is RESOLVED broke polymorphic collection
mapping: a list mapped onto an abstract element class resolves the base once,
uses it only as the element TYPE, and lets the class map pick a concrete subclass
per element - the base is never instantiated, but the guard refused it. It is now
asserted where an object is actually built (the scalar and single-object lanes,
and the collection wrapper), never at resolution, so the abstract base passes
through. Regression pinned by PolymorphicCollectionTest.
The same guard echoed a payload-influenced class name. It decided whether to
name the refused class by string equality (resolved === requested), which is a
proxy for provenance that is wrong on the nested lane: there the mapper re-enters
with the resolver-produced class as its own argument, so the two are equal and
the payload-chosen name reached a message that escapes past the report. Provenance
is now carried structurally - the public map() treats its argument as caller-
supplied and may echo it; the nested re-entry routes through a private doMap()
with echo off. The instantiability fact is memoised in ClassResolver so a large
polymorphic list does not build a fresh ReflectionClass per element.
Write guard too narrow. The catch was InvalidTypeException only and sat around
the accessor call alone. A raw TypeError - from the variadic-setter call that
bypasses the accessor, or a setter body the accessor rethrows raw - still escaped.
The whole write, variadic branch included, is now inside the guard, and it catches
TypeError as well. The expected type for the common case is read from the
accessor's own InvalidTypeException rather than re-derived, which also fixes a
property exposed only through a setter reporting its refused type as "mixed"
(pinned by a new PropertyWriteFailureTest case).
Case folding was ASCII-only but applied to every byte. strtolower() lowers ASCII
only, so an all-uppercase non-ASCII key half-folded ("UEBER_MICH"-style names
mangled). The fold is now guarded to pure-ASCII all-uppercase names; a non-ASCII
SCREAMING key is passed through rather than mangled, since a correct fold would
need ext-mbstring the library does not require.
Two prose errors of my own: error-handling.md claimed an interface "exists as far
as class_exists() is concerned" (it does not; the resolver accepts it via
interface_exists()), and a code comment counted "three callers" where there are
fewer. Both corrected. Three stale "public SPI / DIRECT call" comments that
GH-76 retired when the strategies became internal are swept to the "the chain
changing" framing they now share.
Verified: composer ci:test green (504 tests, 1472 assertions), exit code 0.
Seven tests the review found could pass with their subject reverted, or asserted something weaker than their name. Each is now pinned by what actually distinguishes the behaviour. The memo test for a container that resolves to nothing could not observe the memo: a re-read also returns null, so array_key_exists() and isset() were indistinguishable to it. It now injects a counting docblock factory and resolves an annotated-but-element-less collection twice, asserting the factory parsed once - so a memo that stopped caching the null result fails it. The ReplaceProperty "last spelling wins" test listed the two aliases in the same order the fixture declares them, so a mapper driven by declaration order would pass it too. It now runs both payload orders, each expecting the payload's last value, which only payload order satisfies. The integer-key skip test used a lenient map() that discards the report, so a run that recorded an unknown-property error instead of skipping would leave the property null just the same. It now maps under a strict report and asserts no error - strict mode is where a routed-on integer key would surface. The date and enum "unusable value" providers each had two array rows - get_debug_type() 'array' for both - that exercised one branch with one outcome. Each pair collapses to a single array row, and the detected type is now a second provider column so the remaining rows pin distinct outcomes. The converter idempotency test took its fixed point from the unit's own output, so it held for any idempotent implementation including a broken one. It now asserts the fixed point named by the provider, and reaches it from the raw name too. RefusedCacheKey extended SPL's InvalidArgumentException as well as the PSR interface, so the cache-failure test passed whether TypeResolver caught the SPL class or the PSR one. It now extends the plain Exception, so only a catch on the PSR interface keeps it green. The write-guard control asserted a null result that is also the property's default, so a mapper that skipped the write entirely would satisfy it. The fixture gains an ordinarily-typed sibling property, written in the same run as the refused one, which proves the accessor is reached and that one refused property does not abort the object. Verified: composer ci:test green (503 tests, 1474 assertions), exit code 0.
The scoped ignore for CollectionFactoryTest ended at "expects", so it would have swallowed any future argument.type error on that method's first parameter, not just the empty-class-name case under test. It is now end-anchored on the GIVEN type - the ObjectType<mixed> shape the empty class name produces - so a genuinely wrong $type passed to the same method in that file is still reported. setProperty() gains the four @PARAM lines its docblock was missing while the diff was already editing it. The six new test methods that took a mixed parameter now declare the union the provider actually yields (int|string|null, int|string, and the array|scalar shapes), matching the house rule against mixed where a small union expresses the set. Two comments go: the inline note on wrapCollection()'s null arm restated the method's own docblock, and the constructor's collaborator list carried a four-line paragraph about an unreachable closure arm, now one line. The php-reviewer's "unqualified class_exists()" note resolved itself: the call moved into ClassResolver::isInstantiable() when the instantiability memo landed there, and that file already imports the function. Verified: composer ci:test green (503 tests, 1474 assertions), exit code 0.
A scratch copy left by the tooling that built the anchored ignore pattern; it is not referenced by anything and duplicates phpstan.neon.
A security review of the case-folding change noted that folding case and separators means id, ID and (with the default converter) user_id/userId all resolve to one property. Two spellings in one payload is a last-one-wins overwrite in payload order, and a key filter that runs before mapping never sees the alternative spellings. The custom-name-converter recipe now says so, and tells the reader to run any security-relevant key check on the converted name. Also states the ASCII-only scope of the SCREAMING_SNAKE fold the same section now documents. Docs only; no behaviour change.
The class-map example in JsonMapperTest is the canonical demonstration of a payload-driven resolver, and a security review noted it registered one without $allowedTargets. It returns only constants, so it is safe as written - but a reader copying its shape who later returns a payload-derived name would inherit no guard. Passing [VipPerson::class, Person::class] makes the example demonstrate the safe form at no cost. Test only.
Both were flagged independently by the adversarial reviewer and Codex, and the api-contract reviewer agreed on the second. Collection wrapper instantiation bypassed the guard. A collection-typed PROPERTY resolves its wrapper class inside CollectionFactory - a lane the entry-point check never sees - and instantiated it with a raw `new $className`. An abstract or interface wrapper (named by a docblock @var or a class-map entry) raised a native Error that no MappingException catch collects, so it escaped the report: the exact escape the entry-point guard exists to close, still open on this parallel lane (and present on main). The instantiability check now runs in the factory's instantiator closure - the single choke point that lane passes through - echoing nothing, since the wrapper name may be docblock- or resolver-derived. It is refused with the same catchable InvalidArgumentException the entry point raises. Closes #129. Pinned by a new EntryPointClassValidationTest case. The write guard's broad TypeError catch masked setter-body bugs. Catching every TypeError around the accessor write meant a TypeError raised INSIDE a consumer's setter body - a genuine bug - was re-labelled as a payload type mismatch: it blamed a possibly-valid value and, in report mode, continued silently and buried the real fault. The catch is now tight around the variadic-setter call alone (where a raw argument-type TypeError genuinely is a value mismatch, now named with the variadic PARAMETER's type rather than the backing property's), and the accessor path catches only the accessor's own InvalidTypeException. A setter-body TypeError propagates as the bug it is. AGENTS.md and docs/recipes/error-handling.md document both the write-guard failure modes and the deliberate non-masking of setter-body errors. Verified: composer ci:test green (504 tests, 1476 assertions), exit code 0.
The previous commit narrowed the accessor lane to catch only the accessor's own InvalidTypeException, so a TypeError from inside a consumer's setter body propagates as the bug it is rather than being re-labelled as a payload mismatch. The verification round found the variadic-setter lane still had a broad `catch (TypeError)` and so re-buried exactly that fault: a variadic setter whose elements bind fine but whose body delegates to a strict-typed call would have its body TypeError reported as `Type mismatch ... expected int, got array` and, in report mode, buried. The variadic catch now discriminates the same way Symfony's PropertyAccessor does: PHP attributes an argument-binding TypeError to the called method, so its innermost trace frame is the setter itself, while a body error names the inner call. Only an argument-binding refusal at the call boundary is converted to a recorded mismatch (named with the variadic parameter's type); a body TypeError is rethrown. Pinned by a fixture whose variadic setter binds valid ints and then raises a TypeError from a delegating call inside its body: the test asserts that TypeError propagates rather than being recorded. With the old broad catch it was recorded and no exception escaped, so the test fails against the pre-fix code. The intentional int-to-string call carries a scoped phpstan.neon ignore, like the other purpose-built fixtures. Verified: composer ci:test green (505 tests, 1478 assertions), exit code 0.
…ropagate
The previous commit tried to tell an argument-binding TypeError from a
setter-body one by the caught error's innermost trace frame. A second review
round - Codex and an adversarial pass with an end-to-end repro across PHP
8.3/8.4/8.5 - showed the trace frame cannot make that distinction: a body error
raised from the setter's own frame (a hand-built `throw new TypeError`, a
same-named delegate, or self-recursion) reports the setter as frame 0, exactly
like an argument-binding refusal, and so was masked - the very re-burying the
commit set out to prevent.
The heuristic is removed rather than patched. No `TypeError` is caught around the
variadic spread-call at all. The elements reaching it are already converted to
the resolved element type, so a genuine argument-binding refusal means the
property's docblock element type and the setter's parameter type disagree - a
DTO defect, not a payload problem - and it propagates loudly like the setter-body
TypeError the accessor lane also lets through. This makes the contract the docs
already state ("a setter-body TypeError propagates") unconditionally true, where
the heuristic delivered it only for a differently-named delegate.
Removing the classification also removes the reflected-type renderer it needed:
`describeReflectionType()` and its five now-unused imports are gone, and the
accessor lane keeps using the accessor's own `InvalidTypeException::expectedType`
- so the intersection and accessor-pair cases are unchanged.
The fixture now raises its TypeError from the setter's own frame - the shape the
heuristic masked - so the test fails against both the broad catch AND the
trace-frame version, pinning that the lane classifies nothing. Its scoped
phpstan ignore is gone with the int-to-string call it needed. AGENTS.md records
why a variadic TypeError must not be classified by trace frame.
Verified: composer ci:test green (505 tests, 1478 assertions), exit code 0.
Owner
Author
|
/gemini review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
1 similar comment
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Closes #77. The issue asked for tests covering the paths that hide correctness bugs — lenient error branches, union failure, converter edge cases. Writing those tests surfaced several genuine defects, so this branch is both the coverage work and the fixes it turned up. Statement coverage of
src/rose from 1190/1290 to 1263/1290, with every remaining uncovered line a guard whose comment states what would have to change for it to run.Bugs fixed (found by the coverage work and the review rounds)
mixed, accepts any payload, and then refuses it at the write as a nativeTypeErrorthat escapedmapWithReport(). The write is now guarded: the accessor'sInvalidTypeExceptionis caught and recorded as aTypeMismatchExceptionnaming the refused type (read from the accessor, so it is correct even for a property reachable only through a setter).Error. It now checks instantiability, at the point of instantiation rather than at resolution, so a polymorphic list mapped onto an abstract element class (the ordinary class-map case) is no longer wrongly refused.map()may echo its caller-supplied argument; the nested re-entry does not), so a resolver-produced name never reaches an escaping message.Error. It is now refused with a catchableInvalidArgumentException. (Closes Collection wrapper instantiation bypasses the instantiability guard (native Error escapes) #129.)ADDRESS_LINE_1camelised toaDDRESSLINE1, a name no property carries. An all-uppercase ASCII key is now folded first (addressLine1); a non-ASCII key is passed through rather than half-folded by a byte-onlystrtolower().Behaviour changes worth naming (see the release notes)
InvalidArgumentExceptioninstead of a nativeError(or, for an interface, a misleading "does not exist").CamelCasePropertyNameConverterreturnsaddressLine1/idforADDRESS_LINE_1/ID(wasaDDRESSLINE1/iD); a payload with such keys that previously mapped to nothing now populates the property.TypeErrorraised inside a consumer's setter body now propagates as itself rather than being caught — it is a bug in the setter, not a payload mismatch. Documented indocs/recipes/error-handling.md.No public signature changed;
map()/mapWithReport()are unchanged (a privatedoMap()carries the new provenance flag). These are bug-fix-shaped, so a minor release is appropriate.Test-quality fixes the issue named
ClassResolverTestno longer reflects into a private field — it registers throughadd().itAllowsScalarToObjectCastingWhenConfiguredand the large-dataset test now assert what their names promise; several new tests gained the discriminator they were missing (a control proving the pinned behaviour is not simply the default).Verification
composer ci:testgreen (505 tests, 1478 assertions; PHPStan level max, Rector, php-cs-fixer, jscpd — exit 0). The branch went through several full reviewer rounds (correctness, adversarial, security, maintainability, project-standards, api-contract, performance, plus a Codex adversarial pass on every round); the final state is confirmed clean by two consecutive passes. Findings out of this branch's scope were filed as #126, #127, #128, #130.Docs and AGENTS.md updated throughout.
🤖 Generated with Claude Code