GH-76: Decide the extension story — strategies internal, TypeHandlerInterface public#122
Merged
Conversation
…nterface public The issue asks whether the conversion strategies are a public extension point or internal. The docs already answered it and the code had not: TypeHandlerInterface is documented everywhere, ValueConversionStrategyInterface nowhere, and there is no public hook to register a strategy - ValueConverter is built internally and never exposed. So this is one documented contract plus an implementation detail that happened to be public. Resolved that way. Every Value\Strategy\* class, the trait, ValueConversionStrategyInterface and ValueConverter are now @internal, each pointing at TypeHandlerInterface via addTypeHandler() as the supported path. AGENTS.md and docs/API.md say the same. This makes the two supports() signatures a non-issue rather than drift to reconcile: the internal one carries a MappingContext the public handler contract does not, and they are meant to serve different layers, so no alignment - and no major-version break - is needed. The audit note on the issue records this is also what unblocks #87 in a patch rather than a major: if the chain order is not a contract, converging the entry points is a refactor. Two smaller items the audit added to scope: ClassResolver::add() accepted only a Closure while the constructor $classMap accepts class-string|Closure, so a static SdkFoo => Foo mapping could be given at construction but not at runtime without a trivial closure wrapper. add() now takes Closure|string and validates a class-string target at registration, as the constructor does. resolve() already handled both. register()/registerHandler() on CustomTypeRegistry are NOT the duplicate the issue supposed - register() is the callable convenience that wraps a ClosureTypeHandler and delegates to the handler primitive, a two-tier API. Left as is. And the AGENTS.md "supports/map" wording the audit flagged is already "supports()/convert()" at the point that matters; no fix needed.
…ening Three review findings. The @internal note in docs/API.md sat between a table's delimiter row and its body, which ends the table in GitHub-flavoured Markdown - the whole public Methods reference then rendered as raw piped text. Moved above the header. The note also said a handler is "consulted first / ahead of them". It is not: NullValueConversionStrategy runs before the custom-type strategy, so a handler never sees a null. Corrected in both docs/API.md and AGENTS.md to "ahead of the built-in deciding strategies, after null handling". And the ClassResolver::add() widening was dead code as committed: its only non-test caller, addCustomClassMapEntry(), still took Closure only, so the new string branch was reachable from a unit test alone. The architecture review was right to flag it. Rather than drop it, I completed the chain - addCustomClassMapEntry() now accepts Closure|string too, so a static SdkFoo => Foo mapping is registrable at runtime through the public API, and a test drives it that way. The add() docblock also notes an allowlist is inert beside a static target, since such a target has no payload-derived choice to constrain.
|
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.
Fixes #76.
The issue asks whether the value-conversion strategies are a public extension point or internal, and whether to align two
supports()signatures. The docs had already answered it and the code had not:TypeHandlerInterfaceis documented everywhere,ValueConversionStrategyInterfacenowhere, and there is no public hook to register a strategy —ValueConverteris built internally and never exposed.The decision
Every
Value\Strategy\*class, the trait,ValueConversionStrategyInterfaceandValueConverterare now@internal, each pointing atTypeHandlerInterfaceviaaddTypeHandler()as the supported path.AGENTS.mdanddocs/API.mdsay the same.This makes the two
supports()signatures a non-issue rather than drift to reconcile: the internal one carries aMappingContextthe public handler contract does not, and they serve different layers — no alignment, and no major-version break, needed. The audit on the issue records this is also what unblocks #87 in a patch rather than a major: if the chain order is not a contract, converging the two value-conversion entry points is a behaviour-preserving refactor.Scope additions from the audit
ClassResolver::add()accepted only aClosurewhile the constructor$classMapacceptsclass-string|Closure— a staticSdkFoo => Foomapping could be given at construction but not at runtime. Bothadd()and the publicaddCustomClassMapEntry()now acceptClosure|string, validated at registration like the constructor, with a test driving a static mapping through the public API.register()/registerHandler()onCustomTypeRegistryare not the duplicate the issue supposed —register()is the callable convenience that wraps aClosureTypeHandlerand delegates to the handler primitive. Left as is.What review caught
@internalnote indocs/API.mdwas placed between a table's delimiter and body, breaking the whole Methods table in GitHub Markdown. Moved above the header.NullValueConversionStrategyruns before the custom-type strategy, so a handler never sees a null. Corrected to "ahead of the built-in deciding strategies (after null handling)".add()widening was dead code as first committed (its only caller still tookClosure); completing the public entry point made it live.Verification
composer ci:testgreen, verified by exit code: 384 tests, 1233 assertions; PHPStan max, Rector, CGL and CPD clean.