feat(transport)!: collapse the transport error channel - #307
Merged
KaiSchwarz-cnic merged 6 commits intoAug 5, 2026
Conversation
HttpTransport::post() used to encode a failure twice: once as tuple
element [1], and once as a "httperror|" prefix smuggled onto the raw
payload [0], which AbstractResponseTranslator::translate() then
string-split back off. Replace the sentinel with an explicit, trailing
?string $error parameter on every hook in the pipeline
(TransportInterface::post()'s contract, AbstractClient::newResponse()
and both brand overrides, AbstractResponse::__construct()/translate()
and both brand overrides, AbstractResponseTranslator::translate()),
appended last and defaulted null so every positional call site keeps
working.
The two-level check is unchanged: $error !== null selects the
"httperror" template, $error !== "" gates the {HTTPERROR} injection.
TransportInterface::post()'s docblock now states the contract it
always implied: a non-null [1] means [0] is unusable.
Also removes the unreachable "nocurl" branch (curl_init() === false
cannot happen with ext-curl as a hard dependency) in favour of an
assert(), matching the file's existing idiom, and deletes the "nocurl"
template entry from both brand ResponseTemplateManagers.
BREAKING CHANGE: newResponse()/translate() on a brand Client/Response
gained a trailing ?string $error parameter, and TransportInterface::post()'s
contract now states that a non-null element [1] means element [0] is
discarded in favour of the "httperror" template — a custom transport that
returned bytes alongside an advisory error now has those bytes discarded.
The "nocurl" template id is also gone.
See [MIGRATION.md → v30.0.0](https://github.com/centralnicgroup-opensource/rtldev-middleware-php-sdk/blob/master/MIGRATION.md#-v3000)
Add the RSRMID-2937 entry: the transport error travels as a declared, trailing ?string $error parameter rather than a "httperror|" sentinel encoded into the payload; the raw-as-template-id lookup in resolveTemplateId() stays deliberately, as the sanctioned addTemplate() mocking route, and the method must genuinely be able to return null for PHPStan L9 to accept its ?string signature; the nocurl template id and its unreachable branch are gone; and why none of this needed a new *SeamTest.php guard (the parameter is compiler-enforced via LSP on abstract-method implementations, a naive sentinel re-introduction already fails the conn-error cassette test on both code and description, and a negative-space source sweep would be the vacuous guard shape this file warns against elsewhere) — with the one behavioural test that does pin the newly-stated TransportInterface contract instead.
The parser seam guard (RSRMID-2924) asserted the exact number of AbstractResponse::__construct() parameters. That count is not one of the properties the guard's own docblock claims to protect: it guards that a substitute parser is reachable through the public constructor by name and that injection never becomes mandatory. The latter is already covered by getNumberOfRequiredParameters() === 1. Appending ?string $error in RSRMID-2937 tripped the count and reported an undone decision where there was none, costing a detour and a guard edit that needed justifying. A tripwire whose message misleads is worse than no tripwire, so the assertion is removed and a comment records why it must not come back. Both substantive assertions are untouched.
…g key resolveTemplateId() returned the literal "httperror" whenever $error was non-null, and translate() then indexed $templates[$templateId] unconditionally. templates() is an abstract hook, so a third-party brand translator's container need not declare an "httperror" entry at all -- reachable, and a real regression from master, which only ever indexed a template after array_key_exists() succeeded and otherwise degraded quietly to the "invalid" template. A missing key now degrades an Undefined-array-key warning immediately followed by a TypeError (preg_replace() against a null $newraw). resolveTemplateId() now takes the caller's already-bound $templates snapshot as a parameter (removing a redundant static::templates() call and the snapshot inconsistency that came with it) and returns null -- falling through to the existing hasMissingRequiredFields()/"invalid" path, exactly as master did -- when $templates does not declare "httperror". No new failure mode: no throw, no fallback string. Add tests/AbstractResponseTranslatorFallbackTest.php: a purpose-built fixture translator subclass (not a mutation of a real brand's public static $templates bag, which is process-lifetime and shared across test classes) whose templates() omits "httperror", asserting the degradation lands on the "invalid" template rather than warning or throwing.
The RSRMID-2937 entry claimed a naive sentinel revert "already fails"
tests/CNR/ClientTest.php, stated as if that were sufficient on its own.
It only catches a PARTIAL revert (sentinel re-encoded without also
restoring the explode("|", ..., 2) split, so $raw matches no template
id and the test fails on both code and description). A COMPLETE,
consistent revert -- sentinel re-encoded, the split restored, ?string
$error dropped from all seven signatures together -- compiles, is
behaviour-preserving, and leaves the suite green. Rewrite the
justification to say so plainly and to accept that gap as an
out-of-scope risk (a deliberate, whole-hierarchy reversal of a
documented MIGRATION.md v30 BREAKING CHANGE, not the invisible
one-line drift guard tests exist to catch) rather than claiming it is
closed. The other two justifications (compiler-enforced parameter; a
negative-space sweep being the vacuous-guard shape) are unchanged --
both were verified sound.
Also updates the resolveTemplateId() entry for its new $templates
parameter and the missing-key guard from the companion fix(translator)
commit.
The v30 section's "Who is affected" listed only two groups (hook
overriders, TransportInterface implementers). The nocurl removal is a
third, and it is silent: CNR/IBS ResponseTemplateManager::getTemplate
("nocurl") now falls through to the generic "notfound" template
(CODE=500/"Response Template not found") instead of the removed
"nocurl" one (CODE=423/"API access error: curl_init failed"), and
hasTemplate("nocurl") flips from true to false -- nothing throws, the
value just changes. Add it under "Who is affected".
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #307 +/- ##
============================================
- Coverage 99.51% 99.51% -0.01%
- Complexity 448 450 +2
============================================
Files 32 32
Lines 1027 1025 -2
============================================
- Hits 1022 1020 -2
Misses 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
KaiSchwarz-cnic
deleted the
RSRMID-2937/collapse-transport-error-channel
branch
August 5, 2026 13:42
Collaborator
Author
|
🎉 This PR is included in version 30.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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.
Jira
RSRMID-2937 — Collapse the transport error channel — remove the
"httperror|"sentinelCandidate 1 of 8 from the architecture review of 2026-08-04. Design settled in a grilling session the same day; all ten decisions are recorded in the Jira description and mirrored in docs/agents/architecture.md.
Problem
TransportInterface::post()already returns the failure as tuple element[1], andAbstractClient::performRequest()already destructures it. ButnewResponse()took no error parameter, so the transport also smuggled the same failure through the raw-response channel as the magic prefix"httperror|", which the translator then string-split back off. The failure was encoded twice because one signature was missing a parameter.Change
?string $erroris now an explicit trailing parameter, appended last and defaultednull, on every hook in the pipeline:TransportInterface::post()'s tuple,AbstractClient::newResponse()and both brand overrides,AbstractResponse::__construct()/translate()and both brand overrides,AbstractResponseTranslator::translate(). Every existing positional call site keeps working; only overriders of the two abstract hooks break.TransportInterface::post()'s docblock now states the contract it always implied: a non-null[1]means[0]is unusable, andHttpTransportreturns["", $error]on failure rather than any payload.$error !== nullselects thehttperrortemplate,$error !== ""gates the{HTTPERROR}injection into it.AbstractResponseTranslator::resolveTemplateId(string $raw, ?string $error, array $templates): ?stringis the extracted decision point. It resolveshttperroronly if the brand's$templatescontainer declares that id, so a third-party brand translator need not include it. The raw-as-template-id lookup (theaddTemplate()mocking route CLAUDE.md sanctions) is kept exactly as-is — it's load-bearing, not a leak, and everynew R("<templateId>")in the suite depends on it.nocurlbranch is deleted:curl_init() === falsecannot happen withext-curlas a hard composer dependency. The guard becomes\assert($tmp !== false), matching the file's existing idiom; the"nocurl"entry is removed from both brandResponseTemplateManager::$templates.fix(translator): guard the httperror template lookup against a missing key) hardensresolveTemplateId()so an$errorwith no matching"httperror"entry degrades tonullinstead of aTypeErrorfrom an unconditional array-index.Tests
TransportSeamTest::testTransportErrorDiscardsParseableBytes()— the one genuinely new, previously-impossible-to-express contract: a transport double returning real, parseable bytes and a non-null error together must have its bytes discarded, not merged or preferred.new R("nocurl")tests are repurposed rather than deleted — they're the only direct coverage of the raw-as-template-id route, so the id is swapped for a surviving built-in.tests/CNR/ResponseTranslatorTest.php/tests/IBS/ResponseTranslatorTest.phpcases that provedexplode("|", ..., 2)survived a pipe inside the error message are rewritten (that hazard is gone, but "the message survives verbatim through the parameter" is still worth asserting).tests/CNR/ClientTest.php::testRequestCurlExecFail2— the behaviour-preservation proof, asserting bothgetCode() === 421and the exactgetDescription()string against the hand-authoredconn-errorcassette, unchanged and still passing.*SeamTest.phpguard, deliberately: the parameter is compiler-enforced (PHP performs LSP checking on abstract-method implementations, so a brand dropping?string $errorfrom only some ofnewResponse()/translate()is a fatal at declaration time), and a negative-space source sweep for the old sentinel string would be the vacuous-guard shape this repo's docs warn against. Full reasoning, including what a complete sentinel revert would and wouldn't be caught by, is in architecture.md.Compatibility — breaking, v30.0.0
Two breaking surfaces: the brand-facing
newResponse()/translate()hook signatures, andTransportInterface::post()'s newly-stated contract for third-party transport implementers. MIGRATION.md → v30.0.0 has both sub-sections plus the compatibility-table row.composer lintexit 0 ·composer test593 passed, 1 skipped.Note: this branch also carried RSRMID-2938 (single redaction module) earlier in the review's plan; that ticket ended up shipping separately as its own non-breaking release (v29.1.0, PR #306, already merged) rather than riding this branch. This PR is now rebased onto that merge and carries only the 2937 work.
🤖 Generated with Claude Code