v32.0.0: instance-scoped template registry, narrowed column and pagination seams - #312
Merged
Merged
Conversation
The response-template container was `public static array $templates`, redeclared per brand and read live by `AbstractResponseTranslator` at translate time, so `addTemplate()` in one test class changed response translation in every later one. `resetTemplates()` could not reliably undo that: it was a no-op unless `addTemplate()` had run first, and a direct write to the public property escaped it entirely. Making the property private was blocked while the translator read it across a class boundary. `ResponseTemplateManager` is now instantiable. Its templates are `private array` state seeded per instance from a per-brand `private const BUILTIN_TEMPLATES`, reached through the new `CNIC\ResponseTemplateManagerInterface`, and threaded `AbstractResponse::__construct()` -> brand `translate()` -> `AbstractResponseTranslator::translate()` as an optional trailing `$templates` argument. Every operation that was static is an instance method; `addTemplate()` returns `$this` rather than a throwaway `new static()`. `resetTemplates()` and its `$builtinTemplates` cache are deleted -- state that cannot escape its object has nothing to reset. The seam stops at `AbstractResponse`; `AbstractClient` is untouched. Every registration site in the repo feeds a direct Response construction, so a client-level registry would add public surface for a capability nothing asks for, and adding it later is purely additive. Also fixes the latent bug in the same surface: `matches()` indexed both hashes with no existence check, so an incomplete response hash emitted "Undefined array key" before comparing null. `ResponseTemplateManagerInterface` is a total contract and joins `InterfaceCoverageSeamTest::TOTAL_INTERFACES` -- both brand managers were previously invisible to that sweep. BREAKING CHANGE: the response-template registry is an instance, not `public static array $templates`; `resetTemplates()` is removed. Register on a `ResponseTemplateManager` instance and pass it as `new Response($id, templates: $registry)`. See [MIGRATION.md → v32.0.0](https://github.com/centralnicgroup-opensource/rtldev-middleware-php-sdk/blob/master/MIGRATION.md#-v3200)
.github/phpunit.xml displayed all six diagnostic categories and stopped on them via stopOnDefect="true", but failed on none. Those two settings combined badly: a PHP warning halted the run and nothing turned it into a non-zero exit, so CI reported success on a partial suite. Measured by reintroducing a single "Undefined array key" read into src/AbstractResponseTemplateManager.php: the suite ran 191 of 618 tests and exited 0. With failOnWarning it exits 1. stopOnDefect halts on errors, failures, warnings and risky results; failOnWarning/failOnNotice/failOnRisky now make every one of those exit non-zero, so a green run is a complete run. All three are free today — the suite raises none of them. failOnSkipped stays off: tests/Functional/HttpTransportTest.php skips legitimately when it cannot bind a port. failOnDeprecation stays off pending a check that the 8.4/8.5 CI legs are deprecation-free. Docs record the pairing and the one thing it must not be used for: a guard test whose subject is a PHP diagnostic still has to assert on it directly, because this config lives in a file an unrelated change can edit back out. Ref: RSRMID-2964
Two seams were drawn wider than the wire justified, and both narrowings are breaking, so they ship together in v32.0.0. `CNR\Column` bound `TValue` to `string` and narrowed `getDataByIndex(): ?string`, but `ColumnInterface` is not generic and every reachable path returns the interface -- `getColumn(): ?ColumnInterface` and `getColumns(): ColumnInterface[]`, both on `ResponseInterface`. The binding was erased at that boundary, and this project's own "type-hint against interfaces" rule guaranteed no consumer ever held the narrowed type. The repo proved the erasure twice: `examples/datetime.php` needed a hand-written `@var Column<string>`, and `CNR\Response::columnInt()` re-narrowed at runtime because the static type was gone. `CNR\Column` and `@template TValue` are deleted; `ColumnInterface::getStringByIndex()` and `RecordInterface::getStringByKey()` replace them -- one-line, opt-in, declared on both the class and its interface, the shape already set by the date accessors (RSRMID-2318/2926). Making `ColumnInterface` generic was rejected: the erasure is at `getColumn()`, so `ResponseInterface` would have to become generic too, pushing `ResponseInterface<string>` into every integrator's own type hints. The pagination seam pinned 7 methods as brand primitives, but three of them -- `getCurrentPageNumber()`, `hasNextPage()`, `hasPreviousPage()` -- read no wire column at all. They were pure functions of the four column readers, exactly like the derived getters already on the base, so pinning them protected nothing and pinned a misclassification: CNR's hand-written predicates computed from whole page numbers while their sibling getters computed from record offsets. CNR's list API is offset-based (FIRST/LAST/LIMIT/TOTAL), so the two paths agree only when FIRST is a multiple of LIMIT. On an unaligned window `hasNextPage()` announced a next page for a window already holding the tail of the list (an avoidable empty request), and `hasPreviousPage()` answered false for a window with fifty preceding rows. All three moved to `AbstractResponse`, and every predicate and page number now derives from the same offset grid, so a predicate and its getter cannot disagree. `PRIMITIVES` is now exactly the four wire columns; `requestNextResponsePage()` advances from the response's own `LAST + 1` instead of re-deriving from the command's FIRST. `getRecordsTotalCount()`/`getRecordsLimitation()` become `?int` and drop their `getRecordsCount()` fallback, so "no LIMIT column" and "LIMIT=0" stop colliding. Two gates guard the offset arithmetic against an empty window, which CNR answers by echoing `LAST = FIRST`: the pre-existing `LIMIT <= 0` guard (still load-bearing -- with LIMIT=0 and a small FIRST, `LAST + 1 < TOTAL` holds and the walk would restart at offset 1), and a defensive `LAST < FIRST` floor pinning the monotonic-advance invariant. Its test states outright that no observed response produces that shape. `getRecordsCount()` is not usable as a gate: `assembleRecords()` sizes the record list across every column, pagination ones included, so a response carrying only COLUMN/COUNT/FIRST/LAST/LIMIT/TOTAL still reports one record. `RecordColumnSeamTest` widens from "only CNR declares a Column" to "no brand declares one" plus a check that both interfaces declare the narrowing accessors; `ResponsePaginationSeamTest` narrows to 4 primitives and 8 derived getters, with its docblock rewritten to state the directive, the failure mode, why the guard must be structural, and the one condition that would justify revisiting it -- a brand whose "more results" signal is a cursor rather than a record offset. BREAKING CHANGE: `CNR\Column` is deleted -- use `CNIC\Column` and read values through `ColumnInterface::getStringByIndex()` / `RecordInterface::getStringByKey()`, adding both methods if you implement either interface. `ResponseInterface::getRecordsTotalCount()` and `getRecordsLimitation()` return `?int`, so an implementation must widen its return types and a consumer must handle `null` on a non-list response; `getPagination()["TOTAL"]`/`["LIMIT"]`/`["CURRENTPAGE"]` are `null` there too. A brand `Response` no longer declares `getCurrentPageNumber()`, `hasNextPage()` or `hasPreviousPage()`, and `hasPreviousPage()` now answers `FIRST > 0`, so it reports true on an unaligned window where it used to report false. See [MIGRATION.md → v32.0.0](https://github.com/centralnicgroup-opensource/rtldev-middleware-php-sdk/blob/master/MIGRATION.md#-v3200)
socketTimeout = 300 was declared on AbstractSocketConfig and re-declared identically on both CNR\SocketConfig and IBS\SocketConfig — three copies of one number, with no per-brand intent behind either override. Both brand lines sat inline among the genuinely brand-specific $oteUrl/$liveUrl, which is likely how they were copied in. Behaviour-identical: the base default is unchanged and both brands inherit it. Refs RSRMID-2944
MANAGED_OPTIONS maps CURLOPT_USERAGENT to the bare string "setUserAgent()", but that setter lives on AbstractClient — the config holds no user-agent state at all. A caller holding only the config, via getSocketConfig(), was told to "use the setter, which is the single home for that value" and pointed at a method it cannot reach. Each entry now carries the class that owns its setter and the message renders it, so the rejection reads CURLOPT_USERAGENT (use CNIC\AbstractClient::setUserAgent()) The key list is deliberately untouched: dropping CURLOPT_USERAGENT from it would re-open the second home the constant exists to close, letting the bag carry a value the getter cannot see. The guard test previously accepted a setter existing on *either* the config or the client, which is what let the unreachable name pass. It now checks the setter against the class the entry names as owner, and a second test asserts the thrown message actually carries that class. Both were proven non-vacuous against the mutation they refuse; the old || form passed it. Refs RSRMID-2944
The AbstractClient class docblock and architecture.md both said "~26" forwarders to AbstractSocketConfig. The actual count is 18 — 8 getters, 9 setters and setCredentials(), which composes two — so the docs overstated the forwarded surface by roughly 40%. CNR\Client adds none: its session methods read CNR's own state, not the config. Refs RSRMID-2944
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #312 +/- ##
============================================
- Coverage 99.50% 99.31% -0.20%
- Complexity 440 459 +19
============================================
Files 32 31 -1
Lines 1012 1018 +6
============================================
+ Hits 1007 1011 +4
- Misses 5 7 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…-owner decision CLAUDE.md still described `CNR\Column extends CNIC\Column<string>` as the one brand Column and justified the no-`newColumn()`-factory directive with the PHPStan/Psalm infeasibility argument. RSRMID-2942 deleted that class, so the first sentence was false and the second quoted a premise architecture.md had already flagged as spent. The directive stands — nobody re-tested it — but it now says so instead of citing dead evidence. Records RSRMID-2944's MANAGED_OPTIONS change as a decision rather than a tidy-up: the `owner` key and the guard that checks against it are what stop the entry naming an unreachable method again, and both read as removable without it. Also the last `~26` forwarder reference, in the ClientConfigSeamTest docblock.
…g added Codecov flagged both as uncovered patch lines. They are not the same kind of gate, and the tests say so rather than papering over the difference. CNR\Client::requestNextResponsePage()'s re-check of LIMIT/LAST after hasNextPage() returned true reads like dead code — since 2943 the predicate and the offsets derive from the same readers, so a true answer already implies both. It is what stops that derivation being re-split: a Response subclass whose hasNextPage() answers from something else can say "yes" over a window with no LIMIT, and without the guard the next lines advance from null and re-list from the top. Proven by mutation: deleting it makes the new test drive a real request. AbstractResponse::hasNextPage()'s three-way null check is analyser narrowing, not a behavioural guard, and the test comment now records why instead of leaving the next reader to re-derive it. On CNR only TOTAL can be null there — a positive LIMIT implies a record, and the FIRST/LAST readers fall back to record-derived values rather than null — and that clause is inert too, since PHP coerces the null and the comparison is false either way. What the test pins is the ?int contract itself: an absent pagination column reads as null, not as 0.
Collaborator
Author
|
🎉 This PR is included in version 32.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.
Bundles the breaking work for v32.0.0 with two non-breaking cleanups from the same architecture review, so integrators absorb one major instead of three.
Jira
CNR\Columngenerics mismatch — replace it with native?stringaccessorshasNextPage()/hasPreviousPage()are derived, not brand primitivesBreaking (v32.0.0)
The response-template registry is an instance.
public static array $templateshad process lifetime and the translator read it live, soaddTemplate()in one test class changed translation in every later one andresetTemplates()could not reliably undo it. The registry is now passed to the translator; built-ins are immutable, overrides are scoped to the object that received them.resetTemplates()is gone.CNR\Columnis deleted. It boundTValuetostring, butColumnInterfaceis not generic and every reachable path returns the interface, so the binding was erased at the boundary and this project's own "type-hint against interfaces" rule guaranteed no consumer ever held the narrowed type.ColumnInterface::getStringByIndex()andRecordInterface::getStringByKey()replace it.Three pagination methods stopped being brand primitives.
getCurrentPageNumber()/hasNextPage()/hasPreviousPage()read no wire column — they were pure functions of the four column readers, and pinning them protected nothing while preserving a real disagreement: CNR's hand-written predicates computed from whole page numbers, their sibling getters from record offsets, which agree only whenFIRSTis page-aligned.MIGRATION.md → v32.0.0 carries the before→after for each, plus the compatibility-table row.
Non-breaking, folded in
Config-forwarder tidy-up (RSRMID-2944). Three commits, none breaking:
socketTimeout = 300was declared three times — base plus both brands, byte-identical with no per-brand intent. Collapsed to the base.MANAGED_OPTIONSmappedCURLOPT_USERAGENTto a bare"setUserAgent()", but that setter lives onAbstractClient, so a caller holding only the config was pointed at a method it could not reach. Entries now carry the owning class and the message renders it:CURLOPT_USERAGENT (use CNIC\AbstractClient::setUserAgent()). The key list is deliberately unchanged — dropping the option would re-open the second home the constant exists to close.AbstractClientdocblock andarchitecture.mdsaid "~26"; the real number is 18.Stricter test harness (RSRMID-2964). PHPUnit now fails the run on warnings, notices and risky results.
Guard tests
AbstractClientCurlOptionsTestpreviously accepted a managed-option setter existing on either the config or the client — that||is what let the unreachable method name pass review. It now checks the setter against the class the entry names as owner, plus a new test asserting the thrown message carries that class. Both were proven non-vacuous against the mutation they refuse; the old form passed it.Verification
composer testandcomposer lintboth green — 628 passed, 1 pre-existing skip (CNR login-lockout).