GH-72: Drop the per-nested-object configuration round trip - #118
Merged
Conversation
Nested mapping rebuilt a JsonMapperConfiguration from the context for every nested object and handed it to map(), which wrote it straight back into that same context. A round trip that could only ever restore what was already there. It survives removal because the configuration was carrying almost nothing by the time it got there. Inside the mapper it was read exactly twice - isStrictMode() and shouldIgnoreUnknownProperties() - and the context answers both. So the parameter came out of mapSingleObject() and validateAndNormalize(), out of a closure's use list, and the nested strategy callback reduced to a single expression. The configuration remains the public entry-point type. It is translated into the context once, at the boundary, and the context is what travels. That is the cohesion the issue asks for, without touching the option bag - which has to stay a string-keyed extension point, since a type handler may put keys there that no configuration knows. OPTION_* constants untouched. Behaviour-neutral, and I checked rather than assumed: restoring the round trip on its own leaves the suite green. It is only harmless because #64 made the write a merge - restore the round trip AND the replace semantics it was written against, and four tests fail, including the new one that pins a custom option surviving three levels of nesting. The second new test pins the opposite direction: dropping the round trip must not drop the settings with it. It asserts on an unknown property rather than a missing one, because strict mode validates missing properties on the root object only - issue #105 - so a missing-property assertion would fail there for an unrelated reason.
Review found I had removed the write half and left the read. Once the two questions the mapper asks moved to the context, map()'s third branch rebuilt a configuration that nothing consumed - eight accessor calls and an allocation per nested object, discarded on the next line. That branch is the one every nested object lands in, so the acceptance criterion 'no per-nested-object round trip' was half met. Both surviving branches are live: one for a bare map(), one for mapWithReport(), which passes context and configuration together. fromContext() now has no caller in src/. It stays - it is published API, and a custom strategy receives a context and may want the settings in the shape the public surface speaks - but that is recorded as a decision at the method, since a reader finding no caller is entitled to ask. Its docblock and docs/API.md now also say what it does NOT carry: the mapper's own options only, so a key a type handler put in the bag does not survive the trip. Which is why the mapper stopped making it.
Review verified both new tests pass on the revision before this change, which is correct for a behaviour-preserving removal but makes their name misleading. They pin the property the round trip endangered, not the removal itself - so the docblock says so, and says what does make the first one fail: restoring the round trip together with the replace-instead-of-merge semantics it was written against. A test whose purpose is misread gets deleted in the next cleanup.
|
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
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.
Fixes #72.
Not the suggested fix, and why
The issue proposes that
MappingContexthold aJsonMapperConfigurationdirectly, sotoOptions()/fromContext()/replaceOptions()disappear. That is not expressible:CustomContextOptionTestpins a handler-owned key surviving nesting.OPTION_ABORT_ON_ERRORdeliberately lives only in the context (mapWithReport() plus strict configuration throws instead of returning a report #65): it distinguishes the two entry points rather than expressing a mapping preference, andmapWithReport()sets it alongsidetoOptions()output.So the two representations stay — and the round trip, which is the actual defect, goes. The configuration is translated into the context once at the boundary; the context is what travels.
What made that possible
Inside the mapper the configuration was read exactly twice —
isStrictMode()andshouldIgnoreUnknownProperties()— and the context answers both. So the parameter came out ofmapSingleObject()andvalidateAndNormalize(), out of a closure'suselist, and the nested strategy callback collapsed to a single expression.What review caught
I removed the write half and left the read.
map()'s third branch rebuilt a configuration from the context for callers supplying only a context — which is every nested object — and nothing consumed it. Eight accessor calls and an allocation per nested object, discarded on the next line. The acceptance criterion "no per-nested-object round trip" was half met.fromContext()now has no caller insrc/. It stays — published API, and a custom strategy receives a context and nothing else — but that is now a stated decision at the method, along with what it does not carry: the mapper's own options only, so a handler-owned key does not survive the trip. Which is why the mapper stopped making it.The new tests characterise rather than regress. They pass on the revision before this change, as they must for a behaviour-preserving removal. The docblock says so, and says what does make one fail: restoring the round trip together with the replace-instead-of-merge semantics it was written against.
Behaviour
Unchanged, and checked rather than assumed. Restoring the round trip alone leaves the suite green — it is harmless only because #64 made the write a merge. Restore both and four tests fail.
One observable improvement, verified by probe: a caller-supplied context no longer has the eight mapper keys materialised into its raw bag as a side effect of the first nested object. Every accessor reads identically; the bag is just no longer polluted. That matches the property
UnionResolutionTest::itDoesNotMaterialiseAnOptionTheCallerNeverSetalready pins elsewhere.Acceptance criteria
OPTION_*constants untouched.composer ci:testgreen, verified by exit code: 375 tests, 1216 assertions; PHPStan max, Rector, CGL and CPD clean.