GH-75: Document and cover the defensive guards, rather than delete them - #121
Merged
Conversation
… four The issue lists five "dead defensive code paths" to remove. Measured, none is dead - each is a guard that is unreachable through the value converter's CHAIN but live for another entry, and removing them would drop real protection: - The two null guards (builtin, object trait) throw for a null on a non-nullable target. Through the chain NullValueConversionStrategy claims every null first, so they never run there - but the strategy classes are public SPI, and a direct convert() call reaches them. Proven: a direct builtin convert(null, int) throws TypeMismatchException, as it should. - The convertObjectValue non-object branch hands the value back for a type its own supports() would reject - reached only by a direct call skipping supports(). Proven the same way. - The ValueConverter LogicException is unreachable while the passthrough strategy (supports() always true, last) is registered, and guards the invariant if that ever changes - otherwise the method would fall off the end returning null. - The "collection class must be provided" throw is not a runtime guard at all but a load-bearing assertion: it narrows ?string to string for the calls below, and PHPStan max fails without it. Removing it, verified, breaks static analysis. So the resolution is the acceptance criterion's "documented reason", not removal: each branch now says why it cannot occur on the normal path and why it is kept. The four SPI/invariant guards also gain tests that drive them directly, turning "documented dead branch" into "live, covered branch" - the criterion's preferred outcome for the null guards. The architecture audit on the issue asked that the null guards not be deleted before the two-entry-points fix (#87, still open), since a single entry point may make them live through the chain too. Documenting and covering rather than deleting respects that: nothing here has to be revisited when #87 lands, and if #87 makes a guard live on the chain, its test simply gains a second caller.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
…n the message Review found my commit message claimed to cover four guards but only three tests existed - and the missing one was exactly the object-trait null guard. Removing its throw made convertObjectValue return null for a non-nullable object target with the whole suite still green, the precise regression this was meant to prevent. Added theObjectGuardRefusesANull..., verified by mutation: neutralising the throw now fails it. Dropped theObjectGuardHandsBackAValueForANonObjectType. It asserted the exact silent-passthrough the object trait does for a type it does not support - but the SPI interface promises nothing about convert() on an unsupported type, so the test froze a misuse-only implementation detail as contract, and a future fail-fast refactor would break it for no real reason. The branch keeps its documented reason; it is defensive against a protocol violation, which the acceptance criterion covers by documentation rather than a test. And the builtin guard test asserted only the exception type, which several sites in that strategy throw. It now matches the message, so it pins this guard rather than any TypeMismatchException.
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.
Fixes #75.
The issue lists five "dead defensive code paths" to remove. Measured against HEAD, none is dead — each is a guard unreachable through the value converter's chain but live for another entry, and removing them would drop real protection.
NullValueConversionStrategyclaims every null firstconvert()— the strategies are public SPIconvertObjectValuenon-object branchsupports()returns false for the typesupports()ValueConverterLogicExceptionsupports()always true, last) always matchesextractCollectionType()returns aTypeonly when the class is set?string→string; PHPStan max fails without itEach verified, not assumed:
BuiltinValueConversionStrategy::convert(null, int)throwsTypeMismatchException.DateTimeValueConversionStrategy::convert(BuiltinType, 'x')returns'x'(the non-object branch).Resolution: document, and cover — not remove
The acceptance criterion allows a documented reason for an unreachable defensive branch, and prefers null guards to be "live (covered by a test)". So each branch now states why it cannot occur on the normal path and why it stays; the four SPI/invariant guards also gain tests that drive them directly, turning "documented dead branch" into "live, covered branch".
Coordinated with #87
The architecture audit on the issue asked that the null guards not be deleted before the two-entry-points fix (#87, still open) — a single entry point may make them live through the chain too. Documenting-and-covering respects that: nothing here needs revisiting when #87 lands, and if #87 makes a guard live on the chain, its test simply gains a second caller.
Verification
composer ci:testgreen, verified by exit code: 382 tests, 1228 assertions; PHPStan max, Rector, CGL and CPD clean.