From 664f0c56b6171147ab387af9fdcf60873b97683f Mon Sep 17 00:00:00 2001 From: Kai Schwarz Date: Mon, 10 Aug 2026 10:13:20 +0200 Subject: [PATCH 1/9] feat(response): scope the template registry to an instance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .claude/agents/implementer.md | 2 +- CLAUDE.md | 6 +- MIGRATION.md | 156 +++++++++++-- docs/agents/architecture.md | 15 +- docs/agents/testing.md | 13 +- src/AbstractResponse.php | 15 +- src/AbstractResponseTemplateManager.php | 154 ++++++------ src/AbstractResponseTranslator.php | 55 +++-- src/CNR/Response.php | 13 +- src/CNR/ResponseTemplateManager.php | 34 ++- src/CNR/ResponseTranslator.php | 8 +- src/IBS/Response.php | 13 +- src/IBS/ResponseTemplateManager.php | 34 ++- src/IBS/ResponseTranslator.php | 8 +- src/ResponseTemplateManagerInterface.php | 106 +++++++++ ...AbstractResponseTranslatorFallbackTest.php | 94 ++++++-- tests/CNR/ClientTest.php | 16 +- tests/CNR/ResponseTemplateManagerTest.php | 50 ++-- tests/CNR/ResponseTest.php | 77 +++--- tests/CNR/ResponseTranslatorTest.php | 4 +- tests/IBS/ResponseTemplateManagerTest.php | 87 ++++--- tests/IBS/ResponseTest.php | 7 +- tests/InterfaceCoverageSeamTest.php | 17 +- tests/MONIKER/ResponseTest.php | 39 ++-- tests/ResponseParserSeamTest.php | 49 +--- tests/ResponseTemplateRegistrySeamTest.php | 220 ++++++++++++++++++ 26 files changed, 935 insertions(+), 357 deletions(-) create mode 100644 src/ResponseTemplateManagerInterface.php create mode 100644 tests/ResponseTemplateRegistrySeamTest.php diff --git a/.claude/agents/implementer.md b/.claude/agents/implementer.md index 33ab9d8f..1bb2403c 100644 --- a/.claude/agents/implementer.md +++ b/.claude/agents/implementer.md @@ -13,7 +13,7 @@ Project rules live in `CLAUDE.md`; read it. The traps that matter most here: - **Guard tests are load-bearing.** If a `tests/*SeamTest.php`, `tests/ResponseInterfaceConsumerTest.php`, `tests/AbstractClientConfigDriftTest.php`, `tests/HttpTransportCurlOptionsTest.php` or `tests/Functional/HttpTransportTest.php` starts failing, you are undoing a settled decision, not fixing a stale test. Stop and report it. Never delete or weaken one as a cleanup. - **Exceptions** come from the `CNIC\Exception` hierarchy. Never a bare `\Exception`. - Every new or modified file carries `declare(strict_types=1);`, typed properties, and return types. -- Do not add dependencies. Do not add mocking frameworks — use `ResponseTemplateManager::addTemplate()` or the existing spies. +- Do not add dependencies. Do not add mocking frameworks — register canned responses on a `ResponseTemplateManager` **instance** and pass it in (`new Response($id, templates: (new RTM())->addTemplate(…))`), or use the existing spies. The static `RTM::addTemplate()` form is gone (RSRMID-2941); do not reintroduce a static template container. - `MIGRATION.md` and `docs/agents/architecture.md` are only touched for a genuine `BREAKING CHANGE:`, which is a main-thread decision, not yours. Before reporting done, run `composer lint` and `composer test` and let the results stand. Note `.github/phpunit.xml` sets `stopOnDefect="true"` — a green run can mean the suite stopped early, so check how many tests actually executed. diff --git a/CLAUDE.md b/CLAUDE.md index 514b5b0d..d816aae3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -17,7 +17,7 @@ Facts below; the class inventory is derivable from `src/` and the **full deep di - **The `request()` lifecycle is a template method too** (`AbstractClient::performRequest()`), and public `request(array $cmd = [], string $path = "")` is symmetric across brands. Vary a brand only through `buildCommand()`/`newResponse()`/`newSocketConfig()`. - **Config-driven:** each `SocketConfig` (extends `AbstractSocketConfig`) carries endpoints/params/flags as typed properties (no `config.json`). - **Connection configuration lives on the `SocketConfig`, never on the client** — reach it via `AbstractClient::getSocketConfig()` (covariant `CNR\Client::getSocketConfig(): CNR\SocketConfig` is the one narrowing point). -- **Type-hint against interfaces:** `ColumnInterface`, `RecordInterface`, `ResponseInterface`, `ExtendedResponseInterface`, `RoleCredentialsInterface`, `ResponseParserInterface`, `TransportInterface`, `LoggerInterface`, `LogSinkInterface`. +- **Type-hint against interfaces:** `ColumnInterface`, `RecordInterface`, `ResponseInterface`, `ExtendedResponseInterface`, `RoleCredentialsInterface`, `ResponseParserInterface`, `ResponseTemplateManagerInterface`, `TransportInterface`, `LoggerInterface`, `LogSinkInterface`. - **An interface declaration must match its implementation's signature** — a parameter that exists only on the implementation is unreachable to the interface-typed consumers this project mandates. Adding one to the interface is **breaking**. - **Public API symbols** are annotated `@psalm-api` to suppress unused-symbol warnings. @@ -76,7 +76,7 @@ Rules below; harness detail (cassettes, functional tests, spies) is in [docs/age - **Framework:** PHPUnit 12+, config `.github/phpunit.xml`. Test namespace `CNICTEST\` mirroring `CNIC\`. - **Test classes:** always `final class` extending `\PHPUnit\Framework\TestCase`; methods `testDescriptiveName` in camelCase. -- **Mocking:** register mock API responses via `ResponseTemplateManager::addTemplate()`, or use the hand-written spies (`SpyTransport`, `SpyResponseParser`) — do **not** add Mockery or Prophecy. +- **Mocking:** register mock API responses on a `ResponseTemplateManager` **instance** and hand it to the Response — `new Response($templateId, templates: (new RTM())->addTemplate(…))` — or use the hand-written spies (`SpyTransport`, `SpyResponseParser`). Do **not** add Mockery or Prophecy, and do **not** reintroduce a static template container (RSRMID-2941). - **Shared state:** `static` properties + `setUpBeforeClass()` for one-time client setup. - **No real API calls in unit tests.** `request()`-path tests replay committed cassettes offline (`composer test`); re-record against OT&E only with `composer test:record` when the exercised API behaviour changes. `tests/Functional/` is the one deliberate exception — a loopback HTTP server, skipped if it cannot bind a port. - **Direct parser tests live in `tests//ResponseParserTest.php`** — keep parse assertions out of `ResponseTest.php`. @@ -173,7 +173,7 @@ Opus decides, Sonnet implements: plan and review in the main thread, hand the me - Read, display, or expose the contents of `env.sh` — it contains secrets - Add dependencies without explicit request — this is a lightweight SDK - Throw a bare `\Exception` or declare exception types outside `CNIC\Exception` -- Use mocking frameworks (Mockery, Prophecy) — use ResponseTemplateManager or the repo's spies +- Use mocking frameworks (Mockery, Prophecy) — use a `ResponseTemplateManager` instance or the repo's spies - Add `@author` tags to docblocks - Add `Co-Authored-By:` trailers to commit messages diff --git a/MIGRATION.md b/MIGRATION.md index 2296c744..ec39ada6 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -10,31 +10,32 @@ Semantic versioning applies: **only major bumps (`X.0.0`) can break your code.** ## Version compatibility at a glance -| From → To | PHP required | Headline breaking change | Consumer action | -| --------- | ------------ | --------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| → v9.0.0 | **8.1+** | PHP 8.1 minimum | Bump your runtime | -| → v10.0.0 | 8.1+ | cURL handle cached/reused | Call `close()` in sessionless flows | -| → v11.0.0 | 8.1+ | IBS + Moniker brands added | None (additive) | -| → v12.0.0 | 8.1+ | HEXONET brand removed (EOL) | Migrate off HEXONET | -| → v13.0.0 | 8.1+ | IBS/Moniker switched to JSON API | Re-test IBS/Moniker data handling | -| → v14.0.0 | **8.3+** | Some classes `final`; `getPOSTData()` no longer takes a string | Bump runtime; stop subclassing finals | -| → v15.0.0 | 8.3+ | Logger contract; IBS session methods removed | Retype loggers; guard session calls | -| → v16.0.0 | 8.3+ | `ClientFactory::getClient()` signature slimmed | Configure the client yourself | -| → v17.0.0 | 8.3+ | `getNextPageNumber()` returns `null` on last page | Handle the `null` sentinel | -| → v18.0.0 | 8.3+ | CNR-only response methods moved off `ResponseInterface` | Narrow via `ExtendedResponseInterface` | -| → v19.0.0 | 8.3+ | `getClient()` removed; `setRoleCredentials()` moved | Use `cnr()`/`ibs()`/`moniker()` | -| → v20.0.0 | 8.3+ | IBS/Moniker no longer force IPv4; `getColumnKeys()` declares its `bool` parameter | Set `CURLOPT_IPRESOLVE` yourself if your host needs it; add the parameter if you implement `ResponseInterface` | -| → v21.0.0 | 8.3+ | `setExtraCurlOptions()` now reaches the wire; transport-owned options throw | Audit what you pass it — options previously ignored now take effect, and seven now raise | -| → v22.0.0 | 8.3+ | Sessions are CNR-only by type; IBS/Moniker `SessionClient` deleted | Drop `setSession()`/`getSession()` calls on IBS/Moniker; retype to `IBS\Client`/`MONIKER\Client` | -| → v23.0.0 | 8.3+ | Connection configuration has one home; `getSystem()` is nullable | Handle `null` from `getSystem()`; move `CURLOPT_TIMEOUT`/`USERAGENT`/`PROXY`/`REFERER` to their own setters | -| → v24.0.0 | 8.3+ | CNR IDN command rewriting moved off the shared client into its own module | Nothing, unless you called or overrode `autoIDNConvert()`, or read/set `needsIDNConvert` | -| → v25.0.0 | 8.3+ | One shared `Record` and `Column`; the brand `Record`/`IBS\Column` classes removed | Retype `CNR\Record`/`IBS\Record`/`AbstractRecord` → `CNIC\Record`, and `IBS\Column` → `CNIC\Column` | -| → v26.0.0 | 8.3+ | Response parsing is an injectable seam; `ResponseParser::parse()` is no longer static | Call `(new ResponseParser())->parse(…)`; implement `newResponseParser()` in a custom Response/TemplateManager | -| → v27.0.0 | 8.3+ | Loggers `format()` a record and a sink writes it; `setDefaultLogger()` removed | Rename your `log()` body to `format()` and `return` the string; extend `CNIC\AbstractLogger` | -| → v28.0.0 | 8.3+ | IBS/Moniker hash dates keep `/`; `RecordInterface`/`ColumnInterface` gained a date accessor; `IBS\Response::getStatus()` removed | Accept `/` wherever you parsed a `getHash()`/`getPlain()`/`getListHash()` date; add the new method if you implement either interface directly; read `getHash()["status"]` instead of `getStatus()` | -| → v29.0.0 | 8.3+ | Public method parameters and six protected properties renamed to be self-describing | Nothing, unless you pass named arguments (`getColumn(key: …)` → `columnName:`), implement an SDK interface (match the parameter names), or subclass and read `$this->pw`/`$ua`/`$curlopts` | -| → v30.0.0 | 8.3+ | Transport error is a declared `?string $error` parameter, not a `"httperror\|"` prefix on the raw payload; `nocurl` template gone | Add the parameter if you override `newResponse()`/`translate()`; return `["", $error]` (not bytes) on failure if you implement `TransportInterface` | -| → v31.0.0 | 8.3+ | `Response` is sealed after construction: the two mutators and the four record-cursor methods are off `ResponseInterface` | Replace `getNextRecord()` loops with `foreach ($r as $rec)` (it yields the first row too) and `getCurrentRecord()` with `getRecord(0)`; take `populate()`'s three new arguments if you subclass | +| From → To | PHP required | Headline breaking change | Consumer action | +| --------- | ------------ | --------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| → v9.0.0 | **8.1+** | PHP 8.1 minimum | Bump your runtime | +| → v10.0.0 | 8.1+ | cURL handle cached/reused | Call `close()` in sessionless flows | +| → v11.0.0 | 8.1+ | IBS + Moniker brands added | None (additive) | +| → v12.0.0 | 8.1+ | HEXONET brand removed (EOL) | Migrate off HEXONET | +| → v13.0.0 | 8.1+ | IBS/Moniker switched to JSON API | Re-test IBS/Moniker data handling | +| → v14.0.0 | **8.3+** | Some classes `final`; `getPOSTData()` no longer takes a string | Bump runtime; stop subclassing finals | +| → v15.0.0 | 8.3+ | Logger contract; IBS session methods removed | Retype loggers; guard session calls | +| → v16.0.0 | 8.3+ | `ClientFactory::getClient()` signature slimmed | Configure the client yourself | +| → v17.0.0 | 8.3+ | `getNextPageNumber()` returns `null` on last page | Handle the `null` sentinel | +| → v18.0.0 | 8.3+ | CNR-only response methods moved off `ResponseInterface` | Narrow via `ExtendedResponseInterface` | +| → v19.0.0 | 8.3+ | `getClient()` removed; `setRoleCredentials()` moved | Use `cnr()`/`ibs()`/`moniker()` | +| → v20.0.0 | 8.3+ | IBS/Moniker no longer force IPv4; `getColumnKeys()` declares its `bool` parameter | Set `CURLOPT_IPRESOLVE` yourself if your host needs it; add the parameter if you implement `ResponseInterface` | +| → v21.0.0 | 8.3+ | `setExtraCurlOptions()` now reaches the wire; transport-owned options throw | Audit what you pass it — options previously ignored now take effect, and seven now raise | +| → v22.0.0 | 8.3+ | Sessions are CNR-only by type; IBS/Moniker `SessionClient` deleted | Drop `setSession()`/`getSession()` calls on IBS/Moniker; retype to `IBS\Client`/`MONIKER\Client` | +| → v23.0.0 | 8.3+ | Connection configuration has one home; `getSystem()` is nullable | Handle `null` from `getSystem()`; move `CURLOPT_TIMEOUT`/`USERAGENT`/`PROXY`/`REFERER` to their own setters | +| → v24.0.0 | 8.3+ | CNR IDN command rewriting moved off the shared client into its own module | Nothing, unless you called or overrode `autoIDNConvert()`, or read/set `needsIDNConvert` | +| → v25.0.0 | 8.3+ | One shared `Record` and `Column`; the brand `Record`/`IBS\Column` classes removed | Retype `CNR\Record`/`IBS\Record`/`AbstractRecord` → `CNIC\Record`, and `IBS\Column` → `CNIC\Column` | +| → v26.0.0 | 8.3+ | Response parsing is an injectable seam; `ResponseParser::parse()` is no longer static | Call `(new ResponseParser())->parse(…)`; implement `newResponseParser()` in a custom Response/TemplateManager | +| → v27.0.0 | 8.3+ | Loggers `format()` a record and a sink writes it; `setDefaultLogger()` removed | Rename your `log()` body to `format()` and `return` the string; extend `CNIC\AbstractLogger` | +| → v28.0.0 | 8.3+ | IBS/Moniker hash dates keep `/`; `RecordInterface`/`ColumnInterface` gained a date accessor; `IBS\Response::getStatus()` removed | Accept `/` wherever you parsed a `getHash()`/`getPlain()`/`getListHash()` date; add the new method if you implement either interface directly; read `getHash()["status"]` instead of `getStatus()` | +| → v29.0.0 | 8.3+ | Public method parameters and six protected properties renamed to be self-describing | Nothing, unless you pass named arguments (`getColumn(key: …)` → `columnName:`), implement an SDK interface (match the parameter names), or subclass and read `$this->pw`/`$ua`/`$curlopts` | +| → v30.0.0 | 8.3+ | Transport error is a declared `?string $error` parameter, not a `"httperror\|"` prefix on the raw payload; `nocurl` template gone | Add the parameter if you override `newResponse()`/`translate()`; return `["", $error]` (not bytes) on failure if you implement `TransportInterface` | +| → v31.0.0 | 8.3+ | `Response` is sealed after construction: the two mutators and the four record-cursor methods are off `ResponseInterface` | Replace `getNextRecord()` loops with `foreach ($r as $rec)` (it yields the first row too) and `getCurrentRecord()` with `getRecord(0)`; take `populate()`'s three new arguments if you subclass | +| → v32.0.0 | 8.3+ | The response-template registry is an instance, not `public static array $templates`; `resetTemplates()` removed | Call `(new ResponseTemplateManager())->addTemplate(…)` and pass the registry as `new Response($id, templates: $registry)`; delete `resetTemplates()` calls; take `translate()`'s new argument if you subclass | Two things to respect throughout: @@ -870,6 +871,8 @@ final class LazyColumn implements \CNIC\ColumnInterface It is `addTemplate()`'s counterpart, not a general undo: it restores what the container held the first time you called `addTemplate()` on that class, it is per brand, and a direct assignment to the public `$templates` property is outside its reach. Register through `addTemplate()` and it will always take you back. + **If you are landing on v32 or later, skip this step** — the shared container it works around is gone and `resetTemplates()` with it. See [→ v32.0.0](#-v3200). + **Why this happened:** see the parse-seam entry in [docs/agents/architecture.md](docs/agents/architecture.md) for the full decision record. (Ref: RSRMID-2924.) --- @@ -1200,6 +1203,8 @@ final class MyTransport implements TransportInterface --- + + ## → v31.0.0 — a `Response` is sealed once constructed; `foreach` replaces the record cursor **What changed:** six methods came off `ResponseInterface`. Two were mutators (`addColumn()`, `addRecord()`); four were the record cursor (`getCurrentRecord()`, `getNextRecord()`, `getPreviousRecord()`, `rewindRecordList()`). A response is now fully assembled by its constructor and read-only afterwards, and its rows are walked with `foreach` — the interface extends `IteratorAggregate`. @@ -1258,6 +1263,107 @@ protected function populate(string $raw, ResponseParserInterface $parser, array --- + + +## → v32.0.0 — response templates live on an instance, not in a process-wide static bag + +**What changed:** `AbstractResponseTemplateManager` is now instantiable, and its templates belong to the instance. `public static array $templates` is gone from the base and from both brand managers; every operation that was `static` — `addTemplate()`, `getTemplate()`, `getTemplates()`, `hasTemplate()`, `generateTemplate()`, `isTemplateMatchHash()`, `isTemplateMatchPlain()` — is an instance method, declared on the new `CNIC\ResponseTemplateManagerInterface`. A registry reaches a response through a new trailing `?ResponseTemplateManagerInterface $templates = null` argument on `Response::__construct()`; omit it and you get the brand's built-ins exactly as before. + +**Who is affected — three groups:** + +1. **Anyone registering templates** with `ResponseTemplateManager::addTemplate(…)`. This is the change that touches real code. +2. **Anyone calling `resetTemplates()`.** It is **removed**, with no replacement — see "Why this happened". +3. **Anyone who subclasses `AbstractResponseTemplateManager`, `AbstractResponseTranslator`, or a brand `Response`.** Several hooks changed shape. + +**What to respect — registering and using a template:** + +```php +// BEFORE (v31) — a process-wide bag, and a reset to undo it +\CNIC\CNR\ResponseTemplateManager::addTemplate("OK", "200", "Command completed successfully"); +$r = new \CNIC\CNR\Response("OK"); +// …later, to stop it leaking into unrelated code: +\CNIC\CNR\ResponseTemplateManager::resetTemplates(); + +// AFTER (v32) — the registry is an object, and only what you hand it sees the template +$tpls = (new \CNIC\CNR\ResponseTemplateManager()) + ->addTemplate("OK", "200", "Command completed successfully"); +$r = new \CNIC\CNR\Response("OK", templates: $tpls); +// nothing to reset — $tpls goes out of scope with everything registered on it +``` + +Reads work the same way: `ResponseTemplateManager::hasTemplate("empty")` becomes `(new ResponseTemplateManager())->hasTemplate("empty")`. If you were reading the raw container as `ResponseTemplateManager::$templates`, use `getRawTemplates()` on an instance. + +**Note the one scope limit.** The registry reaches the translator through the `Response` constructor, so it applies to responses you construct directly. A response produced by `$client->request()` always uses the brand's built-ins — there is no client-level registry. If you relied on `addTemplate()` changing what `request()` returned, that worked only as a side effect of the global bag and has no successor; substitute a `TransportInterface` (see `setTransport()`) to control what a request returns instead. + +**What to respect — subclassing:** + +```php +// BEFORE (v31) +final class MyTemplateManager extends AbstractResponseTemplateManager +{ + public static array $templates = ["mycase" => "…"]; + + public static function generateTemplate(string $code, string $description): string { … } + public static function getTemplate(string $templateId): MyResponse { … } + protected static function createResponse(string $raw): MyResponse { … } + protected static function newResponseParser(): ResponseParserInterface { … } +} + +// AFTER (v32) — the container is a hook over a constant; four members are no longer static +final class MyTemplateManager extends AbstractResponseTemplateManager +{ + private const array BUILTIN_TEMPLATES = ["mycase" => "…"]; + + protected static function builtinTemplates(): array { return self::BUILTIN_TEMPLATES; } + + public function generateTemplate(string $code, string $description): string { … } + public function getTemplate(string $templateId): MyResponse { … } + protected function createResponse(string $raw): MyResponse + { + return new MyResponse($raw, templates: $this); // pass yourself down + } + protected function newResponseParser(): ResponseParserInterface { … } +} +``` + +`matchKeys()` stays `protected static`. `resetTemplates()` must be deleted; there is nothing left for it to restore. + +**If your manager declares its own `__construct()`, it must call `parent::__construct()`.** The base constructor is what seeds the typed `private array $templates` from `builtinTemplates()`; skip it and the first `addTemplate()`/`hasTemplate()`/`getRawTemplates()` call raises `Error: Typed property CNIC\AbstractResponseTemplateManager::$templates must not be accessed before initialization`. There was no base constructor before v32, so an existing subclass constructor will not be calling one. + +For a **custom translator**, the `templates(): array` hook is replaced by `newTemplateManager(): ResponseTemplateManagerInterface`, which returns a _fresh_ registry rather than a shared one: + +```php +// BEFORE (v31) +protected static function templates(): array { return MyTemplateManager::$templates; } + +// AFTER (v32) +protected static function newTemplateManager(): ResponseTemplateManagerInterface +{ + return new MyTemplateManager(); +} +``` + +For a **custom `Response`**, `translate()` takes the registry as a fifth argument and must forward it: + +```php +// AFTER (v32) +protected function translate( + string $raw, + array $cmd, + array $placeholders, + ?string $error = null, + ?ResponseTemplateManagerInterface $templates = null +): string { + return MyResponseTranslator::translate($raw, $cmd, $placeholders, $error, $templates); +} +``` + +**Also in this major:** `isTemplateMatchHash()` and `isTemplateMatchPlain()` no longer emit `Undefined array key` when the hash being compared is missing one of the brand's two match keys — they return `false`, which is what they already effectively answered, just without the notice. Both route through the same comparison, so the fix applies whether you pass the hash directly or a plain response that parses short of a key. + +**Why this happened:** the container was `public static` with process lifetime and the translator read it live at translate time, so a template registered for one scenario silently changed response translation in every later one — across test classes in a single PHPUnit process, and across requests in any long-lived consumer. `resetTemplates()` could not reliably contain that: it was a no-op unless `addTemplate()` had run first, and a direct assignment to the public property escaped it entirely. Making the property private was impossible while the translator read it across a class boundary. Handing the registry to the response that needs it removes the shared state rather than patching around it — which is also why `resetTemplates()` is gone rather than kept: state that cannot escape its object has nothing to reset. (Ref: RSRMID-2941.) + +--- + ## Reference: the canonical usage **Once you have finished upgrading, check your code against [the Usage section of README.md](README.md#usage)** — it holds the worked, per-brand example of idiomatic code for the current major (client construction, the `$path` argument, `close()`, and which capabilities are CNR-only), kept up to date rather than pinned to any one version. diff --git a/docs/agents/architecture.md b/docs/agents/architecture.md index 230dce67..7805aa8f 100644 --- a/docs/agents/architecture.md +++ b/docs/agents/architecture.md @@ -44,7 +44,7 @@ Full architectural reference for the PHP SDK. - **CNR takes `$cmd` and ignores it, on purpose.** A contract covering only IBS's shape, or two contracts, buys nothing — CNR's wire format is self-describing, and a uniform signature is what lets one seam serve both brands. - **`AbstractResponseTemplateManager::parseResponse()` is gone**, replaced by the same `newResponseParser()`. It still parses with **no** command (a template is not tied to one) while `populate()` passes the command it was built with; for IBS that selects the JSON versus plain-text branch. **Trap when asserting on that divergence:** the command must be one *without* `ResponseFormat` — one carrying `ResponseFormat=JSON` takes the same branch as the empty one and makes the assertion vacuous. - **A non-string cell — and equally a column entry that is not a list — throws** `UnsupportedFeatureException` naming the column, from `CNR\Response::stringCells()`. Skipping would be the silent no-op RSRMID-2919/RSRMID-2920 ruled out; coercing would invent a value the wire could never carry. The container half is the one that is easy to forget: `foreach ((array)$values …)` turns a bare string into a one-cell column while a bare `int` still throws a line later. Deliberately asymmetric with the level above: a **missing or non-array `PROPERTY` block** yields no columns rather than throwing, because most CNR responses legitimately have none. - - **`resetTemplates()` is `addTemplate()`'s counterpart, not a general undo, and a direct assignment to the public `$templates` property escapes it** — the RSRMID-2921 lesson that a guard on one writer does not protect state with several. Closing that writer means making the property non-public, which `CNR\ResponseTranslator::templates()` reads across a class boundary; that is a separate change. + - **`resetTemplates()` was `addTemplate()`'s counterpart, not a general undo — and it is gone.** It restored what the container held the first time `addTemplate()` ran for a class, so it was a no-op when the class was never added to, and a direct assignment to the public `$templates` property escaped it entirely: the RSRMID-2921 lesson that a guard on one writer does not protect state with several. The separate change it pointed to is RSRMID-2941 below, which removed the second writer by removing the shared container. - **Build-then-populate remains impossible by design.** The seam **adds a sixth** constructor dispatch rather than removing any, because injection makes the parse step substitutable without touching the constructor's load-bearing shape. **Revisit** as a ticket about the constructor, not about parsing. - **Guard:** `tests/ResponseParserSeamTest.php` (behavioural half via `CNICTEST\Support\SpyResponseParser`, structural half against a re-inlined `new RP()`). - **The logger seam is at the format, not the sink (RSRMID-2925, breaking, v27.0.0).** `LoggerInterface::format(string $post, ResponseInterface $response, ?string $error = null): string` owns what varies per brand; `CNIC\LogSinkInterface::write(string): void` owns the destination. `CNIC\AbstractLogger` holds the sink (default `CNIC\EchoSink`), implements `log()` as `sink->write(format(…))` and leaves `format()` abstract. Previously the seam sat where nothing varied — every brand ended in `echo` — while the part that does vary returned nothing and could only be observed by capturing output. @@ -138,9 +138,9 @@ Full architectural reference for the PHP SDK. - **No `*SeamTest.php` guard** — `tests/RedactionParityTest.php` pins that each brand's `SocketConfig` and `Response` defaults for `$sensitiveFields` are reflectively equal to each other and to `SensitiveFields::KEYS`, which is a value-equality check, not a structural seam, so it does not follow the guard-test docblock ritual. `tests/CommandRedactorTest.php` is `CommandRedactor`'s direct, brand-independent unit coverage — the redaction algorithm's first test surface of its own, previously reachable only through a brand config or response. - **MONIKER is asserted, not just asserted-in-prose.** `testMonikerInheritsTheIbsListRatherThanDeclaringItsOwn()` pins that `MONIKER\SocketConfig` still resolves to `IBS\SensitiveFields::KEYS` by inheritance — there is no `MONIKER\Response` and no MONIKER field list, and a MONIKER-local override that drifted from IBS would otherwise be the one drift this file's own anti-drift test could not see. Proven non-vacuous by adding `protected array $sensitiveFields = ["password"]` to `MONIKER\SocketConfig` and watching that test alone fail (the CNR and IBS cases stayed green, so the assertion is targeted rather than incidentally coupled). - **The transport error is a declared parameter, not encoded into the payload (RSRMID-2937, breaking, v30.0.0).** `HttpTransport::post()` used to return the failure twice — as tuple element `[1]`, and as a `"httperror|"` prefix smuggled onto `[0]`, which `AbstractResponseTranslator::translate()` string-split back off with `explode("|", $newraw, 2)`. The sentinel is removed: `?string $error` is now an explicit trailing parameter, appended **last** and defaulted `null` on every hook in the pipeline (`TransportInterface::post()`'s tuple, `AbstractClient::newResponse()`/both brand overrides, `AbstractResponse::__construct()`/`translate()`/both brand overrides, `AbstractResponseTranslator::translate()`), so every existing positional call site keeps working. The two-level check is unchanged: `$error !== null` selects the `httperror` template, `$error !== ""` gates the `{HTTPERROR}` injection into it. `TransportInterface::post()`'s docblock now **states** the contract it always implied: a non-null `[1]` means `[0]` is unusable, and `HttpTransport` honours it by returning `["", $error]` on failure rather than any payload at all. - - **`nocurl` is gone with the branch it existed for.** `curl_init() === false` is unreachable with `ext-curl` as a hard composer dependency; the guard is now `\assert($tmp !== false)`, matching the file's existing idiom, and the `"nocurl"` entry is deleted from both brand `ResponseTemplateManager::$templates`. Do not re-add it without a real reachable failure mode to attach it to. - - **`AbstractResponseTranslator::resolveTemplateId(string $raw, ?string $error, array $templates): ?string` is the extracted decision point, and it is where the raw-as-template-id mocking route becomes explicit rather than incidental.** A non-null `$error` resolves to `"httperror"` **only if `$templates` declares that id** — `templates()` is an abstract hook, so a third-party brand translator's container need not include it, and indexing `$templates["httperror"]` unconditionally degrades an `Undefined array key` warning into a `TypeError` a few lines later. Otherwise `$raw` is checked against `$templates` and returned only if it matches — which is exactly what lets a test construct `new Response("empty")` or `new Response("nocurl-replacement-id")` (after `ResponseTemplateManager::addTemplate()`) and get a canned response with no real API round-trip. That lookup is the sanctioned mocking route CLAUDE.md names, not a leak of transport internals into response data; do not "fix" it by requiring a real API shape. `$templates` is a parameter rather than a second `static::templates()` call inside the method: `translate()` already binds one snapshot for the rest of its body, and resolving the id against a different call's snapshot than the one it is then dereferenced against is exactly the kind of inconsistency a registry-backed hook invites. The method must genuinely return `null` (an ordinary raw response, or an `$error` with no matching `"httperror"` entry) for PHPStan L9 to accept the `?string` signature — a version that only ever returned a string is flagged `return.unusedType` and is not just a style nit, it is evidence the null branch was never reachable. Regression coverage: `tests/AbstractResponseTranslatorFallbackTest.php`, using a fixture translator subclass rather than mutating a real brand's public static `$templates` bag (mutating that shared, process-lifetime state would make the suite order-dependent — the RSRMID-2941 defect). - - **No enum or const set for template ids, on purpose.** RSRMID-2941 replaces the `array` template registry itself; typing `resolveTemplateId()`'s return against a set of ids from the registry about to be thrown away would be designed against a foundation already scheduled for removal. + - **`nocurl` is gone with the branch it existed for.** `curl_init() === false` is unreachable with `ext-curl` as a hard composer dependency; the guard is now `\assert($tmp !== false)`, matching the file's existing idiom, and the `"nocurl"` entry is deleted from both brand template sets (then `ResponseTemplateManager::$templates`, now `BUILTIN_TEMPLATES`). Do not re-add it without a real reachable failure mode to attach it to. + - **`AbstractResponseTranslator::resolveTemplateId(string $raw, ?string $error, array $rawTemplates): ?string` is the extracted decision point, and it is where the raw-as-template-id mocking route becomes explicit rather than incidental.** A non-null `$error` resolves to `"httperror"` **only if `$rawTemplates` declares that id** — the registry is caller-supplied, so a third-party brand's need not include it, and indexing `$rawTemplates["httperror"]` unconditionally degrades an `Undefined array key` warning into a `TypeError` a few lines later. Otherwise `$raw` is checked against `$rawTemplates` and returned only if it matches — which is exactly what lets a test construct `new Response("empty")` or `new Response("myid", templates: $registry)` (after `addTemplate()` on that registry) and get a canned response with no real API round-trip. That lookup is the sanctioned mocking route CLAUDE.md names, not a leak of transport internals into response data; do not "fix" it by requiring a real API shape. `$rawTemplates` is a parameter rather than a second read from the registry inside the method: `translate()` already binds one snapshot for the rest of its body, and resolving the id against a different read's snapshot than the one it is then dereferenced against is exactly the kind of inconsistency a registry-backed hook invites. The method must genuinely return `null` (an ordinary raw response, or an `$error` with no matching `"httperror"` entry) for PHPStan L9 to accept the `?string` signature — a version that only ever returned a string is flagged `return.unusedType` and is not just a style nit, it is evidence the null branch was never reachable. Regression coverage: `tests/AbstractResponseTranslatorFallbackTest.php`, using a fixture registry rather than a real brand's — originally because the container was shared process-wide state (the RSRMID-2941 defect), now because what it needs is a registry *missing* an id every real brand ships. + - **No enum or const set for template ids, on purpose.** The judgement predates RSRMID-2941 (which was then still going to replace the registry wholesale) and survives it: the id set is open by design — `addTemplate()` accepts any string, and that is the mocking route — so a closed type would have to be reopened by every caller registering one. - **Why this has no `*SeamTest.php` guard, unlike most seams in this file — and what is and is not actually caught.** (1) The parameter is compiler-enforced — PHP performs LSP checking on abstract-method implementations, so a brand `Client`/`Response` that drops the trailing `?string $error` from only *some* of `newResponse()`/`translate()` — an accidental, partial drift rather than a deliberate lockstep revert — is a fatal at declaration time, not a runtime drift a reflection test would need to catch. (2) **State plainly what is and is not caught, rather than overclaiming coverage.** A *partial* sentinel revert fails loudly: re-encoding `"httperror|"` onto `$raw` inside `post()` without also restoring `explode("|", $newraw, 2)` inside `translate()` leaves `$raw` matching no known template id, so `hasMissingRequiredFields()` routes to `"invalid"` (423) instead of `"httperror"` (421), and `tests/CNR/ClientTest.php::testRequestCurlExecFail2` — which asserts both `getCode() === 421` and the exact `getDescription()` string against the hand-authored `conn-error` cassette — fails on both axes. A *complete*, consistent revert (sentinel re-encoded, the `explode()` split restored, and `?string $error` dropped from all seven signatures together) compiles and is behaviour-preserving, so it would leave the suite green — that gap is **accepted as an out-of-scope risk, not closed by a guard**, because undoing the decision that way is a deliberate, whole-hierarchy reversal of a documented [MIGRATION.md → v30.0.0](../../MIGRATION.md#-v3000) `BREAKING CHANGE`, not the invisible one-line drift guard tests in this project exist to catch. (3) The only test shape left that would close even that remaining gap is a negative-space source sweep ("assert `"httperror|"` does not appear in `src/`"), which is exactly the vacuous-guard shape this file warns against elsewhere — it pins a string literal's absence, not a decision, and a rewrite using a different delimiter would slip through untouched. - **One plain behavioural test pins the newly-stated `TransportInterface` contract, because nothing else does.** `TransportSeamTest::testTransportErrorDiscardsParseableBytes()` injects a transport double returning real, parseable CNR bytes **and** a non-null error together, and asserts the `httperror` template wins — the bytes are discarded, not merged or preferred. This is ordinary behavioural coverage, not a guard: no reflection, no source sweep, just `request()` through the seam `TransportSeamTest` already exists to exercise (RSRMID-2910). - **All three client-held collaborators are readable, so tests assert wiring through the interface instead of reflecting (RSRMID-2940, non-breaking).** The transport and logger seams were write-only — `newTransport()`/`setTransport()` and `newLogger()`/`setLogSink()`/`setCustomLogger()` with no reader — while the config seam had `getSocketConfig()`, whose docblock already credits the accessor with stopping forwarder sprawl. `AbstractClient::getTransport(): TransportInterface` and `getLogger(): LoggerInterface` close the asymmetry; there is nothing to remember about which of the three is readable. @@ -149,3 +149,10 @@ Full architectural reference for the PHP SDK. - **No guard test, and none is possible in the usual shape.** These accessors are not a seam whose removal is behaviour-preserving — delete one and every test calling it fails to compile. The reflection they replace is the thing being removed, so there is nothing structural left to pin. - **`LoggerSeamTest::testTheClientDefaultsToTheEchoSink()` was rewritten behaviourally rather than served by a third accessor.** It read `AbstractLogger::$sink` reflectively, which a *client*-side getter cannot reach; it now buffers output and asserts the default logger's record lands on standard output — which is the promise the shipped default actually makes, with the `EchoSink` identity being an implementation detail. **Rejected: `AbstractLogger::getSink()`.** `CNR\Logger`/`IBS\Logger` are concrete implementors of `LoggerInterface`, one of `InterfaceCoverageSeamTest`'s total contracts, so an inherited public `getSink()` is a stray-method failure; fixing that means declaring it on `LoggerInterface`, which is breaking — turning a no-bump ticket into a major to delete one reflection call. - **`SpyTransport` now records `$data` and `$closed`, closing two gaps its own docblock's promise had left open.** The spy recorded four of `post()`'s five arguments, so "the bytes on the wire are what `getPOSTData()` produced" was only ever asserted in halves — the encoding in `tests/CNR/ClientTest.php::testGetPostDataSecured()`, the delivery in `TransportSeamTest` — and a rewrite of either half alone kept both green. Proven by mutation: appending `"&MUTANT=1"` to `performRequest()`'s payload fails `testTheBytesOnTheWireAreWhatGetPostDataProduced()` **and nothing else in the suite**. Separately, no test called a *client's* `close()` — every `->close()` ran against a transport instance directly — so the one-line delegation in `AbstractClient::close()` was unexercised; emptying its body now fails `testClientCloseDelegatesToTheTransport()`. +- **The response-template registry is an instance, handed to the Response that needs it (RSRMID-2941, breaking, v32.0.0).** The container was `public static array $templates`, redeclared per brand and read live by the translator at translate time, so `addTemplate()` in one test class changed response translation in every later one. It is now `private array $templates` on an instantiable `ResponseTemplateManager`, seeded per instance from a per-brand `private const BUILTIN_TEMPLATES`, reached through the new `CNIC\ResponseTemplateManagerInterface`, and threaded `AbstractResponse::__construct(…, ?ResponseTemplateManagerInterface $templates = null)` → brand `translate()` → `AbstractResponseTranslator::translate()`. Every operation that was `static` is now an instance method; `resetTemplates()` and the `$builtinTemplates` per-class cache behind it are **deleted**, because state that cannot escape its object has nothing to reset. + - **The seam stops at `AbstractResponse`; `AbstractClient` is untouched — a scope call, not an omission.** Every `addTemplate()` call site in the repo feeds a direct `new Response($templateId, …)`, never a `$client->request()`, so threading a registry through `performRequest()`/`newResponse()` and each brand override would add public surface for a capability nothing asks for. Adding it later is purely additive. **Revisit** on a real consumer need for bootstrap-time brand-wide overrides that every later `request()` sees — and answer it with the client-level thread, **not** by restoring the static container. + - **`addTemplate()` returns `$this`, where the static predecessor returned `new static()`** — a throwaway instance of an all-static class, fluent in shape only. Chaining now registers onto the object the caller holds, which is the difference between `(new RTM())->addTemplate(…)->addTemplate(…)` working and quietly registering into nowhere. + - **`getRawTemplates()` is separate from `getTemplates()` and both are on the interface.** The translator needs the raw wire strings; `getTemplates()` builds a `Response` per entry, and building responses in order to translate one would recurse. The old code had the same split implicitly — the translator read the public property directly while `getTemplates()` constructed — which is precisely why the property could not be made private. + - **`matches()` now checks key existence before comparing, fixing the latent bug the ticket named.** `isTemplateMatchHash(["status" => "SUCCESS"], "404")` emitted `Undefined array key` and then compared `null`. **Trap when covering this:** the unguarded version still *returns* `false` (null never equals the template's value), so `assertFalse` alone is vacuous — and `.github/phpunit.xml` sets no `failOnWarning`, so the notice does not fail the build either. The guard installs a `set_error_handler` and asserts nothing was raised; without that, the mutation exits 0. + - **`ResponseTemplateManagerInterface` is a "total" contract and joins `InterfaceCoverageSeamTest::TOTAL_INTERFACES` (now 8, implementor floor now 13).** That guard's own revisit condition is "a genuinely new total interface is introduced", and this is one: it fully describes both brand managers, which until now implemented no swept interface and so had eight public methods the sweep could not see. Adding it was free — zero stray and zero widened methods on either brand. Verified non-vacuous the same way the guard's original entry was: a stray `public function strayMethod()` on `IBS\ResponseTemplateManager` now fails it, and did not before. + - **Guard:** `tests/ResponseTemplateRegistrySeamTest.php`. Proven non-vacuous against four mutations, each failing it and nothing else: reintroducing any static property on a brand manager; a brand `translate()` that drops the `$templates` argument it was handed; a shared container behind the per-instance façade; and reverting the `matches()` existence check. The two `resetTemplates()` cases that used to close `tests/ResponseParserSeamTest.php` are gone with the API they described. diff --git a/docs/agents/testing.md b/docs/agents/testing.md index 722deb57..02c369f5 100644 --- a/docs/agents/testing.md +++ b/docs/agents/testing.md @@ -20,9 +20,18 @@ The server starts in `setUpBeforeClass()` and the class calls `markTestSkipped() ## Mocking is template-driven, never a mocking framework -Register mock API responses with `ResponseTemplateManager::addTemplate()`; do not add Mockery or Prophecy. Where a substitute for a collaborator is needed, the repo uses hand-written spies against the interface seams — `CNICTEST\Support\SpyTransport` (`TransportInterface`) and `CNICTEST\Support\SpyResponseParser` (`ResponseParserInterface`). Both reach the behaviour through public API with no reflection and no subclassing, which is the property those seams exist to provide. +Register mock API responses on a `ResponseTemplateManager` instance and hand that instance to the Response; do not add Mockery or Prophecy. Where a substitute for a collaborator is needed, the repo uses hand-written spies against the interface seams — `CNICTEST\Support\SpyTransport` (`TransportInterface`) and `CNICTEST\Support\SpyResponseParser` (`ResponseParserInterface`). Both reach the behaviour through public API with no reflection and no subclassing, which is the property those seams exist to provide. -Note that `$templates` is public static state with process lifetime: `AbstractResponseTemplateManager::resetTemplates()` is `addTemplate()`'s counterpart and is per concrete brand class. A direct assignment to the public property escapes it — see the RSRMID-2924 entry in [architecture.md](architecture.md). +```php +$tpls = (new \CNIC\CNR\ResponseTemplateManager()) + ->addTemplate("OK", "200", "Command completed successfully"); + +$r = new \CNIC\CNR\Response("OK", templates: $tpls); +``` + +The registry is instance state (RSRMID-2941), so **there is nothing to tear down** — a template registered here cannot reach a test class that did not ask for it, and the old `resetTemplates()` is gone along with the leak it patched. A class-wide set of templates goes in a `static` property assigned from `setUpBeforeClass()`, as `tests/CNR/ResponseTest.php` does; the responses that need them pass `templates:` explicitly. Do not add a static container back — `tests/ResponseTemplateRegistrySeamTest.php` refuses it. + +Templates only reach the translator through that argument, so this route works for a Response built directly, not for one produced by `$client->request()`. Every mock in the suite is a direct construction; a client-level registry was deliberately not added (see the guard's revisit condition). ## MONIKER test files mirroring IBS is intentional diff --git a/src/AbstractResponse.php b/src/AbstractResponse.php index 96db8421..5b291132 100644 --- a/src/AbstractResponse.php +++ b/src/AbstractResponse.php @@ -158,6 +158,7 @@ abstract class AbstractResponse implements ResponseInterface * @param array $context context data for the response (for use in custom loggers etc., optional, has no impact on SDK behaviour) * @param ResponseParserInterface|null $parser parser to use instead of the brand default (see newResponseParser()) * @param string|null $error transport error, if any; non-null means $raw is unusable and the brand's "httperror" template is substituted instead (see {@see AbstractResponseTranslator::translate()}) + * @param ResponseTemplateManagerInterface|null $templates registry the translator resolves template ids against; null uses the brand's built-ins. Supplying one is how a caller scopes a registered template to this response instead of to the whole process (RSRMID-2941) */ public function __construct( string $raw, @@ -165,13 +166,14 @@ public function __construct( array $placeholders = [], array $context = [], ?ResponseParserInterface $parser = null, - ?string $error = null + ?string $error = null, + ?ResponseTemplateManagerInterface $templates = null ) { $cmd = $this->sanitizeCommand($cmd); $this->context = $context; $this->command = $cmd; $this->requestUrl = $placeholders["CONNECTION_URL"] ?? ""; - $translated = $this->translate($raw, $cmd, $placeholders, $error); + $translated = $this->translate($raw, $cmd, $placeholders, $error, $templates); $this->raw = $translated; $this->populate($translated, $parser ?? $this->newResponseParser(), $cmd); } @@ -183,8 +185,15 @@ public function __construct( * @param array $cmd API command used within this request * @param array{CONNECTION_URL?: string} $placeholders * @param string|null $error transport error, if any; non-null means $raw is unusable (see {@see AbstractResponseTranslator::translate()}) + * @param ResponseTemplateManagerInterface|null $templates registry to resolve template ids against; null uses the brand's built-ins */ - abstract protected function translate(string $raw, array $cmd, array $placeholders, ?string $error = null): string; + abstract protected function translate( + string $raw, + array $cmd, + array $placeholders, + ?string $error = null, + ?ResponseTemplateManagerInterface $templates = null + ): string; /** * Parse the translated response into the hash and build the column/record diff --git a/src/AbstractResponseTemplateManager.php b/src/AbstractResponseTemplateManager.php index 0ee4f74e..5dcd8d7f 100644 --- a/src/AbstractResponseTemplateManager.php +++ b/src/AbstractResponseTemplateManager.php @@ -12,47 +12,53 @@ /** * Shared base for all registrar ResponseTemplateManager implementations. * - * The template container plus its add/get/has/reset/match operations are - * identical across brands; only the raw template strings, the generateTemplate() + * The template container plus its add/get/has/match operations are identical + * across brands; only the built-in template strings, the generateTemplate() * wire format, the two hash keys used for matching, and the concrete Response / * ResponseParser classes differ. Concrete subclasses supply those via the - * abstract hooks below and redeclare their own $templates array. + * abstract hooks below. + * + * **The container is per instance** (RSRMID-2941). It used to be a + * `public static array $templates` redeclared per brand, read live by + * {@see AbstractResponseTranslator}, so `addTemplate()` in one test class + * changed response translation in every later one; the `resetTemplates()` that + * tried to contain that was a no-op unless `addTemplate()` had run first and + * could not undo a direct write to the public property at all. Both are gone. + * The built-ins now live in an immutable per-brand hook and are copied into + * each new instance, so an override is scoped to the object that received it + * and there is nothing left to reset. See {@see ResponseTemplateManagerInterface}. * - * @psalm-consistent-constructor * @package CNIC */ -abstract class AbstractResponseTemplateManager +abstract class AbstractResponseTemplateManager implements ResponseTemplateManagerInterface { /** - * Template container - * @var array + * This registry's templates (template id => raw wire text), seeded from the + * brand's built-ins and mutated only by {@see addTemplate()}. + * @var array */ - public static array $templates = []; + private array $templates; - /** - * The brand's built-in templates, captured per concrete class the first time - * that class' container is mutated, so {@see resetTemplates()} can restore - * them. Keyed by class name because each subclass redeclares $templates and - * therefore has a container of its own. - * @var array> - */ - private static array $builtinTemplates = []; - - /** - * Generate API response template string for given code and description - */ - abstract public static function generateTemplate(string $code, string $description): string; + public function __construct() + { + $this->templates = static::builtinTemplates(); + } /** - * Get response template instance from template container. - * Subclasses narrow the return type to their concrete Response. + * The brand's built-in templates (template id => raw wire text). + * + * Declared as a hook over a constant rather than a property so the + * built-ins cannot be written to: each instance gets a copy, and no route + * exists to change what the *next* instance starts from. + * @return array */ - abstract public static function getTemplate(string $templateId): ResponseInterface; + abstract protected static function builtinTemplates(): array; /** - * Create a brand Response instance from a template id or raw response. + * Create a brand Response instance from a template id or raw response, + * resolving template ids against **this** registry. */ - abstract protected static function createResponse(string $raw): ResponseInterface; + abstract protected function createResponse(string $raw): ResponseInterface; /** * Instantiate the brand's response parser. @@ -61,7 +67,7 @@ abstract protected static function createResponse(string $raw): ResponseInterfac * both name the same brand parser, so the shared pipeline can parse a plain * response without each subclass repeating the call. */ - abstract protected static function newResponseParser(): ResponseParserInterface; + abstract protected function newResponseParser(): ResponseParserInterface; /** * The two response-hash keys this brand compares when matching a template @@ -71,80 +77,67 @@ abstract protected static function newResponseParser(): ResponseParserInterface; abstract protected static function matchKeys(): array; /** - * Add response template to template container + * Register a template on this registry. * @param string $plain API plain response, or API response code when $description is given */ - public static function addTemplate(string $templateId, string $plain, ?string $description = null): static + #[\Override] + public function addTemplate(string $templateId, string $plain, ?string $description = null): static { - self::$builtinTemplates[static::class] ??= static::$templates; - static::$templates[$templateId] = is_null($description) + $this->templates[$templateId] = is_null($description) ? $plain - : static::generateTemplate($plain, $description); - return new static(); + : $this->generateTemplate($plain, $description); + return $this; } /** - * Restore the brand's built-in templates, discarding everything - * {@see addTemplate()} has registered since. - * - * The container is public static state with process lifetime, so a template - * registered for one scenario stays visible to every later one — across test - * classes in the same PHPUnit process, and in any long-lived consumer that - * registers templates per request. Call this when a scenario ends (e.g. from - * tearDownAfterClass()) so the next one starts from the brand defaults. - * - * Scope, stated rather than implied: this is the counterpart of - * {@see addTemplate()}, not a general undo. It restores what the container - * held the first time addTemplate() ran for *this* class, so it is per brand - * and a no-op when the class was never added to. A direct assignment to the - * public $templates property is outside its reach — closing that second - * writer would mean making the property non-public, which - * {@see \CNIC\CNR\ResponseTranslator::templates()} reads across a class - * boundary, so it is a separate change. Go through addTemplate(). - * @psalm-api + * Every template in this registry as a Response, keyed by template id. + * @return array */ - public static function resetTemplates(): void + #[\Override] + public function getTemplates(): array { - if (isset(self::$builtinTemplates[static::class])) { - static::$templates = self::$builtinTemplates[static::class]; + $tpls = []; + foreach ($this->templates as $key => $raw) { + $tpls[$key] = $this->createResponse($raw); } + return $tpls; } /** - * Return all available response templates - * @return array + * Every template in this registry as its raw wire text. + * @return array */ - public static function getTemplates(): array + #[\Override] + public function getRawTemplates(): array { - $tpls = []; - foreach (static::$templates as $key => $raw) { - $tpls[$key] = static::createResponse($raw); - } - return $tpls; + return $this->templates; } /** - * Check if given template exists in template container + * Check if given template exists in this registry. */ - public static function hasTemplate(string $templateId): bool + #[\Override] + public function hasTemplate(string $templateId): bool { - return array_key_exists($templateId, static::$templates); + return array_key_exists($templateId, $this->templates); } /** * Check if given API response hash matches a given template by code and description * @param array $responseHash */ - public static function isTemplateMatchHash(array $responseHash, string $templateId): bool + #[\Override] + public function isTemplateMatchHash(array $responseHash, string $templateId): bool { - return self::matches(static::getTemplate($templateId)->getHash(), $responseHash); + return $this->matches($this->getTemplate($templateId)->getHash(), $responseHash); } /** * Check if given API plain response matches a given template by code and description * @param string $plain API plain response */ - public static function isTemplateMatchPlain(string $plain, string $templateId): bool + #[\Override] + public function isTemplateMatchPlain(string $plain, string $templateId): bool { // Parsed with no command on purpose: a template is not tied to one, and // the brand parsers that read $cmd use it only to pick their wire branch @@ -154,20 +147,31 @@ public static function isTemplateMatchPlain(string $plain, string $templateId): // yield the same hash. Pinned by IBS's ResponseTemplateManagerTest and its // ResponseParserTest; keep that assertion, it is what stops the two routes // diverging unnoticed. - return self::matches(static::getTemplate($templateId)->getHash(), static::newResponseParser()->parse($plain)); + return $this->matches($this->getTemplate($templateId)->getHash(), $this->newResponseParser()->parse($plain)); } /** * Compare two response hashes on this brand's match keys. + * + * A key absent from either hash means "no match", not a warning: the + * response being compared is arbitrary caller input (see + * {@see isTemplateMatchHash()}), so `["status" => "SUCCESS"]` against a + * template carrying a `message` must answer false rather than emit + * "Undefined array key" and then compare null (RSRMID-2941). + * * @param array $templateHash * @param array $responseHash */ - private static function matches(array $templateHash, array $responseHash): bool + private function matches(array $templateHash, array $responseHash): bool { - [$codeKey, $descrKey] = static::matchKeys(); - return ( - ($templateHash[$codeKey] === $responseHash[$codeKey]) && - ($templateHash[$descrKey] === $responseHash[$descrKey]) - ); + foreach (static::matchKeys() as $key) { + if (!array_key_exists($key, $templateHash) || !array_key_exists($key, $responseHash)) { + return false; + } + if ($templateHash[$key] !== $responseHash[$key]) { + return false; + } + } + return true; } } diff --git a/src/AbstractResponseTranslator.php b/src/AbstractResponseTranslator.php index 1e9ddadd..a882e077 100644 --- a/src/AbstractResponseTranslator.php +++ b/src/AbstractResponseTranslator.php @@ -18,7 +18,7 @@ * fallback, the two description-map rewrite loops, findMatch(), and placeholder * replacement. Only a few narrow points differ, supplied by the abstract hooks * below: - * - the static template container (templates()) + * - the brand's default template registry (newTemplateManager()) * - the two description rewrite maps (descriptionRegexMap()/descriptionRawPatternMap()) * - the response field carrying the human-readable text (fieldName(): * "description" for CNR, "message" for IBS) @@ -37,10 +37,13 @@ abstract class AbstractResponseTranslator { /** - * The brand's static template container (id => raw template string). - * @return array + * The brand's default template registry, used when the caller supplies none. + * + * A factory, not a shared instance: handing every caller the same object + * would put the process-global container back that RSRMID-2941 removed — + * one `addTemplate()` on it would be visible to every later translate(). */ - abstract protected static function templates(): array; + abstract protected static function newTemplateManager(): ResponseTemplateManagerInterface; /** * plain-string description keys for translation; keys are preg_quote'd before matching @@ -74,28 +77,34 @@ abstract protected static function hasMissingRequiredFields(string $raw): bool; * @param array $cmd requested API command * @param array{CONNECTION_URL?: string} $placeholders * @param string|null $error transport error, if any (see {@see AbstractClient::performRequest()}); non-null means $raw is unusable and the "httperror" template is substituted instead + * @param ResponseTemplateManagerInterface|null $templates registry to resolve template ids against; null uses the brand's built-ins (see {@see newTemplateManager()}) */ - public static function translate(string $raw, array $cmd, array $placeholders = [], ?string $error = null): string - { + public static function translate( + string $raw, + array $cmd, + array $placeholders = [], + ?string $error = null, + ?ResponseTemplateManagerInterface $templates = null + ): string { $newraw = $raw === '' || $raw === '0' ? "empty" : $raw; // Hint: Empty API Response (replace {CONNECTION_URL} later) - $templates = static::templates(); + $rawTemplates = ($templates ?? static::newTemplateManager())->getRawTemplates(); // Explicit call for a static template, or a declared transport failure - $templateId = self::resolveTemplateId($newraw, $error, $templates); + $templateId = self::resolveTemplateId($newraw, $error, $rawTemplates); if ($templateId !== null) { // don't use getTemplate as it leads to endless loop as of again // creating a response instance - $newraw = $templates[$templateId]; + $newraw = $rawTemplates[$templateId]; if ($error !== null && $error !== "") { $newraw = preg_replace("/\{HTTPERROR\}/", " (" . $error . ")", $newraw) ?? $newraw; } } // Missing or empty required field(s) in API response - if (static::hasMissingRequiredFields($newraw) && array_key_exists("invalid", $templates)) { - $newraw = $templates["invalid"]; + if (static::hasMissingRequiredFields($newraw) && array_key_exists("invalid", $rawTemplates)) { + $newraw = $rawTemplates["invalid"]; } // generic API response description rewrite @@ -126,18 +135,18 @@ public static function translate(string $raw, array $cmd, array $placeholders = * * A non-null $error resolves to "httperror", taking priority over $raw — * this is what replaced the former "httperror|" sentinel that used to be - * smuggled through $raw itself — but ONLY if $templates actually declares - * that id. templates() is an abstract hook: a third-party brand - * translator's container need not include "httperror" at all, and - * indexing $templates["httperror"] unconditionally would degrade an + * smuggled through $raw itself — but ONLY if $rawTemplates actually + * declares that id. The registry is caller-supplied: a third-party brand's + * need not include "httperror" at all, and indexing + * $rawTemplates["httperror"] unconditionally would degrade an * Undefined-array-key warning into a TypeError a few lines later. Null * here, same as any other unmatched id, keeps that a quiet fallback * instead of a crash — mirroring the "invalid" lookup a few lines down in * translate(), which is guarded by the same array_key_exists() shape. * - * Otherwise $raw is checked against $templates: a raw payload equal to a + * Otherwise $raw is checked against $rawTemplates: a raw payload equal to a * known template id (e.g. "empty", "invalid", or one registered via - * {@see AbstractResponseTemplateManager::addTemplate()}) is the sanctioned + * {@see ResponseTemplateManagerInterface::addTemplate()}) is the sanctioned * mocking route CLAUDE.md documents for tests — constructing a * Response/Translator call with the template id as $raw is how a test * exercises a specific canned response without a real API round-trip. @@ -145,18 +154,18 @@ public static function translate(string $raw, array $cmd, array $placeholders = * response that does not match any known id resolves to null and is used * as-is by the caller. * - * $templates is passed in rather than re-fetched via templates(): the + * $rawTemplates is passed in rather than re-read from the registry: the * caller already bound one snapshot for the rest of translate(), and a - * second call would resolve the id against a possibly different snapshot + * second read would resolve the id against a possibly different snapshot * than the one it is then dereferenced against. - * @param array $templates the caller's already-bound templates() snapshot + * @param array $rawTemplates the caller's already-bound registry snapshot */ - private static function resolveTemplateId(string $raw, ?string $error, array $templates): ?string + private static function resolveTemplateId(string $raw, ?string $error, array $rawTemplates): ?string { if ($error !== null) { - return array_key_exists("httperror", $templates) ? "httperror" : null; + return array_key_exists("httperror", $rawTemplates) ? "httperror" : null; } - return array_key_exists($raw, $templates) ? $raw : null; + return array_key_exists($raw, $rawTemplates) ? $raw : null; } /** diff --git a/src/CNR/Response.php b/src/CNR/Response.php index c01ba7a8..8d3a182e 100755 --- a/src/CNR/Response.php +++ b/src/CNR/Response.php @@ -18,6 +18,7 @@ use CNIC\ExtendedResponseInterface; use CNIC\Record; use CNIC\ResponseParserInterface; +use CNIC\ResponseTemplateManagerInterface; /** * CNR Response @@ -58,11 +59,17 @@ class Response extends AbstractResponse implements ExtendedResponseInterface * @param array $cmd API command used within this request * @param array{CONNECTION_URL?: string} $placeholders * @param string|null $error transport error, if any; non-null means $raw is unusable + * @param ResponseTemplateManagerInterface|null $templates registry to resolve template ids against; null uses CNR's built-ins */ #[\Override] - protected function translate(string $raw, array $cmd, array $placeholders, ?string $error = null): string - { - return RT::translate($raw, $cmd, $placeholders, $error); + protected function translate( + string $raw, + array $cmd, + array $placeholders, + ?string $error = null, + ?ResponseTemplateManagerInterface $templates = null + ): string { + return RT::translate($raw, $cmd, $placeholders, $error, $templates); } /** diff --git a/src/CNR/ResponseTemplateManager.php b/src/CNR/ResponseTemplateManager.php index 1ef55ada..8b5b06b8 100644 --- a/src/CNR/ResponseTemplateManager.php +++ b/src/CNR/ResponseTemplateManager.php @@ -23,10 +23,12 @@ final class ResponseTemplateManager extends AbstractResponseTemplateManager { /** - * Template container - * @var array + * CNR's built-in templates. A constant, not a property: each registry + * instance copies these at construction and mutates only its copy, so there + * is no route by which one caller's override reaches another (RSRMID-2941). + * @var array */ - public static array $templates = [ + private const array BUILTIN_TEMPLATES = [ "404" => "[RESPONSE]\r\nCODE=421\r\nDESCRIPTION=Page not found\r\nEOF\r\n", "500" => "[RESPONSE]\r\nCODE=500\r\nDESCRIPTION=Internal server error\r\nEOF\r\n", "empty" => "[RESPONSE]\r\nCODE=423\r\nDESCRIPTION=Empty API response. Probably unreachable API end point {CONNECTION_URL}\r\nEOF\r\n", @@ -38,38 +40,50 @@ final class ResponseTemplateManager extends AbstractResponseTemplateManager "unauthorized" => "[RESPONSE]\r\nCODE=530\r\nDESCRIPTION=Unauthorized\r\nEOF\r\n" ]; + /** + * @return array + */ + #[\Override] + protected static function builtinTemplates(): array + { + return self::BUILTIN_TEMPLATES; + } + /** * Generate API response template string for given code and description */ #[\Override] - public static function generateTemplate(string $code, string $description): string + public function generateTemplate(string $code, string $description): string { return "[RESPONSE]\r\nCODE=" . $code . "\r\nDESCRIPTION=" . $description . "\r\nEOF\r\n"; } /** - * Get response template instance from template container + * Get response template instance from this registry */ #[\Override] - public static function getTemplate(string $templateId): Response + public function getTemplate(string $templateId): Response { - return self::createResponse(self::hasTemplate($templateId) ? $templateId : "notfound"); + return $this->createResponse($this->hasTemplate($templateId) ? $templateId : "notfound"); } /** * Create a CNR Response instance from a template id or raw response. + * + * The registry is handed to the Response so a template id resolves against + * *this* object — that hand-off is what replaced the global lookup. */ #[\Override] - protected static function createResponse(string $raw): Response + protected function createResponse(string $raw): Response { - return new Response($raw); + return new Response($raw, templates: $this); } /** * Instantiate the CNR response parser. */ #[\Override] - protected static function newResponseParser(): ResponseParserInterface + protected function newResponseParser(): ResponseParserInterface { return new RP(); } diff --git a/src/CNR/ResponseTranslator.php b/src/CNR/ResponseTranslator.php index 62b8efa5..6558254d 100644 --- a/src/CNR/ResponseTranslator.php +++ b/src/CNR/ResponseTranslator.php @@ -11,6 +11,7 @@ use CNIC\AbstractResponseTranslator; use CNIC\CNR\ResponseTemplateManager as RTM; +use CNIC\ResponseTemplateManagerInterface; /** * CNR ResponseTranslator @@ -44,13 +45,12 @@ final class ResponseTranslator extends AbstractResponseTranslator ]; /** - * The CNR static template container. - * @return array + * A fresh CNR template registry holding the brand's built-ins. */ #[\Override] - protected static function templates(): array + protected static function newTemplateManager(): ResponseTemplateManagerInterface { - return RTM::$templates; + return new RTM(); } /** diff --git a/src/IBS/Response.php b/src/IBS/Response.php index 1f27e227..7d2f3130 100755 --- a/src/IBS/Response.php +++ b/src/IBS/Response.php @@ -16,6 +16,7 @@ use CNIC\Record; use CNIC\ResponseInterface; use CNIC\ResponseParserInterface; +use CNIC\ResponseTemplateManagerInterface; /** * IBS Response @@ -69,11 +70,17 @@ class Response extends AbstractResponse implements ResponseInterface * @param array $cmd API command used within this request * @param array{CONNECTION_URL?: string} $placeholders * @param string|null $error transport error, if any; non-null means $raw is unusable + * @param ResponseTemplateManagerInterface|null $templates registry to resolve template ids against; null uses IBS's built-ins */ #[\Override] - protected function translate(string $raw, array $cmd, array $placeholders, ?string $error = null): string - { - return RT::translate($raw, $cmd, $placeholders, $error); + protected function translate( + string $raw, + array $cmd, + array $placeholders, + ?string $error = null, + ?ResponseTemplateManagerInterface $templates = null + ): string { + return RT::translate($raw, $cmd, $placeholders, $error, $templates); } /** diff --git a/src/IBS/ResponseTemplateManager.php b/src/IBS/ResponseTemplateManager.php index 02643fda..b2d1eab5 100644 --- a/src/IBS/ResponseTemplateManager.php +++ b/src/IBS/ResponseTemplateManager.php @@ -23,10 +23,12 @@ final class ResponseTemplateManager extends AbstractResponseTemplateManager { /** - * template container - * @var array + * IBS's built-in templates. A constant, not a property: each registry + * instance copies these at construction and mutates only its copy, so there + * is no route by which one caller's override reaches another (RSRMID-2941). + * @var array */ - public static array $templates = [ + private const array BUILTIN_TEMPLATES = [ "403" => "status=FAILURE\r\nmessage=403 Forbidden\r\n", "404" => "status=FAILURE\r\nmessage=421 Page not found\r\n", "500" => "status=FAILURE\r\nmessage=500 Internal server error\r\n", @@ -38,39 +40,51 @@ final class ResponseTemplateManager extends AbstractResponseTemplateManager "unauthorized" => "status=FAILURE\r\nmessage=530 Unauthorized\r\n" ]; + /** + * @return array + */ + #[\Override] + protected static function builtinTemplates(): array + { + return self::BUILTIN_TEMPLATES; + } + /** * Generate API response template string for given status and description * @param string $code goes on the wire as IBS's `status` field */ #[\Override] - public static function generateTemplate(string $code, string $description): string + public function generateTemplate(string $code, string $description): string { return "status=$code\r\nmessage=$description\r\n"; } /** - * Get response template instance from template container + * Get response template instance from this registry */ #[\Override] - public static function getTemplate(string $templateId): Response + public function getTemplate(string $templateId): Response { - return self::createResponse(self::hasTemplate($templateId) ? $templateId : "notfound"); + return $this->createResponse($this->hasTemplate($templateId) ? $templateId : "notfound"); } /** * Create an IBS Response instance from a template id or raw response. + * + * The registry is handed to the Response so a template id resolves against + * *this* object — that hand-off is what replaced the global lookup. */ #[\Override] - protected static function createResponse(string $raw): Response + protected function createResponse(string $raw): Response { - return new Response($raw); + return new Response($raw, templates: $this); } /** * Instantiate the IBS response parser. */ #[\Override] - protected static function newResponseParser(): ResponseParserInterface + protected function newResponseParser(): ResponseParserInterface { return new RP(); } diff --git a/src/IBS/ResponseTranslator.php b/src/IBS/ResponseTranslator.php index 126a88ab..078fcccd 100644 --- a/src/IBS/ResponseTranslator.php +++ b/src/IBS/ResponseTranslator.php @@ -11,6 +11,7 @@ use CNIC\AbstractResponseTranslator; use CNIC\IBS\ResponseTemplateManager as RTM; +use CNIC\ResponseTemplateManagerInterface; /** * IBS ResponseTranslator @@ -37,13 +38,12 @@ final class ResponseTranslator extends AbstractResponseTranslator private const array DESCRIPTION_RAW_PATTERN_MAP = []; /** - * The IBS static template container. - * @return array + * A fresh IBS template registry holding the brand's built-ins. */ #[\Override] - protected static function templates(): array + protected static function newTemplateManager(): ResponseTemplateManagerInterface { - return RTM::$templates; + return new RTM(); } /** diff --git a/src/ResponseTemplateManagerInterface.php b/src/ResponseTemplateManagerInterface.php new file mode 100644 index 00000000..117d0e94 --- /dev/null +++ b/src/ResponseTemplateManagerInterface.php @@ -0,0 +1,106 @@ + + */ + public function getTemplates(): array; + + /** + * Every template in this registry as its raw wire text, keyed by template + * id — the snapshot {@see AbstractResponseTranslator::translate()} resolves + * ids against. + * + * Distinct from {@see getTemplates()} on purpose: the translator needs the + * strings, and building a Response per entry to translate one response + * would recurse. + * @return array + */ + public function getRawTemplates(): array; + + /** + * Whether the given API response hash matches a template held here, on this + * brand's two match keys (CNR: CODE/DESCRIPTION, IBS: status/message). + * @param array $responseHash + */ + public function isTemplateMatchHash(array $responseHash, string $templateId): bool; + + /** + * Whether the given API plain response matches a template held here, on + * this brand's two match keys. + * @param string $plain API plain response + */ + public function isTemplateMatchPlain(string $plain, string $templateId): bool; +} diff --git a/tests/AbstractResponseTranslatorFallbackTest.php b/tests/AbstractResponseTranslatorFallbackTest.php index 41c2a257..1556447e 100644 --- a/tests/AbstractResponseTranslatorFallbackTest.php +++ b/tests/AbstractResponseTranslatorFallbackTest.php @@ -9,43 +9,99 @@ namespace CNICTEST; +use CNIC\AbstractResponseTemplateManager; use CNIC\AbstractResponseTranslator; +use CNIC\Exception\UnsupportedFeatureException; +use CNIC\IBS\Response as IBSResponse; +use CNIC\IBS\ResponseParser as IBSParser; +use CNIC\ResponseInterface; +use CNIC\ResponseParserInterface; +use CNIC\ResponseTemplateManagerInterface; use PHPUnit\Framework\TestCase; /** * Regression coverage for AbstractResponseTranslator::resolveTemplateId()'s * "httperror" branch (RSRMID-2937 follow-up). * - * templates() is an abstract hook: a third-party brand translator's container - * need not declare an "httperror" entry at all. Indexing $templates["httperror"] - * unconditionally whenever $error !== null would turn a missing key into an - * Undefined-array-key warning immediately followed by a TypeError a few lines - * later (preg_replace() against a now-null $newraw) — a real regression from - * master, which only ever indexed a template after array_key_exists() - * succeeded and otherwise degraded quietly to the "invalid" template. + * The registry is caller-supplied: a third-party brand's need not declare an + * "httperror" entry at all. Indexing $rawTemplates["httperror"] unconditionally + * whenever $error !== null would turn a missing key into an Undefined-array-key + * warning immediately followed by a TypeError a few lines later (preg_replace() + * against a now-null $newraw) — 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 purpose-built fixture translator is used rather than mutating a real - * brand's public static $templates bag: that state has process lifetime and - * is shared across test classes (the exact defect RSRMID-2941 exists to - * fix), so mutating it here would make the suite order-dependent. + * A purpose-built fixture registry is used rather than a real brand's. Under + * the old design that was a hard requirement — the container was a public + * static bag with process lifetime, so mutating it here would have made the + * suite order-dependent (the defect RSRMID-2941 fixed). It stays a fixture for + * the reason that outlived the defect: what is under test is a translator whose + * registry is *missing* an id every real brand ships, which no brand registry + * can express. */ final class AbstractResponseTranslatorFallbackTest extends TestCase { public function testMissingHttperrorTemplateDegradesToInvalidInsteadOfWarningOrThrowing(): void { - $translator = new class extends AbstractResponseTranslator { + $templates = new class extends AbstractResponseTemplateManager { /** * Deliberately no "httperror" key. - * @return array + * @return array */ #[\Override] - protected static function templates(): array + protected static function builtinTemplates(): array { return [ "invalid" => "status=FAILURE\r\nmessage=423 Invalid API response. Contact Support\r\n", ]; } + #[\Override] + public function generateTemplate(string $code, string $description): string + { + return "status=$code\r\nmessage=$description\r\n"; + } + + #[\Override] + public function getTemplate(string $templateId): ResponseInterface + { + return $this->createResponse($this->hasTemplate($templateId) ? $templateId : "invalid"); + } + + #[\Override] + protected function createResponse(string $raw): ResponseInterface + { + return new IBSResponse($raw, templates: $this); + } + + #[\Override] + protected function newResponseParser(): ResponseParserInterface + { + return new IBSParser(); + } + + /** @return array{0: string, 1: string} */ + #[\Override] + protected static function matchKeys(): array + { + return ["status", "message"]; + } + }; + + $translator = new class extends AbstractResponseTranslator { + /** + * This fixture has no default registry on purpose — the one under + * test must arrive as translate()'s argument. Throwing here (rather + * than returning something plausible) is what makes a silent + * fallback to a default a failure instead of a passing test against + * the wrong registry. + */ + #[\Override] + protected static function newTemplateManager(): ResponseTemplateManagerInterface + { + throw new UnsupportedFeatureException("this fixture translator has no default registry"); + } + /** @return array */ #[\Override] protected static function descriptionRegexMap(): array @@ -73,11 +129,11 @@ protected static function hasMissingRequiredFields(string $raw): bool } }; - // A non-null $error would select "httperror" on a translator whose - // templates() declares it; this fixture does not, so resolveTemplateId() - // must resolve to null and fall through to hasMissingRequiredFields()/ - // "invalid" — exactly the path an ordinary unmatched $raw already takes. - $result = $translator::translate("some raw payload", [], [], "connection refused"); + // A non-null $error would select "httperror" on a registry that declares + // it; this fixture does not, so resolveTemplateId() must resolve to null + // and fall through to hasMissingRequiredFields()/"invalid" — exactly the + // path an ordinary unmatched $raw already takes. + $result = $translator::translate("some raw payload", [], [], "connection refused", $templates); $this->assertStringContainsString("423 Invalid API response. Contact Support", $result); // The error never reached a template, so it must not leak into the output either. diff --git a/tests/CNR/ClientTest.php b/tests/CNR/ClientTest.php index 017ce06e..2b34979d 100644 --- a/tests/CNR/ClientTest.php +++ b/tests/CNR/ClientTest.php @@ -761,7 +761,7 @@ public function testRequestNextResponsePageZeroLimit(): void // count/limit come back as 0 while total reflects the full list size. // Without the guard in requestNextResponsePage(), $first never advances // and requestAllResponsePages() would loop forever. - RTM::addTemplate( + $tpls = (new RTM())->addTemplate( "listLimitZero", "[RESPONSE]\r\nPROPERTY[COUNT][0]=0\r\nPROPERTY[FIRST][0]=0\r\nPROPERTY[LAST][0]=0\r\n" . "PROPERTY[LIMIT][0]=0\r\nPROPERTY[TOTAL][0]=1725494\r\n" @@ -771,7 +771,7 @@ public function testRequestNextResponsePageZeroLimit(): void "COMMAND" => "QueryDomainList", "FIRST" => "0", "LIMIT" => "0" - ]); + ], templates: $tpls); $this->assertTrue($r->isSuccess()); $this->assertSame(0, $r->getRecordsLimitation()); $this->assertSame(1725494, $r->getRecordsTotalCount()); @@ -785,7 +785,7 @@ public function testRequestNextResponsePageLastPage(): void // current page already holds the last rows, so there is no next page. // Response::hasNextPage() returns false here, and requestNextResponsePage() // must return null accordingly (termination logic is no longer duplicated). - RTM::addTemplate( + $tpls = (new RTM())->addTemplate( "listLastPage", "[RESPONSE]\r\nPROPERTY[COUNT][0]=2\r\nPROPERTY[FIRST][0]=8\r\nPROPERTY[LAST][0]=9\r\n" . "PROPERTY[LIMIT][0]=2\r\nPROPERTY[TOTAL][0]=10\r\n" @@ -795,7 +795,7 @@ public function testRequestNextResponsePageLastPage(): void "COMMAND" => "QueryDomainList", "FIRST" => "8", "LIMIT" => "2" - ]); + ], templates: $tpls); $this->assertTrue($r->isSuccess()); $this->assertFalse($r->hasNextPage()); $this->assertNull($r->getNextPageNumber()); @@ -996,12 +996,4 @@ public function testSortCommandParams(): void ]; $this->assertEquals($expected, $response->getCommand()); } - - #[\Override] - public static function tearDownAfterClass(): void - { - // Templates are process-wide static state — drop this class' own so - // they do not leak into later test classes (RSRMID-2924). - RTM::resetTemplates(); - } } diff --git a/tests/CNR/ResponseTemplateManagerTest.php b/tests/CNR/ResponseTemplateManagerTest.php index 512b60c6..636f952a 100644 --- a/tests/CNR/ResponseTemplateManagerTest.php +++ b/tests/CNR/ResponseTemplateManagerTest.php @@ -2,8 +2,6 @@ declare(strict_types=1); -//declare(strict_types=1); - namespace CNICTEST\CNR; use CNIC\CNR\Response as R; @@ -14,15 +12,16 @@ final class ResponseTemplateManagerTest extends TestCase { public function testGetTemplateNotFound(): void { - $tpl = RTM::getTemplate("IwontExist"); + $tpl = (new RTM())->getTemplate("IwontExist"); $this->assertEquals(500, $tpl->getCode()); $this->assertEquals("Response Template not found", $tpl->getDescription()); } public function testGetTemplates(): void { - $tpl = RTM::getTemplates(); - $keys = array_keys(RTM::$templates); + $rtm = new RTM(); + $tpl = $rtm->getTemplates(); + $keys = array_keys($rtm->getRawTemplates()); foreach ($keys as $key) { $this->assertArrayHasKey($key, $tpl); } @@ -31,41 +30,56 @@ public function testGetTemplates(): void public function testIsTemplateMatchHash(): void { $tpl = new R(""); - $this->assertEquals(true, RTM::isTemplateMatchHash($tpl->getHash(), "empty")); + $this->assertEquals(true, (new RTM())->isTemplateMatchHash($tpl->getHash(), "empty")); + } + + public function testIsTemplateMatchHashWithAMissingMatchKeyReturnsFalse(): void + { + // See the IBS twin: an incomplete hash must answer false rather than + // emit "Undefined array key" and compare null (RSRMID-2941). + $rtm = new RTM(); + $this->assertFalse($rtm->isTemplateMatchHash(["CODE" => "423"], "empty")); + $this->assertFalse($rtm->isTemplateMatchHash(["DESCRIPTION" => "whatever"], "empty")); + $this->assertFalse($rtm->isTemplateMatchHash([], "empty")); } public function testIsTemplateMatchPlain(): void { $tpl = new R(""); - $this->assertEquals(true, RTM::isTemplateMatchPlain($tpl->getPlain(), "empty")); + $this->assertEquals(true, (new RTM())->isTemplateMatchPlain($tpl->getPlain(), "empty")); } public function testAddTemplate(): void { // providing template in plain text + $rtm = new RTM(); $tplid = "custom404"; $descr = "Page not found"; $code = 421; - RTM::addTemplate($tplid, "[RESPONSE]\r\nCODE=$code\r\nDESCRIPTION=$descr\r\nEOF\r\n"); - $this->assertEquals(true, RTM::hasTemplate($tplid)); - $tpl = RTM::getTemplate($tplid); + $rtm->addTemplate($tplid, "[RESPONSE]\r\nCODE=$code\r\nDESCRIPTION=$descr\r\nEOF\r\n"); + $this->assertEquals(true, $rtm->hasTemplate($tplid)); + $tpl = $rtm->getTemplate($tplid); $this->assertEquals($code, $tpl->getCode()); $this->assertEquals($descr, $tpl->getDescription()); // providing template by code and description $tplid = "custom2_404"; - RTM::addTemplate($tplid, "$code", $descr); - $this->assertEquals(true, RTM::hasTemplate($tplid)); - $tpl = RTM::getTemplate($tplid); + $rtm->addTemplate($tplid, "$code", $descr); + $this->assertEquals(true, $rtm->hasTemplate($tplid)); + $tpl = $rtm->getTemplate($tplid); $this->assertEquals($code, $tpl->getCode()); $this->assertEquals($descr, $tpl->getDescription()); } - #[\Override] - public static function tearDownAfterClass(): void + public function testRegistriesDoNotShareRegisteredTemplates(): void { - // Templates are process-wide static state — drop this class' own so - // they do not leak into later test classes (RSRMID-2924). - RTM::resetTemplates(); + // No tearDownAfterClass here on purpose (RSRMID-2941): nothing this + // class registers reaches another, so there is nothing to reset. + $mine = (new RTM())->addTemplate("scoped", "200", "only mine"); + $theirs = new RTM(); + + $this->assertTrue($mine->hasTemplate("scoped")); + $this->assertFalse($theirs->hasTemplate("scoped")); + $this->assertTrue($theirs->hasTemplate("empty"), "the brand's built-ins are still there"); } } diff --git a/tests/CNR/ResponseTest.php b/tests/CNR/ResponseTest.php index 22eb5195..6e742463 100644 --- a/tests/CNR/ResponseTest.php +++ b/tests/CNR/ResponseTest.php @@ -21,11 +21,19 @@ final class ResponseTest extends TestCase */ public static string $pw; + /** + * This class' template registry. Instance state, so the templates below + * reach only the responses explicitly built against it (RSRMID-2941) — + * which is why there is no tearDownAfterClass() putting anything back. + */ + public static RTM $tpls; + #[\Override] public static function setUpBeforeClass(): void { - RTM::addTemplate("OK", "200", "Command completed successfully") - ::addTemplate("listP0", "[RESPONSE]\r\nPROPERTY[TOTAL][0]=2701\r\nPROPERTY[FIRST][0]=0\r\nPROPERTY[DOMAIN][0]=0-60motorcycletimes.com\r\nPROPERTY[DOMAIN][1]=0-be-s01-0.com\r\nPROPERTY[COUNT][0]=2\r\nPROPERTY[LAST][0]=1\r\nPROPERTY[LIMIT][0]=2\r\nDESCRIPTION=Command completed successfully\r\nCODE=200\r\nQUEUETIME=0\r\nRUNTIME=0.023\r\nEOF\r\n"); + self::$tpls = (new RTM()) + ->addTemplate("OK", "200", "Command completed successfully") + ->addTemplate("listP0", "[RESPONSE]\r\nPROPERTY[TOTAL][0]=2701\r\nPROPERTY[FIRST][0]=0\r\nPROPERTY[DOMAIN][0]=0-60motorcycletimes.com\r\nPROPERTY[DOMAIN][1]=0-be-s01-0.com\r\nPROPERTY[COUNT][0]=2\r\nPROPERTY[LAST][0]=1\r\nPROPERTY[LIMIT][0]=2\r\nDESCRIPTION=Command completed successfully\r\nCODE=200\r\nQUEUETIME=0\r\nRUNTIME=0.023\r\nEOF\r\n"); self::$user = (string) getenv("RTLDEV_MW_CI_USER_CNR"); self::$pw = (string) getenv("RTLDEV_MW_CI_USERPASSWORD_CNR"); } @@ -65,25 +73,25 @@ public function testCommandPlainSecureCaseInsensitive(): void public function testGetContext(): void { $context = ["traceId" => "abc123", "attempt" => 1]; - $r = new R("OK", [], [], $context); + $r = new R("OK", [], [], $context, templates: self::$tpls); $this->assertSame($context, $r->getContext()); } public function testGetCurrentPageNumberEntries(): void { - $r = new R("listP0"); + $r = new R("listP0", templates: self::$tpls); $this->assertEquals(1, $r->getCurrentPageNumber()); } public function testGetCurrentPageNumberNoEntries(): void { - $r = new R("OK"); + $r = new R("OK", templates: self::$tpls); $this->assertNull($r->getCurrentPageNumber()); } public function testGetFirstRecordIndexNoFirstNoRows(): void { - $r = new R("OK"); + $r = new R("OK", templates: self::$tpls); $this->assertNull($r->getFirstRecordIndex()); } @@ -103,27 +111,27 @@ public function testGetFirstRecordIndexNoFirstRows(): void public function testGetColumns(): void { - $r = new R("listP0"); + $r = new R("listP0", templates: self::$tpls); $cols = $r->getColumns(); $this->assertEquals(6, count($cols)); } public function testGetColumnIndexExists(): void { - $r = new R("listP0"); + $r = new R("listP0", templates: self::$tpls); $this->assertEquals("0-60motorcycletimes.com", $r->getColumnIndex("DOMAIN", 0)); } public function testGetColumnIndexNotExists(): void { - $r = new R("listP0"); + $r = new R("listP0", templates: self::$tpls); $data = $r->getColumnIndex("COLUMN_NOT_EXISTS", 0); $this->assertNull($data); } public function testGetColumnKeys(): void { - $r = new R("listP0"); + $r = new R("listP0", templates: self::$tpls); $colKeys = $r->getColumnKeys(); $this->assertCount(6, $colKeys); $this->assertContains("COUNT", $colKeys); @@ -137,7 +145,7 @@ public function testGetColumnKeys(): void public function testGetRecordRows(): void { - $r = new R("listP0"); + $r = new R("listP0", templates: self::$tpls); $rec = $r->getRecord(0); $this->assertNotNull($rec); $this->assertEquals([ @@ -152,13 +160,13 @@ public function testGetRecordRows(): void public function testGetRecordNoRows(): void { - $r = new R("OK"); + $r = new R("OK", templates: self::$tpls); $this->assertNull($r->getRecord(0)); } public function testGetListHash(): void { - $r = new R("listP0"); + $r = new R("listP0", templates: self::$tpls); $lh = $r->getListHash(); $this->assertCount(2, $lh["LIST"]); $this->assertEquals($lh["meta"]["columns"], $r->getColumnKeys(true)); @@ -215,7 +223,7 @@ public function testIterationYieldsEveryRecordInOrder(): void // The listP0 fixture holds two rows: the first carries the pagination // columns alongside DOMAIN, the second only DOMAIN. Iteration walks both // and stops — no cursor, no rewind (RSRMID-2939). - $r = new R("listP0"); + $r = new R("listP0", templates: self::$tpls); $rows = []; foreach ($r as $index => $rec) { @@ -229,7 +237,7 @@ public function testIterationYieldsEveryRecordInOrder(): void public function testGetPagination(): void { - $r = new R("listP0"); + $r = new R("listP0", templates: self::$tpls); $pager = $r->getPagination(); $this->assertArrayHasKey("COUNT", $pager); $this->assertArrayHasKey("CURRENTPAGE", $pager); @@ -246,7 +254,7 @@ public function testIterationIsRepeatableWithoutARewindStep(): void { // What the removed cursor could not do: walk the rows twice and get the // same rows both times, with nothing to reset in between (RSRMID-2939). - $r = new R("listP0"); + $r = new R("listP0", templates: self::$tpls); $first = []; foreach ($r as $rec) { @@ -263,31 +271,31 @@ public function testIterationIsRepeatableWithoutARewindStep(): void public function testHasNextPageNoRows(): void { - $r = new R("OK"); + $r = new R("OK", templates: self::$tpls); $this->assertEquals(false, $r->hasNextPage()); } public function testHasNextPageRows(): void { - $r = new R("listP0"); + $r = new R("listP0", templates: self::$tpls); $this->assertEquals(true, $r->hasNextPage()); } public function testHasPreviousPageNoRows1(): void { - $r = new R("OK"); + $r = new R("OK", templates: self::$tpls); $this->assertEquals(false, $r->hasPreviousPage()); } public function testHasPreviousPageNoRows2(): void { - $r = new R("listP0"); + $r = new R("listP0", templates: self::$tpls); $this->assertEquals(false, $r->hasPreviousPage()); } public function testGetLastRecordIndexNoRows(): void { - $r = new R("OK"); + $r = new R("OK", templates: self::$tpls); $this->assertNull($r->getLastRecordIndex()); } @@ -307,13 +315,13 @@ public function testGetLastRecordIndexNoLastRows(): void public function testGetNextPageNumberNoRows(): void { - $r = new R("OK"); + $r = new R("OK", templates: self::$tpls); $this->assertNull($r->getNextPageNumber()); } public function testGetNextPageNumberRows(): void { - $r = new R("listP0"); + $r = new R("listP0", templates: self::$tpls); $this->assertEquals(2, $r->getNextPageNumber()); } @@ -322,14 +330,17 @@ public function testGetNextPageNumberLastPage(): void // Single-page list (FIRST=0, LIMIT=10, TOTAL=2): the last page has no // next page, so getNextPageNumber() must honour the documented null // contract rather than clamping to the current page number. - RTM::addTemplate( + // Registered on a local registry, not the class-wide one: this template + // is needed by exactly one test, and widening its scope would be the + // shared-bag pattern in miniature (RSRMID-2941). + $tpls = (new RTM())->addTemplate( "listLastPage", "[RESPONSE]\r\nPROPERTY[TOTAL][0]=2\r\nPROPERTY[FIRST][0]=0\r\n" . "PROPERTY[DOMAIN][0]=example1.com\r\nPROPERTY[DOMAIN][1]=example2.com\r\n" . "PROPERTY[COUNT][0]=2\r\nPROPERTY[LAST][0]=1\r\nPROPERTY[LIMIT][0]=10\r\n" . "DESCRIPTION=Command completed successfully\r\nCODE=200\r\nQUEUETIME=0\r\nRUNTIME=0.023\r\nEOF\r\n" ); - $r = new R("listLastPage"); + $r = new R("listLastPage", templates: $tpls); $this->assertEquals(1, $r->getNumberOfPages()); $this->assertFalse($r->hasNextPage()); $this->assertNull($r->getNextPageNumber()); @@ -337,25 +348,25 @@ public function testGetNextPageNumberLastPage(): void public function testGetNumberOfPages(): void { - $r = new R("OK"); + $r = new R("OK", templates: self::$tpls); $this->assertEquals(0, $r->getNumberOfPages()); } public function testGetPreviousPageNumberNoRows(): void { - $r = new R("OK"); + $r = new R("OK", templates: self::$tpls); $this->assertNull($r->getPreviousPageNumber()); } public function testGetPreviousPageNumberRows(): void { - $r = new R("listP0"); + $r = new R("listP0", templates: self::$tpls); $this->assertNull($r->getPreviousPageNumber()); } public function testIteratingAResponseWithoutRecordsYieldsNothing(): void { - $r = new R("OK"); + $r = new R("OK", templates: self::$tpls); $this->assertSame([], iterator_to_array($r)); } @@ -428,12 +439,4 @@ public function testIsTmpError(): void $r = new R("[RESPONSE]\r\ncode=423\r\ndescription=Empty API response. Probably unreachable API end point\r\nEOF\r\n"); $this->assertEquals(true, $r->isTmpError()); } - - #[\Override] - public static function tearDownAfterClass(): void - { - // Templates are process-wide static state — drop this class' own so - // they do not leak into later test classes (RSRMID-2924). - RTM::resetTemplates(); - } } diff --git a/tests/CNR/ResponseTranslatorTest.php b/tests/CNR/ResponseTranslatorTest.php index fbb15e9b..8e697ab6 100644 --- a/tests/CNR/ResponseTranslatorTest.php +++ b/tests/CNR/ResponseTranslatorTest.php @@ -53,7 +53,7 @@ public function testLiteralBraceContentIsPreserved(): void public function testIsTemplateMatchHash(): void { $r = new R(""); - $this->assertTrue(RTM::isTemplateMatchHash($r->getHash(), "empty")); + $this->assertTrue((new RTM())->isTemplateMatchHash($r->getHash(), "empty")); } /** @@ -62,7 +62,7 @@ public function testIsTemplateMatchHash(): void public function testIsTemplateMatchPlain(): void { $r = new R(""); - $this->assertTrue(RTM::isTemplateMatchPlain($r->getPlain(), "empty")); + $this->assertTrue((new RTM())->isTemplateMatchPlain($r->getPlain(), "empty")); } /** diff --git a/tests/IBS/ResponseTemplateManagerTest.php b/tests/IBS/ResponseTemplateManagerTest.php index 8549cb28..9bb6de0d 100644 --- a/tests/IBS/ResponseTemplateManagerTest.php +++ b/tests/IBS/ResponseTemplateManagerTest.php @@ -12,15 +12,16 @@ final class ResponseTemplateManagerTest extends TestCase { public function testGetTemplateNotFound(): void { - $tpl = RTM::getTemplate("IwontExist"); + $tpl = (new RTM())->getTemplate("IwontExist"); $this->assertEquals("FAILURE", $tpl->getHash()["status"] ?? null); $this->assertEquals("500 Response Template not found", $tpl->getDescription()); } public function testGetTemplates(): void { - $tpl = RTM::getTemplates(); - foreach (array_keys(RTM::$templates) as $key) { + $rtm = new RTM(); + $tpl = $rtm->getTemplates(); + foreach (array_keys($rtm->getRawTemplates()) as $key) { $this->assertArrayHasKey($key, $tpl); } } @@ -29,35 +30,50 @@ public function testGenerateTemplate(): void { $this->assertSame( "status=SUCCESS\r\nmessage=Command completed successfully\r\n", - RTM::generateTemplate("SUCCESS", "Command completed successfully") + (new RTM())->generateTemplate("SUCCESS", "Command completed successfully") ); } public function testHasTemplate(): void { - $this->assertTrue(RTM::hasTemplate("empty")); - $this->assertFalse(RTM::hasTemplate("IwontExist")); + $rtm = new RTM(); + $this->assertTrue($rtm->hasTemplate("empty")); + $this->assertFalse($rtm->hasTemplate("IwontExist")); } public function testIsTemplateMatchHash(): void { + $rtm = new RTM(); $r = new R(""); - $this->assertTrue(RTM::isTemplateMatchHash($r->getHash(), "empty")); + $this->assertTrue($rtm->isTemplateMatchHash($r->getHash(), "empty")); // non-matching hash returns false - $this->assertFalse(RTM::isTemplateMatchHash( + $this->assertFalse($rtm->isTemplateMatchHash( ["status" => "SUCCESS", "message" => "Command completed successfully"], "empty" )); } + public function testIsTemplateMatchHashWithAMissingMatchKeyReturnsFalse(): void + { + // A hash short of one of the brand's two match keys is ordinary caller + // input, not a programming error: this used to index both hashes + // unguarded and emit "Undefined array key" before comparing null + // (RSRMID-2941). Both directions matter — a missing key on either side. + $rtm = new RTM(); + $this->assertFalse($rtm->isTemplateMatchHash(["status" => "FAILURE"], "empty")); + $this->assertFalse($rtm->isTemplateMatchHash(["message" => "whatever"], "empty")); + $this->assertFalse($rtm->isTemplateMatchHash([], "empty")); + } + public function testIsTemplateMatchPlain(): void { + $rtm = new RTM(); $r = new R(""); - $this->assertTrue(RTM::isTemplateMatchPlain($r->getPlain(), "empty")); + $this->assertTrue($rtm->isTemplateMatchPlain($r->getPlain(), "empty")); // non-matching plain response returns false - $this->assertFalse(RTM::isTemplateMatchPlain( + $this->assertFalse($rtm->isTemplateMatchPlain( "status=SUCCESS\r\nmessage=Command completed successfully\r\n", "empty" )); @@ -71,44 +87,57 @@ public function testIsTemplateMatchPlainAgreesWithAResponseParsedOnTheOtherBranc // passing ResponseFormat=JSON here would put both on the same branch and // prove nothing. Matching a template must not depend on which branch // produced the hash. (Ref: RSRMID-2924.) + $rtm = new RTM(); $r = new R("", ["Command" => "DomainInfo"]); - $this->assertTrue(RTM::isTemplateMatchHash($r->getHash(), "empty")); - $this->assertTrue(RTM::isTemplateMatchPlain($r->getPlain(), "empty")); + $this->assertTrue($rtm->isTemplateMatchHash($r->getHash(), "empty")); + $this->assertTrue($rtm->isTemplateMatchPlain($r->getPlain(), "empty")); } public function testAddTemplate(): void { // providing template in plain text + $rtm = new RTM(); $tplid = "custom403"; - RTM::addTemplate($tplid, "status=FAILURE\r\nmessage=Forbidden\r\n"); - $this->assertTrue(RTM::hasTemplate($tplid)); - $tpl = RTM::getTemplate($tplid); + $rtm->addTemplate($tplid, "status=FAILURE\r\nmessage=Forbidden\r\n"); + $this->assertTrue($rtm->hasTemplate($tplid)); + $tpl = $rtm->getTemplate($tplid); $this->assertEquals("FAILURE", $tpl->getHash()["status"] ?? null); $this->assertEquals("Forbidden", $tpl->getDescription()); // providing template by status and description $tplid = "custom2_403"; - RTM::addTemplate($tplid, "FAILURE", "Forbidden"); - $this->assertTrue(RTM::hasTemplate($tplid)); - $tpl = RTM::getTemplate($tplid); + $rtm->addTemplate($tplid, "FAILURE", "Forbidden"); + $this->assertTrue($rtm->hasTemplate($tplid)); + $tpl = $rtm->getTemplate($tplid); $this->assertEquals("FAILURE", $tpl->getHash()["status"] ?? null); $this->assertEquals("Forbidden", $tpl->getDescription()); } - public function testAddTemplateReturnsSelfForChaining(): void + public function testAddTemplateReturnsTheSameInstanceForChaining(): void { - $this->assertInstanceOf(RTM::class, RTM::addTemplate("chainA", "FAILURE", "A")); - - RTM::addTemplate("chainB", "FAILURE", "B")::addTemplate("chainC", "FAILURE", "C"); - $this->assertTrue(RTM::hasTemplate("chainB")); - $this->assertTrue(RTM::hasTemplate("chainC")); + // The static predecessor returned `new static()` — a throwaway instance + // of an all-static class, so the return value was fluent in shape only. + // Now it must be the very object that received the template, or a chain + // would register onto something the caller never sees (RSRMID-2941). + $rtm = new RTM(); + $this->assertSame($rtm, $rtm->addTemplate("chainA", "FAILURE", "A")); + + $rtm->addTemplate("chainB", "FAILURE", "B")->addTemplate("chainC", "FAILURE", "C"); + $this->assertTrue($rtm->hasTemplate("chainB")); + $this->assertTrue($rtm->hasTemplate("chainC")); } - #[\Override] - public static function tearDownAfterClass(): void + public function testRegistriesDoNotShareRegisteredTemplates(): void { - // Templates are process-wide static state — drop this class' own so - // they do not leak into later test classes (RSRMID-2924). - RTM::resetTemplates(); + // The point of RSRMID-2941: registering a template must not be visible + // to anyone who did not ask for it. There is no tearDown here on + // purpose — nothing this class registers can outlive its own instances, + // which is exactly what the deleted resetTemplates() used to paper over. + $mine = (new RTM())->addTemplate("scoped", "FAILURE", "only mine"); + $theirs = new RTM(); + + $this->assertTrue($mine->hasTemplate("scoped")); + $this->assertFalse($theirs->hasTemplate("scoped")); + $this->assertTrue($theirs->hasTemplate("empty"), "the brand's built-ins are still there"); } } diff --git a/tests/IBS/ResponseTest.php b/tests/IBS/ResponseTest.php index 93e4c31c..08936b41 100644 --- a/tests/IBS/ResponseTest.php +++ b/tests/IBS/ResponseTest.php @@ -46,11 +46,12 @@ public function testHttpErrorTemplate(): void $this->assertStringContainsString("Connection timed out", $r->getDescription()); } - public function testStaticTemplateLookupByRawId(): void + public function testTemplateLookupByRawId(): void { // A raw payload equal to a known template id is the sanctioned - // ResponseTemplateManager::addTemplate() mocking route (see - // AbstractResponseTranslator::resolveTemplateId()), not a leak. + // mocking route (see AbstractResponseTranslator::resolveTemplateId()), + // not a leak. "notfound" is a built-in, so it resolves against the + // brand default registry with nothing to register. $r = new R("notfound"); $this->assertTrue($r->isError()); $this->assertEquals("FAILURE", $r->getHash()["status"] ?? null); diff --git a/tests/InterfaceCoverageSeamTest.php b/tests/InterfaceCoverageSeamTest.php index 2fb386fd..5d7fff80 100644 --- a/tests/InterfaceCoverageSeamTest.php +++ b/tests/InterfaceCoverageSeamTest.php @@ -8,6 +8,7 @@ use CNIC\CNR\Logger as CNRLogger; use CNIC\CNR\Response as CNRResponse; use CNIC\CNR\ResponseParser as CNRResponseParser; +use CNIC\CNR\ResponseTemplateManager as CNRTemplates; use CNIC\Column; use CNIC\ColumnInterface; use CNIC\EchoSink; @@ -15,12 +16,14 @@ use CNIC\IBS\Logger as IBSLogger; use CNIC\IBS\Response as IBSResponse; use CNIC\IBS\ResponseParser as IBSResponseParser; +use CNIC\IBS\ResponseTemplateManager as IBSTemplates; use CNIC\LoggerInterface; use CNIC\LogSinkInterface; use CNIC\Record; use CNIC\RecordInterface; use CNIC\ResponseInterface; use CNIC\ResponseParserInterface; +use CNIC\ResponseTemplateManagerInterface; use CNIC\TransportInterface; use FilesystemIterator; use PHPUnit\Framework\TestCase; @@ -56,11 +59,12 @@ * {@see ResponsePaginationSeamTest}. Reflection comparing the implementation * against the interface is therefore the only instrument that can. * - * Only 7 interfaces are swept as **total contracts** — ones meant to fully + * Only 8 interfaces are swept as **total contracts** — ones meant to fully * describe their implementors: {@see \CNIC\ResponseInterface}, * {@see \CNIC\RecordInterface}, {@see \CNIC\ColumnInterface}, * {@see \CNIC\TransportInterface}, {@see \CNIC\ResponseParserInterface}, - * {@see \CNIC\LoggerInterface}, {@see \CNIC\LogSinkInterface}. + * {@see \CNIC\LoggerInterface}, {@see \CNIC\LogSinkInterface}, + * {@see \CNIC\ResponseTemplateManagerInterface}. * {@see \CNIC\ExtendedResponseInterface} and * {@see \CNIC\RoleCredentialsInterface} are deliberately excluded from that * role — they are additive capability interfaces (CLAUDE.md, "Core vs. @@ -85,13 +89,13 @@ * nothing left for this sweep to add there. * * The allow-list below is intentionally empty. After RSRMID-2927 removed - * `IBS\Response::getStatus()` the sweep is green across all 7 contracts with + * `IBS\Response::getStatus()` the sweep is green across all 8 contracts with * zero exceptions — keep it that way rather than adding an exception "just in * case"; a stray or widened method found here is a defect to fix, not a * reason to grow the list. * * Revisit this guard only if a genuinely new "total" interface is introduced - * (add it to {@see self::TOTAL_INTERFACES}) or if one of the current 7 stops + * (add it to {@see self::TOTAL_INTERFACES}) or if one of the current 8 stops * being meant to fully describe its implementors (move it to the excluded * set alongside `ExtendedResponseInterface`/`RoleCredentialsInterface`, with * the same justification these carry). @@ -108,7 +112,7 @@ * find a subject at all. * {@see self::testTheSweepActuallyExaminesTheKnownImplementors()} closes it * by pinning that discovery still finds a fixed, independently-verified set - * of 11 real implementors (see {@see self::KNOWN_TOTAL_IMPLEMENTORS}) and + * of 13 real implementors (see {@see self::KNOWN_TOTAL_IMPLEMENTORS}) and * still walks a plausible number of files under `src/`. It asserts * **containment**, not equality — {@see self::KNOWN_TOTAL_IMPLEMENTORS} is a * floor, not a snapshot — so a newly added brand class is swept @@ -142,6 +146,7 @@ final class InterfaceCoverageSeamTest extends TestCase ResponseParserInterface::class, LoggerInterface::class, LogSinkInterface::class, + ResponseTemplateManagerInterface::class, ]; /** @@ -168,12 +173,14 @@ final class InterfaceCoverageSeamTest extends TestCase CNRLogger::class, CNRResponse::class, CNRResponseParser::class, + CNRTemplates::class, Column::class, EchoSink::class, HttpTransport::class, IBSLogger::class, IBSResponse::class, IBSResponseParser::class, + IBSTemplates::class, Record::class, ]; diff --git a/tests/MONIKER/ResponseTest.php b/tests/MONIKER/ResponseTest.php index 50ab7170..abca9389 100644 --- a/tests/MONIKER/ResponseTest.php +++ b/tests/MONIKER/ResponseTest.php @@ -15,15 +15,16 @@ final class ResponseTest extends TestCase public function testGetTemplateNotFound(): void { - $tpl = RTM::getTemplate("IwontExist"); + $tpl = (new RTM())->getTemplate("IwontExist"); $this->assertEquals("FAILURE", $tpl->getHash()["status"] ?? null); $this->assertEquals("500 Response Template not found", $tpl->getDescription()); } public function testGetTemplates(): void { - $tpl = RTM::getTemplates(); - foreach (array_keys(RTM::$templates) as $key) { + $rtm = new RTM(); + $tpl = $rtm->getTemplates(); + foreach (array_keys($rtm->getRawTemplates()) as $key) { $this->assertArrayHasKey($key, $tpl); } } @@ -31,30 +32,31 @@ public function testGetTemplates(): void public function testIsTemplateMatchHash(): void { $r = new R(""); - $this->assertTrue(RTM::isTemplateMatchHash($r->getHash(), "empty")); + $this->assertTrue((new RTM())->isTemplateMatchHash($r->getHash(), "empty")); } public function testIsTemplateMatchPlain(): void { $r = new R(""); - $this->assertTrue(RTM::isTemplateMatchPlain($r->getPlain(), "empty")); + $this->assertTrue((new RTM())->isTemplateMatchPlain($r->getPlain(), "empty")); } public function testAddTemplate(): void { // providing template in plain text + $rtm = new RTM(); $tplid = "custom403"; - RTM::addTemplate($tplid, "status=FAILURE\r\nmessage=Forbidden\r\n"); - $this->assertTrue(RTM::hasTemplate($tplid)); - $tpl = RTM::getTemplate($tplid); + $rtm->addTemplate($tplid, "status=FAILURE\r\nmessage=Forbidden\r\n"); + $this->assertTrue($rtm->hasTemplate($tplid)); + $tpl = $rtm->getTemplate($tplid); $this->assertEquals("FAILURE", $tpl->getHash()["status"] ?? null); $this->assertEquals("Forbidden", $tpl->getDescription()); // providing template by status and description $tplid = "custom2_403"; - RTM::addTemplate($tplid, "FAILURE", "Forbidden"); - $this->assertTrue(RTM::hasTemplate($tplid)); - $tpl = RTM::getTemplate($tplid); + $rtm->addTemplate($tplid, "FAILURE", "Forbidden"); + $this->assertTrue($rtm->hasTemplate($tplid)); + $tpl = $rtm->getTemplate($tplid); $this->assertEquals("FAILURE", $tpl->getHash()["status"] ?? null); $this->assertEquals("Forbidden", $tpl->getDescription()); } @@ -164,11 +166,12 @@ public function testJsonDomainInfoResponse(): void $this->assertEquals("ns1.ispapi.net", $nameserver[0]); } - public function testStaticTemplateLookupByRawId(): void + public function testTemplateLookupByRawId(): void { // A raw payload equal to a known template id is the sanctioned - // ResponseTemplateManager::addTemplate() mocking route (see - // AbstractResponseTranslator::resolveTemplateId()), not a leak. + // mocking route (see AbstractResponseTranslator::resolveTemplateId()), + // not a leak. "notfound" is a built-in, so it resolves against the + // brand default registry with nothing to register. $r = new R("notfound"); $this->assertTrue($r->isError()); $this->assertEquals("FAILURE", $r->getHash()["status"] ?? null); @@ -183,12 +186,4 @@ public function testEmptyResponseWithJsonCommand(): void $this->assertEquals("FAILURE", $r->getHash()["status"] ?? null); $this->assertStringContainsString("Empty API response", $r->getDescription()); } - - #[\Override] - public static function tearDownAfterClass(): void - { - // Templates are process-wide static state — drop this class' own so - // they do not leak into later test classes (RSRMID-2924). - RTM::resetTemplates(); - } } diff --git a/tests/ResponseParserSeamTest.php b/tests/ResponseParserSeamTest.php index 79cbb594..0399d5a3 100644 --- a/tests/ResponseParserSeamTest.php +++ b/tests/ResponseParserSeamTest.php @@ -204,47 +204,16 @@ public function testTemplateManagersGoThroughTheSameHookAndNotAParseHelper(): vo method_exists(AbstractResponseTemplateManager::class, "parseResponse"), "parseResponse() was replaced by the newResponseParser() hook — a second route would drift again" ); - foreach ([CNRTemplates::class, IBSTemplates::class] as $class) { - $m = new ReflectionMethod($class, "newResponseParser"); - $this->assertSame($class, $m->getDeclaringClass()->getName()); - $this->assertInstanceOf(ResponseParserInterface::class, $m->invoke(null)); + foreach ([new CNRTemplates(), new IBSTemplates()] as $manager) { + $m = new ReflectionMethod($manager, "newResponseParser"); + $this->assertSame($manager::class, $m->getDeclaringClass()->getName()); + $this->assertInstanceOf(ResponseParserInterface::class, $m->invoke($manager)); } } - public function testResetTemplatesRestoresTheBuiltInsAndDropsRegisteredOnes(): void - { - // The container is public static state with process lifetime, so a - // template registered by one test class was visible to every later one. - $builtin = IBSTemplates::$templates; - IBSTemplates::addTemplate("seamLeak", "FAILURE", "leaked"); - $this->assertTrue(IBSTemplates::hasTemplate("seamLeak")); - - IBSTemplates::resetTemplates(); - - $this->assertFalse(IBSTemplates::hasTemplate("seamLeak")); - $this->assertSame($builtin, IBSTemplates::$templates); - $this->assertTrue(IBSTemplates::hasTemplate("empty"), "the brand's own templates must survive a reset"); - } - - public function testResetTemplatesIsPerBrand(): void - { - CNRTemplates::addTemplate("seamOnlyCNR", "200", "cnr only"); - IBSTemplates::addTemplate("seamOnlyIBS", "SUCCESS", "ibs only"); - - IBSTemplates::resetTemplates(); - - $this->assertTrue(CNRTemplates::hasTemplate("seamOnlyCNR"), "one brand's reset must not clear another's"); - $this->assertFalse(IBSTemplates::hasTemplate("seamOnlyIBS")); - - CNRTemplates::resetTemplates(); - $this->assertFalse(CNRTemplates::hasTemplate("seamOnlyCNR")); - } - - #[\Override] - public static function tearDownAfterClass(): void - { - // Same rule this file documents (RSRMID-2924). - CNRTemplates::resetTemplates(); - IBSTemplates::resetTemplates(); - } + // The two resetTemplates() cases that used to close this file are gone with + // the state they described: the template container is no longer process-wide + // static, so there is nothing for a tearDownAfterClass() to put back + // (RSRMID-2941). Their replacement — that a registry's contents are scoped to + // the object holding it — is tests/ResponseTemplateRegistrySeamTest.php. } diff --git a/tests/ResponseTemplateRegistrySeamTest.php b/tests/ResponseTemplateRegistrySeamTest.php new file mode 100644 index 00000000..1c4f2cd1 --- /dev/null +++ b/tests/ResponseTemplateRegistrySeamTest.php @@ -0,0 +1,220 @@ +addTemplate("seamScoped", "200", "scoped to mine"); + + $this->assertSame("scoped to mine", (new CNRResponse("seamScoped", templates: $mine))->getDescription()); + $this->assertNotSame("scoped to mine", (new CNRResponse("seamScoped"))->getDescription()); + } + + public function testAnIBSRegisteredTemplateIsVisibleOnlyToTheRegistryThatReceivedIt(): void + { + $mine = (new IBSTemplates())->addTemplate("seamScoped", "SUCCESS", "scoped to mine"); + + $this->assertSame("scoped to mine", (new IBSResponse("seamScoped", templates: $mine))->getDescription()); + $this->assertNotSame("scoped to mine", (new IBSResponse("seamScoped"))->getDescription()); + } + + public function testTwoRegistriesOfTheSameBrandDoNotSeeEachOther(): void + { + $a = (new IBSTemplates())->addTemplate("seamA", "FAILURE", "a"); + $b = new IBSTemplates(); + + $this->assertTrue($a->hasTemplate("seamA")); + $this->assertFalse($b->hasTemplate("seamA"), "a registry must not observe another's registrations"); + $this->assertTrue($b->hasTemplate("empty"), "the brand's built-ins are seeded into every instance"); + } + + public function testTheBuiltInsCannotBeReachedOrRewrittenThroughAnInstance(): void + { + // Mutating one instance to exhaustion must leave the next one pristine. + // This is what a `static` container would fail: there, overwriting a + // built-in id would change what every later instance starts from. + $vandal = new IBSTemplates(); + foreach (array_keys($vandal->getRawTemplates()) as $id) { + $vandal->addTemplate((string)$id, "FAILURE", "vandalised"); + } + + $fresh = new IBSTemplates(); + $this->assertSame( + (new IBSTemplates())->getRawTemplates(), + $fresh->getRawTemplates(), + "the built-ins a new registry starts from must not be reachable for writing" + ); + $this->assertStringNotContainsString("vandalised", $fresh->getTemplate("empty")->getDescription()); + } + + public function testNoBrandRegistryHoldsAStaticTemplateContainer(): void + { + // The literal shape RSRMID-2941 removed. Any static property here — of + // any visibility, under any name — is process-lifetime state shared by + // every instance, which is the defect regardless of what it is called. + foreach ([AbstractResponseTemplateManager::class, CNRTemplates::class, IBSTemplates::class] as $class) { + foreach ((new ReflectionClass($class))->getProperties() as $property) { + $this->assertFalse( + $property->isStatic(), + "{$class}::\${$property->getName()} is static — the registry must be instance state" + ); + } + } + } + + public function testTheRegistryReachesTheTranslatorAsAnArgumentAndNotThroughAHook(): void + { + // translate() must *take* the registry. A brand that reads one from + // anywhere else has reopened the global route, so the parameter is + // pinned by position, name, type and optionality: dropping it, renaming + // it, or making it required all fail here. (Named-argument callers bind + // to the implementation's parameter name, so the name is contract.) + foreach ([AbstractResponseTranslator::class, CNRResponse::class, IBSResponse::class] as $class) { + $parameters = (new ReflectionMethod($class, "translate"))->getParameters(); + $last = end($parameters); + + $this->assertNotFalse($last); + $this->assertSame("templates", $last->getName(), "{$class}::translate() must take a \$templates argument"); + $this->assertTrue($last->isOptional(), "the registry must stay optional — brand built-ins are the default"); + + $type = $last->getType(); + $this->assertInstanceOf(ReflectionNamedType::class, $type); + $this->assertSame(ResponseTemplateManagerInterface::class, $type->getName()); + $this->assertTrue($type->allowsNull()); + } + } + + public function testTheResponseConstructorForwardsTheRegistryItWasGiven(): void + { + // Behavioural counterpart to the reflection above: it is not enough for + // the parameter to exist, the constructor has to actually pass it down. + // A `translate()` that ignores its $templates argument would satisfy + // every structural assertion in this file and fail this one. + $registry = (new CNRTemplates())->addTemplate("seamForwarded", "421", "forwarded to the translator"); + + $this->assertSame( + "forwarded to the translator", + (new CNRResponse("seamForwarded", templates: $registry))->getDescription() + ); + $this->assertSame(421, (new CNRResponse("seamForwarded", templates: $registry))->getCode()); + } + + public function testMatchingAnIncompleteHashAnswersFalseWithoutReadingAMissingKey(): void + { + // The latent bug RSRMID-2941 names: matches() indexed both hashes with + // no existence check, so an incomplete response hash emitted "Undefined + // array key" and then compared null. + // + // Asserting only the false return would be vacuous — the unguarded + // version *also* returns false, because null !== "423". The defect is + // the diagnostic, and .github/phpunit.xml sets no failOnWarning, so a + // plain assertion would let the whole thing scroll past at exit code 0. + // Hence the handler: the notice has to become the failure. + foreach ([new CNRTemplates(), new IBSTemplates()] as $registry) { + /** @var string[] $raised */ + $raised = []; + set_error_handler(static function (int $severity, string $message) use (&$raised): bool { + $raised[] = $message; + return true; + }); + try { + $matched = $registry->isTemplateMatchHash(["only" => "one key"], "empty"); + } finally { + restore_error_handler(); + } + + $this->assertFalse($matched); + $this->assertSame([], $raised, $registry::class . " read a key the response hash does not carry"); + } + } + + public function testMatchingAnIncompletePlainResponseTakesTheSameGuardedPath(): void + { + // isTemplateMatchPlain() reaches the same comparison through the brand + // parser, so a payload that parses short of a match key must answer the + // same way. Covered separately because the hash case cannot reach the + // parser, and a future fix applied to only one entry point would leave + // this one emitting the notice again. + foreach ([new CNRTemplates(), new IBSTemplates()] as $registry) { + /** @var string[] $raised */ + $raised = []; + set_error_handler(static function (int $severity, string $message) use (&$raised): bool { + $raised[] = $message; + return true; + }); + try { + $matched = $registry->isTemplateMatchPlain("nothing=here\r\n", "empty"); + } finally { + restore_error_handler(); + } + + $this->assertFalse($matched); + $this->assertSame([], $raised, $registry::class . " read a key the parsed response does not carry"); + } + } + + public function testResetTemplatesIsGoneAndMustNotComeBack(): void + { + // Its only reason to exist was state outliving its user. A registry + // that needs resetting is a registry someone else can see. + $this->assertFalse( + method_exists(AbstractResponseTemplateManager::class, "resetTemplates"), + "resetTemplates() undid a leak that no longer exists — reinstating it means the leak is back" + ); + } +} From dd26da269b24c3987ff6f2c0740be7470acbc874 Mon Sep 17 00:00:00 2001 From: Kai Schwarz Date: Mon, 10 Aug 2026 10:26:47 +0200 Subject: [PATCH 2/9] test(phpunit): fail the run on warnings, notices and risky results MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit .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 --- .claude/agents/implementer.md | 2 +- .github/phpunit.xml | 3 +++ CLAUDE.md | 16 ++++++++-------- CONTRIBUTING.md | 2 ++ docs/agents/testing.md | 11 +++++++++++ 5 files changed, 25 insertions(+), 9 deletions(-) diff --git a/.claude/agents/implementer.md b/.claude/agents/implementer.md index 1bb2403c..c4a733e1 100644 --- a/.claude/agents/implementer.md +++ b/.claude/agents/implementer.md @@ -16,7 +16,7 @@ Project rules live in `CLAUDE.md`; read it. The traps that matter most here: - Do not add dependencies. Do not add mocking frameworks — register canned responses on a `ResponseTemplateManager` **instance** and pass it in (`new Response($id, templates: (new RTM())->addTemplate(…))`), or use the existing spies. The static `RTM::addTemplate()` form is gone (RSRMID-2941); do not reintroduce a static template container. - `MIGRATION.md` and `docs/agents/architecture.md` are only touched for a genuine `BREAKING CHANGE:`, which is a main-thread decision, not yours. -Before reporting done, run `composer lint` and `composer test` and let the results stand. Note `.github/phpunit.xml` sets `stopOnDefect="true"` — a green run can mean the suite stopped early, so check how many tests actually executed. +Before reporting done, run `composer lint` and `composer test` and let the results stand. `.github/phpunit.xml` sets `stopOnDefect="true"` alongside `failOnWarning`/`failOnNotice`/`failOnRisky`, so every category that halts the run also fails it (RSRMID-2964) — a green run is now a complete one. A red run stops at the first defect, so the remaining count says nothing about what else is broken. **Do not commit or push** unless the task explicitly says to. diff --git a/.github/phpunit.xml b/.github/phpunit.xml index 65c4c51b..5c4d563d 100644 --- a/.github/phpunit.xml +++ b/.github/phpunit.xml @@ -9,6 +9,9 @@ displayDetailsOnTestsThatTriggerErrors="true" displayDetailsOnTestsThatTriggerNotices="true" displayDetailsOnTestsThatTriggerWarnings="true" + failOnWarning="true" + failOnNotice="true" + failOnRisky="true" stopOnDefect="true"> diff --git a/CLAUDE.md b/CLAUDE.md index d816aae3..7a670661 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -130,14 +130,14 @@ Short reminders; full detail in the linked docs. Class inventory is derivable from `src/` in a couple of greps and is deliberately not tabulated here (see [architecture.md](docs/agents/architecture.md)). The paths worth knowing because they are **not** guessable: -| Path | Purpose | -| ------------------------------------------------------------------------ | ------------------------------------------------------- | -| `.github/linters/{phpcs.xml,phpstan.neon,psalm.xml,rector.php}` | Linter/analyser/modernization configs | -| `.github/phpunit.xml` | PHPUnit configuration (note `stopOnDefect="true"`) | -| `tests//cassettes/` | Committed `request()` cassettes (replay is the default) | -| `env.example.sh` | Template for required env variables (copy to `env.sh`) | -| `src/Exception/CnicException.php` | Base of the additive `CNIC\Exception` hierarchy | -| `src/{ResponseInterface,ResponseParserInterface,TransportInterface}.php` | The seams brands and tests substitute through | +| Path | Purpose | +| ------------------------------------------------------------------------ | -------------------------------------------------------------- | +| `.github/linters/{phpcs.xml,phpstan.neon,psalm.xml,rector.php}` | Linter/analyser/modernization configs | +| `.github/phpunit.xml` | PHPUnit config (`stopOnDefect` + `failOnWarning/Notice/Risky`) | +| `tests//cassettes/` | Committed `request()` cassettes (replay is the default) | +| `env.example.sh` | Template for required env variables (copy to `env.sh`) | +| `src/Exception/CnicException.php` | Base of the additive `CNIC\Exception` hierarchy | +| `src/{ResponseInterface,ResponseParserInterface,TransportInterface}.php` | The seams brands and tests substitute through | ## Atlassian / JIRA diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2a710992..95cc63f5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -28,6 +28,8 @@ Two rules follow, and they cut in opposite directions: Then prove the guard is not vacuous: apply the mutation it is supposed to refuse, confirm the guard fails, and confirm the rest of the suite stays green — that green suite is the whole argument for the test existing. Note that `.github/phpunit.xml` sets `stopOnDefect="true"`, so a plain `composer test` halts at the first failure; set the guard aside temporarily to observe the "nothing else fails" half. +**Check the exit code, not the summary line**, and be specific about what your mutation actually produces. A guard whose subject is a PHP diagnostic rather than a wrong value needs particular care: the config now sets `failOnWarning`/`failOnNotice`/`failOnRisky` so a warning does fail the build (RSRMID-2964), but a guard that leans on that alone goes vacuous the day someone edits those attributes back out. If the thing you are refusing is a diagnostic, assert on it inside the test — install a `set_error_handler`, capture, and assert nothing was raised. [tests/ResponseTemplateRegistrySeamTest.php](tests/ResponseTemplateRegistrySeamTest.php) does this, and its docblock says why. + ## Code of Conduct ### Our Pledge diff --git a/docs/agents/testing.md b/docs/agents/testing.md index 02c369f5..f85193ea 100644 --- a/docs/agents/testing.md +++ b/docs/agents/testing.md @@ -2,6 +2,17 @@ Reference for the test harnesses that need more than a one-line rule: the cassette record/replay flow, the functional (loopback) tests, and the deliberate MONIKER/IBS duplication. CLAUDE.md carries the imperative rules and links here. Guard-test authoring rules live in [CONTRIBUTING.md → Guard tests](../../CONTRIBUTING.md#guard-tests); the decisions the guards lock are in [architecture.md](architecture.md). +## A green run is a complete run (RSRMID-2964) + +`.github/phpunit.xml` pairs `stopOnDefect="true"` with `failOnWarning`, `failOnNotice` and `failOnRisky`. That pairing is the point: `stopOnDefect` halts on an error, failure, warning or risky result, and the three `failOn*` attributes make every one of those categories exit non-zero. Before they were set, the config displayed all six diagnostic categories and stopped on them but failed on none, so **a single "Undefined array key" in `src/` truncated the suite from 618 tests to 191 and still exited 0** — a green CI on under a third of the suite. + +Two consequences worth keeping in mind: + +- Read the **exit code**, not the summary line. A red run stops at the first defect, so its "passed" count says nothing about what else is broken — set the failing test aside temporarily if you need the rest of the picture. +- Do not treat this config as coverage for a guard test whose subject is a PHP diagnostic. It lives in a file an unrelated change can edit, and the guard would go quietly vacuous. Assert on the diagnostic inside the test instead — see [CONTRIBUTING.md → Guard tests](../../CONTRIBUTING.md#guard-tests). + +`failOnSkipped` is deliberately **not** set: the functional loopback test below skips legitimately when it cannot bind a port. `failOnDeprecation` is unset pending a check that the 8.4/8.5 CI legs are deprecation-free. + ## `request()`-path tests are cassette-driven (RSRMID-2910) The brand `ClientTest`s drive the full `request()`/`login()`/`logout()`/pagination lifecycle through `CNICTEST\Support\CassetteTransport`, injected via `AbstractClient::setTransport()` — the `TransportInterface` seam. From 9c2d9ec629760054100f963f2b30b2b21a9763b9 Mon Sep 17 00:00:00 2001 From: Kai Schwarz Date: Mon, 10 Aug 2026 16:00:28 +0200 Subject: [PATCH 3/9] feat(response): narrow the column and pagination seams MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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`, 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` 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) --- MIGRATION.md | 139 ++++++++++++++---- README.md | 11 +- docs/agents/architecture.md | 34 +++-- examples/datetime.php | 1 - src/AbstractResponse.php | 204 +++++++++++++++++++++++---- src/CNR/Client.php | 21 +-- src/CNR/Column.php | 36 ----- src/CNR/Response.php | 158 +++++++++------------ src/CNR/SessionCapable.php | 3 +- src/Column.php | 32 ++++- src/ColumnInterface.php | 10 ++ src/IBS/Response.php | 57 +++----- src/Record.php | 16 +++ src/RecordInterface.php | 10 ++ src/ResponseInterface.php | 31 +++- tests/CNR/ClientTest.php | 25 ++++ tests/CNR/ColumnTest.php | 65 --------- tests/CNR/ResponseTest.php | 153 ++++++++++++++++++++ tests/ColumnTest.php | 41 ++++-- tests/InterfaceCoverageSeamTest.php | 4 +- tests/RecordColumnSeamTest.php | 108 ++++++++------ tests/RecordTest.php | 20 +++ tests/ResponsePaginationSeamTest.php | 65 ++++++--- 23 files changed, 836 insertions(+), 408 deletions(-) delete mode 100644 src/CNR/Column.php delete mode 100644 tests/CNR/ColumnTest.php diff --git a/MIGRATION.md b/MIGRATION.md index ec39ada6..0442a887 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -10,32 +10,32 @@ Semantic versioning applies: **only major bumps (`X.0.0`) can break your code.** ## Version compatibility at a glance -| From → To | PHP required | Headline breaking change | Consumer action | -| --------- | ------------ | --------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| → v9.0.0 | **8.1+** | PHP 8.1 minimum | Bump your runtime | -| → v10.0.0 | 8.1+ | cURL handle cached/reused | Call `close()` in sessionless flows | -| → v11.0.0 | 8.1+ | IBS + Moniker brands added | None (additive) | -| → v12.0.0 | 8.1+ | HEXONET brand removed (EOL) | Migrate off HEXONET | -| → v13.0.0 | 8.1+ | IBS/Moniker switched to JSON API | Re-test IBS/Moniker data handling | -| → v14.0.0 | **8.3+** | Some classes `final`; `getPOSTData()` no longer takes a string | Bump runtime; stop subclassing finals | -| → v15.0.0 | 8.3+ | Logger contract; IBS session methods removed | Retype loggers; guard session calls | -| → v16.0.0 | 8.3+ | `ClientFactory::getClient()` signature slimmed | Configure the client yourself | -| → v17.0.0 | 8.3+ | `getNextPageNumber()` returns `null` on last page | Handle the `null` sentinel | -| → v18.0.0 | 8.3+ | CNR-only response methods moved off `ResponseInterface` | Narrow via `ExtendedResponseInterface` | -| → v19.0.0 | 8.3+ | `getClient()` removed; `setRoleCredentials()` moved | Use `cnr()`/`ibs()`/`moniker()` | -| → v20.0.0 | 8.3+ | IBS/Moniker no longer force IPv4; `getColumnKeys()` declares its `bool` parameter | Set `CURLOPT_IPRESOLVE` yourself if your host needs it; add the parameter if you implement `ResponseInterface` | -| → v21.0.0 | 8.3+ | `setExtraCurlOptions()` now reaches the wire; transport-owned options throw | Audit what you pass it — options previously ignored now take effect, and seven now raise | -| → v22.0.0 | 8.3+ | Sessions are CNR-only by type; IBS/Moniker `SessionClient` deleted | Drop `setSession()`/`getSession()` calls on IBS/Moniker; retype to `IBS\Client`/`MONIKER\Client` | -| → v23.0.0 | 8.3+ | Connection configuration has one home; `getSystem()` is nullable | Handle `null` from `getSystem()`; move `CURLOPT_TIMEOUT`/`USERAGENT`/`PROXY`/`REFERER` to their own setters | -| → v24.0.0 | 8.3+ | CNR IDN command rewriting moved off the shared client into its own module | Nothing, unless you called or overrode `autoIDNConvert()`, or read/set `needsIDNConvert` | -| → v25.0.0 | 8.3+ | One shared `Record` and `Column`; the brand `Record`/`IBS\Column` classes removed | Retype `CNR\Record`/`IBS\Record`/`AbstractRecord` → `CNIC\Record`, and `IBS\Column` → `CNIC\Column` | -| → v26.0.0 | 8.3+ | Response parsing is an injectable seam; `ResponseParser::parse()` is no longer static | Call `(new ResponseParser())->parse(…)`; implement `newResponseParser()` in a custom Response/TemplateManager | -| → v27.0.0 | 8.3+ | Loggers `format()` a record and a sink writes it; `setDefaultLogger()` removed | Rename your `log()` body to `format()` and `return` the string; extend `CNIC\AbstractLogger` | -| → v28.0.0 | 8.3+ | IBS/Moniker hash dates keep `/`; `RecordInterface`/`ColumnInterface` gained a date accessor; `IBS\Response::getStatus()` removed | Accept `/` wherever you parsed a `getHash()`/`getPlain()`/`getListHash()` date; add the new method if you implement either interface directly; read `getHash()["status"]` instead of `getStatus()` | -| → v29.0.0 | 8.3+ | Public method parameters and six protected properties renamed to be self-describing | Nothing, unless you pass named arguments (`getColumn(key: …)` → `columnName:`), implement an SDK interface (match the parameter names), or subclass and read `$this->pw`/`$ua`/`$curlopts` | -| → v30.0.0 | 8.3+ | Transport error is a declared `?string $error` parameter, not a `"httperror\|"` prefix on the raw payload; `nocurl` template gone | Add the parameter if you override `newResponse()`/`translate()`; return `["", $error]` (not bytes) on failure if you implement `TransportInterface` | -| → v31.0.0 | 8.3+ | `Response` is sealed after construction: the two mutators and the four record-cursor methods are off `ResponseInterface` | Replace `getNextRecord()` loops with `foreach ($r as $rec)` (it yields the first row too) and `getCurrentRecord()` with `getRecord(0)`; take `populate()`'s three new arguments if you subclass | -| → v32.0.0 | 8.3+ | The response-template registry is an instance, not `public static array $templates`; `resetTemplates()` removed | Call `(new ResponseTemplateManager())->addTemplate(…)` and pass the registry as `new Response($id, templates: $registry)`; delete `resetTemplates()` calls; take `translate()`'s new argument if you subclass | +| From → To | PHP required | Headline breaking change | Consumer action | +| --------- | ------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| → v9.0.0 | **8.1+** | PHP 8.1 minimum | Bump your runtime | +| → v10.0.0 | 8.1+ | cURL handle cached/reused | Call `close()` in sessionless flows | +| → v11.0.0 | 8.1+ | IBS + Moniker brands added | None (additive) | +| → v12.0.0 | 8.1+ | HEXONET brand removed (EOL) | Migrate off HEXONET | +| → v13.0.0 | 8.1+ | IBS/Moniker switched to JSON API | Re-test IBS/Moniker data handling | +| → v14.0.0 | **8.3+** | Some classes `final`; `getPOSTData()` no longer takes a string | Bump runtime; stop subclassing finals | +| → v15.0.0 | 8.3+ | Logger contract; IBS session methods removed | Retype loggers; guard session calls | +| → v16.0.0 | 8.3+ | `ClientFactory::getClient()` signature slimmed | Configure the client yourself | +| → v17.0.0 | 8.3+ | `getNextPageNumber()` returns `null` on last page | Handle the `null` sentinel | +| → v18.0.0 | 8.3+ | CNR-only response methods moved off `ResponseInterface` | Narrow via `ExtendedResponseInterface` | +| → v19.0.0 | 8.3+ | `getClient()` removed; `setRoleCredentials()` moved | Use `cnr()`/`ibs()`/`moniker()` | +| → v20.0.0 | 8.3+ | IBS/Moniker no longer force IPv4; `getColumnKeys()` declares its `bool` parameter | Set `CURLOPT_IPRESOLVE` yourself if your host needs it; add the parameter if you implement `ResponseInterface` | +| → v21.0.0 | 8.3+ | `setExtraCurlOptions()` now reaches the wire; transport-owned options throw | Audit what you pass it — options previously ignored now take effect, and seven now raise | +| → v22.0.0 | 8.3+ | Sessions are CNR-only by type; IBS/Moniker `SessionClient` deleted | Drop `setSession()`/`getSession()` calls on IBS/Moniker; retype to `IBS\Client`/`MONIKER\Client` | +| → v23.0.0 | 8.3+ | Connection configuration has one home; `getSystem()` is nullable | Handle `null` from `getSystem()`; move `CURLOPT_TIMEOUT`/`USERAGENT`/`PROXY`/`REFERER` to their own setters | +| → v24.0.0 | 8.3+ | CNR IDN command rewriting moved off the shared client into its own module | Nothing, unless you called or overrode `autoIDNConvert()`, or read/set `needsIDNConvert` | +| → v25.0.0 | 8.3+ | One shared `Record` and `Column`; the brand `Record`/`IBS\Column` classes removed | Retype `CNR\Record`/`IBS\Record`/`AbstractRecord` → `CNIC\Record`, and `IBS\Column` → `CNIC\Column` | +| → v26.0.0 | 8.3+ | Response parsing is an injectable seam; `ResponseParser::parse()` is no longer static | Call `(new ResponseParser())->parse(…)`; implement `newResponseParser()` in a custom Response/TemplateManager | +| → v27.0.0 | 8.3+ | Loggers `format()` a record and a sink writes it; `setDefaultLogger()` removed | Rename your `log()` body to `format()` and `return` the string; extend `CNIC\AbstractLogger` | +| → v28.0.0 | 8.3+ | IBS/Moniker hash dates keep `/`; `RecordInterface`/`ColumnInterface` gained a date accessor; `IBS\Response::getStatus()` removed | Accept `/` wherever you parsed a `getHash()`/`getPlain()`/`getListHash()` date; add the new method if you implement either interface directly; read `getHash()["status"]` instead of `getStatus()` | +| → v29.0.0 | 8.3+ | Public method parameters and six protected properties renamed to be self-describing | Nothing, unless you pass named arguments (`getColumn(key: …)` → `columnName:`), implement an SDK interface (match the parameter names), or subclass and read `$this->pw`/`$ua`/`$curlopts` | +| → v30.0.0 | 8.3+ | Transport error is a declared `?string $error` parameter, not a `"httperror\|"` prefix on the raw payload; `nocurl` template gone | Add the parameter if you override `newResponse()`/`translate()`; return `["", $error]` (not bytes) on failure if you implement `TransportInterface` | +| → v31.0.0 | 8.3+ | `Response` is sealed after construction: the two mutators and the four record-cursor methods are off `ResponseInterface` | Replace `getNextRecord()` loops with `foreach ($r as $rec)` (it yields the first row too) and `getCurrentRecord()` with `getRecord(0)`; take `populate()`'s three new arguments if you subclass | +| → v32.0.0 | 8.3+ | The response-template registry is an instance, not `public static array $templates`; `resetTemplates()` removed; `CNR\Column` deleted in favour of `?string` accessors on the interfaces; `getRecordsTotalCount()`/`getRecordsLimitation()` are `?int` and a brand `Response` no longer declares `getCurrentPageNumber()`/`hasNextPage()`/`hasPreviousPage()` | Call `(new ResponseTemplateManager())->addTemplate(…)` and pass the registry as `new Response($id, templates: $registry)`; delete `resetTemplates()` calls; take `translate()`'s new argument if you subclass; swap `CNR\Column`/`@var Column` for `getStringByIndex()`/`getStringByKey()`, and add both methods if you implement `ColumnInterface`/`RecordInterface`; widen your `ResponseInterface` implementation's return types and handle `null` from `getRecordsTotalCount()`/`getRecordsLimitation()`; re-check any `hasPreviousPage()` check against an unaligned `FIRST` | Two things to respect throughout: @@ -1265,7 +1265,7 @@ protected function populate(string $raw, ResponseParserInterface $parser, array -## → v32.0.0 — response templates live on an instance, not in a process-wide static bag +## → v32.0.0 — response templates live on an instance, not in a process-wide static bag; `CNR\Column` replaced by `?string` accessors **What changed:** `AbstractResponseTemplateManager` is now instantiable, and its templates belong to the instance. `public static array $templates` is gone from the base and from both brand managers; every operation that was `static` — `addTemplate()`, `getTemplate()`, `getTemplates()`, `hasTemplate()`, `generateTemplate()`, `isTemplateMatchHash()`, `isTemplateMatchPlain()` — is an instance method, declared on the new `CNIC\ResponseTemplateManagerInterface`. A registry reaches a response through a new trailing `?ResponseTemplateManagerInterface $templates = null` argument on `Response::__construct()`; omit it and you get the brand's built-ins exactly as before. @@ -1362,6 +1362,91 @@ protected function translate( **Why this happened:** the container was `public static` with process lifetime and the translator read it live at translate time, so a template registered for one scenario silently changed response translation in every later one — across test classes in a single PHPUnit process, and across requests in any long-lived consumer. `resetTemplates()` could not reliably contain that: it was a no-op unless `addTemplate()` had run first, and a direct assignment to the public property escaped it entirely. Making the property private was impossible while the translator read it across a class boundary. Handing the registry to the response that needs it removes the shared state rather than patching around it — which is also why `resetTemplates()` is gone rather than kept: state that cannot escape its object has nothing to reset. (Ref: RSRMID-2941.) +### `CNIC\CNR\Column` is gone — ask for a `?string` instead + +**What changed:** `CNIC\CNR\Column` is **deleted**. Both brands now build the shared `CNIC\Column` directly, and the `@template TValue` parameter on `CNIC\Column` is gone with it. In its place, two new methods are declared on the interfaces: + +| New method | Returns | Behaviour | +| ------------------------------------------------- | --------- | -------------------------------------------------------------------- | +| `ColumnInterface::getStringByIndex(int $idx)` | `?string` | `null` for an out-of-range index **or** a value that is not a string | +| `RecordInterface::getStringByKey(string $column)` | `?string` | `null` for a missing key **or** a value that is not a string | + +**Who is affected:** anyone who named `CNIC\CNR\Column` in a type hint, an `instanceof`, or a `@var` annotation; anyone implementing `ColumnInterface` or `RecordInterface` themselves; and anyone supplying their own parser to a `CNR\Response`. + +**What to respect — reading a string cell:** + +```php +// BEFORE (v31) — mixed, unless you hand-annotated your way out of it +/** @var \CNIC\CNR\Column $col */ +$col = $r->getColumn("DOMAIN"); +$name = $col->getDataByIndex(0); // mixed, despite the annotation above + +// AFTER (v32) — a real return type, through the interface, on every brand +$name = $r->getColumn("DOMAIN")?->getStringByIndex(0); // ?string +$owner = $r->getRecord(0)?->getStringByKey("OWNER"); // ?string +``` + +Delete any `/** @var Column $col */` you wrote to work around the old `mixed`: the generic is gone, and the annotation now describes a type parameter that no longer exists. + +`getDataByIndex()` and `getDataByKey()` are unchanged and still return `mixed` — use them when you want the raw value, including IBS/Moniker cells carrying nested arrays or objects. On such a cell `getStringByIndex()` answers `null`; it narrows, it does not coerce. + +**If you implement `ColumnInterface` or `RecordInterface` yourself,** add the corresponding method — this is an interface widening and your class stops satisfying the contract without it. One line each: + +```php +public function getStringByIndex(int $recordIndex): ?string +{ + $value = $this->getDataByIndex($recordIndex); + return is_string($value) ? $value : null; +} +``` + +**If you supply a parser to a `CNR\Response`, nothing changes for you.** CNR still requires each `PROPERTY` entry to be a list of strings and still raises `UnsupportedFeatureException` naming the column otherwise — that check is what keeps `getStringByIndex()` from ever answering `null`-for-wrong-type on CNR, so it outlived the generic it was originally introduced to support. + +**Why this happened:** `CNR\Column` existed to bind the column's value type to `string` and narrow `getDataByIndex()` to `?string`. The binding was real and both analysers enforced it — but `ColumnInterface` is not generic, and every path you can reach returns the interface (`getColumn(): ?ColumnInterface`, `getColumns(): ColumnInterface[]`). The narrowing was erased before it reached anyone, which is why the SDK's own example needed a hand-written `@var` and why CNR re-checked with `is_scalar()` at runtime. Making the interfaces generic instead would have pushed `ResponseInterface` into _your_ type hints and still meant nothing without PHPStan or Psalm in your build. A native `?string` return type needs no annotation, no analyser and no configuration — it is the same shape `getDateTimeByIndex()`/`getDateTimeByKey()` introduced in v28. (Ref: RSRMID-2942.) + +### The pagination seam narrowed to 4 wire-column primitives; `getRecordsTotalCount()`/`getRecordsLimitation()` are `?int` + +**What changed:** `ResponseInterface::getRecordsTotalCount()` and `getRecordsLimitation()` now return `?int` instead of `int`. A brand `Response` (`CNR\Response`/`IBS\Response`) no longer declares `getCurrentPageNumber()`, `hasNextPage()` or `hasPreviousPage()` — those three moved to `AbstractResponse` as pure derivations of the four column-reading primitives (`getFirstRecordIndex`/`getLastRecordIndex`/`getRecordsTotalCount`/`getRecordsLimitation`). `hasPreviousPage()`'s answer changed on an unaligned offset window: it now answers `FIRST > 0` directly instead of comparing whole page numbers. + +**Who is affected:** anyone implementing `ResponseInterface` directly; anyone consuming `getRecordsTotalCount()`/`getRecordsLimitation()` or `getPagination()["TOTAL"]`/`["LIMIT"]`/`["CURRENTPAGE"]` on a non-list response; anyone who subclassed a brand `Response` and overrode `getCurrentPageNumber()`/`hasNextPage()`/`hasPreviousPage()`; and anyone relying on `hasPreviousPage()` being false on a mid-window CNR page (e.g. `FIRST=50, LIMIT=100`). + +```php +// BEFORE (v31) — a non-list response's total/limit silently equalled its +// record count, indistinguishable from a real list whose total/limit +// genuinely equalled that count +$r = new CNIC\CNR\Response($raw); // no TOTAL/LIMIT columns, 2 rows +$r->getRecordsTotalCount(); // int(2) — is this a real total, or just "no column"? +$r->getRecordsLimitation(); // int(2) — same ambiguity +$r->getPagination()["TOTAL"]; // int(2) + +// AFTER (v32) — "no column" is representable, and 0 is a real answer again +$r->getRecordsTotalCount(); // null — this response carries no TOTAL column +$r->getRecordsLimitation(); // null — this response carries no LIMIT column +$r->getPagination()["TOTAL"]; // null +$r->getPagination()["CURRENTPAGE"]; // null + +// A response that DOES send LIMIT=0 (a real, requested value) is now +// distinguishable from one that sends no LIMIT column at all: +$zeroLimit->getRecordsLimitation(); // int(0), not null + +// hasPreviousPage() on a mid-window CNR page (FIRST=50, LIMIT=100): +// BEFORE: false (getCurrentPageNumber() was 1, and 1 - 1 === 0 read as "no previous page") +// AFTER: true (FIRST=50 > 0 — there genuinely are 50 preceding rows) +``` + +`getPagination()`'s key set and the `PAGES` value are unchanged: `PAGES` still resolves to `1` for a response with rows but no pagination columns (an implicit single page), matching what a non-paginating brand like IBS has always reported. + +**If you implement `ResponseInterface` yourself,** widen both return types to `?int` — this is an interface widening, so a stricter `int` return no longer satisfies the contract: + +```php +public function getRecordsTotalCount(): ?int { … } +public function getRecordsLimitation(): ?int { … } +``` + +Delete any `getCurrentPageNumber()`/`hasNextPage()`/`hasPreviousPage()` override that only reproduced the shared arithmetic — `AbstractResponse` now supplies it from your `getFirstRecordIndex()`/`getLastRecordIndex()`/`getRecordsTotalCount()`/`getRecordsLimitation()`. An override that does something genuinely brand-specific still works; it now overrides shared arithmetic rather than filling an interface hole the base left open. + +**Why this happened:** `getCurrentPageNumber()`, `hasNextPage()` and `hasPreviousPage()` read no wire column of their own — they were always pure functions of the four column readers, exactly like the derived getters that stayed on the base all along. Pinning them as brand-declared "primitives" protected nothing, and let CNR's hand-written versions compute from whole page numbers while the sibling getters (`getNextPageNumber()`, `getNumberOfPages()`) computed from record offsets — two arithmetic paths over the same four numbers that agree only when `FIRST` happens to be a multiple of `LIMIT`. On an unaligned offset window (CNR's list API is offset-based: `FIRST`/`LAST`/`LIMIT`/`TOTAL`), the two paths disagreed: `hasNextPage()` could answer `true` for a window that already held the tail of the list (an avoidable empty request), and `hasPreviousPage()` could answer `false` for a window with fifty preceding rows. Deriving every predicate and page number from the same offset grid removes the possibility of disagreement, aligned or not. (Ref: RSRMID-2943.) + --- ## Reference: the canonical usage diff --git a/README.md b/README.md index 58054d81..4d650a3c 100644 --- a/README.md +++ b/README.md @@ -68,7 +68,7 @@ A response is fully assembled by the time you hold one, and read-only from then $r = $cl->request(["COMMAND" => "QueryDomainList", "LIMIT" => "100"]); foreach ($r as $index => $rec) { - echo $index, ": ", $rec->getDataByKey("DOMAIN"), "\n"; + echo $index, ": ", $rec->getStringByKey("DOMAIN"), "\n"; } $r->getRecord(0); // ?RecordInterface — by index, or null if out of range @@ -77,6 +77,15 @@ $r->getColumn("DOMAIN"); // ?ColumnInterface — column-wise instead of row-w $r->getPagination(); // COUNT / FIRST / LAST / LIMIT / TOTAL / PAGES / … ``` +**Ask for the type you want.** `getDataByKey()`/`getDataByIndex()` return `mixed`, because an IBS/Moniker cell may legitimately carry a nested array or object. When you expect a plain value, the typed accessors save you the check — each returns `null` for a missing key, an out-of-range index, or a value of the wrong type, so there is nothing to narrow by hand and no annotation to write: + +```php +$name = $rec->getStringByKey("DOMAIN"); // ?string +$expiry = $rec->getDateTimeByKey("expirationdate"); // ?ApiDateTime + +$name = $r->getColumn("DOMAIN")?->getStringByIndex(0); // same, by column +``` + `foreach` keeps its position in the loop rather than on the response, so iterating is repeatable, needs no rewind step, and two places iterating the same response cannot interfere. If you are coming from a version with `getNextRecord()`/`rewindRecordList()`, see [Migration Guide → v31.0.0](https://github.com/centralnicgroup-opensource/rtldev-middleware-php-sdk/blob/master/MIGRATION.md#-v3100). ### Debug output diff --git a/docs/agents/architecture.md b/docs/agents/architecture.md index 7805aa8f..abdf6f18 100644 --- a/docs/agents/architecture.md +++ b/docs/agents/architecture.md @@ -20,11 +20,16 @@ Full architectural reference for the PHP SDK. ## Decision records -- **The pagination seam is deliberately asymmetric — do not "simplify" it by hoisting single-page defaults (RSRMID-2912 declined, RSRMID-2918).** `AbstractResponse` implements the 5 getters that are pure functions of the primitives (`getNextPageNumber`/`getNumberOfPages`/`getPagination`/`getPreviousPageNumber`/`getRecordsCount`); the 7 primitives (`getCurrentPageNumber`/`getFirstRecordIndex`/`getLastRecordIndex`/`getRecordsTotalCount`/`getRecordsLimitation`/`hasNextPage`/`hasPreviousPage`) stay per brand. The ticket was **declined and closed as Cancelled**, so this entry is the authoritative record, not the ticket. - - **Failure mode: a silent wrong answer.** Today a brand that forgets pagination cannot be instantiated — PHP raises a declaration-time fatal for the unimplemented interface methods. With base defaults it would quietly report "page 1 of 1, no next page", and a consumer paging a list would lose pages 2..N with no error anywhere. Never trade a loud compile-time error for a silent data-loss default. - - **Rejected:** base defaults (above); and a `SinglePageResponse` trait, despite the `CNR\SessionCapable` precedent — with only two Response implementations, a default *or* trait would serve exactly one consumer. - - Note the 7 are plain `ResponseInterface` methods the base never implements, **not** members `AbstractResponse` declares `abstract`. Reflection lists them as abstract members of `AbstractResponse` while `getDeclaringClass()` points at `ResponseInterface` — the misreading that produced the declined proposal. The status/code accessors are in the same category; what is specific to pagination is only the no-defaults rule. (`addColumn` used to be too, until RSRMID-2939 took it off the interface and made it a plain `protected` brand hook.) - - **Revisit** only if a third genuinely non-paginating brand arrives. **Guard:** `tests/ResponsePaginationSeamTest.php`. +- **The pagination seam is drawn at the wire — do not hoist single-page defaults onto the base (RSRMID-2912 declined, RSRMID-2918 delivered, RSRMID-2943 narrowed).** `AbstractResponse` implements 8 derived getters that are pure functions of 4 wire-column primitives (`getCurrentPageNumber`/`getNextPageNumber`/`getNumberOfPages`/`getPagination`/`getPreviousPageNumber`/`getRecordsCount`/`hasNextPage`/`hasPreviousPage`); the 4 primitives (`getFirstRecordIndex`/`getLastRecordIndex`/`getRecordsTotalCount`/`getRecordsLimitation`) — the methods that actually read a brand's pagination columns — stay per brand. RSRMID-2912 was **declined and closed as Cancelled**; RSRMID-2918 is the issue that delivered the original 7/5 split; this entry is the authoritative record for the current 4/8 split, not either ticket. + - **What still stands from 2912/2918: do not hoist single-page DEFAULTS onto the base.** A brand must still be forced to answer what its own columns say for the 4 primitives — a base default would let a brand that forgot pagination silently report "page 1 of 1, no next page" instead of failing at declaration time. That half of the original decision is untouched by RSRMID-2943. + - **What changed and why.** `getCurrentPageNumber()`/`hasNextPage()`/`hasPreviousPage()` read no wire column of their own — they were always pure functions of `getFirstRecordIndex()`/`getLastRecordIndex()`/`getRecordsLimitation()`/`getRecordsTotalCount()`, exactly like `getNextPageNumber()`/`getNumberOfPages()` already were. Pinning them as "primitives" protected nothing (there was no column read to force a brand to answer) and pinned a misclassification instead: CNR's own hand-rolled versions computed `hasNextPage()`/`hasPreviousPage()` from whole page numbers (`getCurrentPageNumber() ± 1` vs. `getNumberOfPages()`), while the sibling getters computed from record offsets — two arithmetic paths over the same four numbers that agree only when `FIRST` is a multiple of `LIMIT`. CNR's list API is offset-based (`FIRST`/`LAST`/`LIMIT`/`TOTAL`), so an unaligned window (e.g. `FIRST=50, LIMIT=100`) made the two paths disagree: the page-arithmetic predicate answered `hasNextPage()` from `getCurrentPageNumber()+1 <= getNumberOfPages()`, which said "true" for a window that already held the tail of the list, costing an empty follow-up request — and `hasPreviousPage()` claimed a mid-window position had no previous page (`getCurrentPageNumber()-1 > 0` needs a whole page's gap) when `FIRST=50 > 0` plainly does have preceding rows. Every predicate and page-number getter is now derived from the same offset grid (`FIRST`/`LAST` vs. `TOTAL`/`LIMIT`), so a predicate and its corresponding getter cannot disagree, aligned or not. + - **An empty CNR window echoes `LAST = FIRST`, and that is what makes the offset walk terminate.** Three captured `QueryDomainList` shapes, all `count=0`: `FIRST=0, LIMIT=0` → `last=0, total=1825820`; `FIRST=2000000, LIMIT=0` → `last=2000000, total=1825824`; `FIRST=20000000, LIMIT=10` → `last=20000000, total=1825824`. `LAST` is **not** a flat floor of `0` — the first shape only looks like one because `FIRST` is `0` there too. Consequences: the third self-terminates on the arithmetic (`20000001 < 1825824` is false), while the first two need the **`LIMIT <= 0` gate**, because `LAST + 1 < TOTAL` holds and `requestNextResponsePage()` would otherwise advance to `FIRST = 1` and re-walk the list from near the start. That older guard is therefore still the load-bearing one; the offset grid did not replace it. + - The second gate, `LAST < FIRST`, is **defensive only and no observed response triggers it** — it pins the invariant the advance rests on: since `LAST >= FIRST` always, `FIRST = LAST + 1` strictly increases and the walk is monotonic. A wire change breaking that would page *backward* rather than fail. Guarded by `CNR\ResponseTest::testWindowEndingBeforeItStartsHasNoNextPage`, whose comment says outright that the shape is synthetic. + - **`getRecordsCount() === 0` is NOT a usable substitute for either gate**, however much "an empty window has no next page" sounds like the same statement: `assembleRecords()` sizes the record list across *every* column, pagination columns included, so a CNR response carrying nothing but `COLUMN`/`COUNT`/`FIRST`/`LAST`/`LIMIT`/`TOTAL` still reports **one** record. + - `LAST` is not a valid *request* parameter either — `FIRST`/`LIMIT` are the whole paging vocabulary — which is why `requestNextResponsePage()` throws `PaginationException` when a caller puts `LAST` in the command. Captures pinned in `CNR\ResponseTest::testPastTheEndWindow*` and `CNR\ClientTest::testRequestNextResponsePagePastTheEnd`. + - **Rejected:** base defaults for the 4 primitives (as before); a `SinglePageResponse` trait, despite the `CNR\SessionCapable` precedent — with only two Response implementations, a default *or* trait would serve exactly one consumer. + - Note the 4 are plain `ResponseInterface` methods the base never implements, **not** members `AbstractResponse` declares `abstract`. Reflection lists them as abstract members of `AbstractResponse` while `getDeclaringClass()` points at `ResponseInterface` — the misreading that produced the 2912 proposal. The status/code accessors are in the same category; what is specific to pagination is only the no-defaults rule. (`addColumn` used to be too, until RSRMID-2939 took it off the interface and made it a plain `protected` brand hook.) + - **Revisit** if a brand's "more results" signal becomes a cursor or opaque token rather than a record offset — for such a brand `hasNextPage()` genuinely becomes a wire read again and belongs back in the primitive set. **Guard:** `tests/ResponsePaginationSeamTest.php`. - **A `Response` is sealed once constructed, and its records are iterated rather than stepped (RSRMID-2939, breaking, v31.0.0).** `ResponseInterface` lost six methods: the mutators `addColumn()`/`addRecord()` (now `protected`) and the record cursor `getCurrentRecord()`/`getNextRecord()`/`getPreviousRecord()`/`rewindRecordList()` (gone, with the three `protected` `has*Record()` predicates and the `$recordIndex` property). The interface `extends \IteratorAggregate`; consumers `foreach`, or use `getRecord(int)`/`getRecords()`. - **Five undeclared rules, all now type-enforced or impossible.** (1) Constructor step order was load-bearing and enforced only by a comment — `populate()` read `$this->command`/`$this->parser`/`$this->raw`, and moving the `$this->command` assignment below the call silently switched the IBS parser's wire branch. It now takes `populate(string $raw, ResponseParserInterface $parser, array $cmd): void`, so there is no order to get wrong and no `$parser` property at all. (2) A column added post-construction was absent from every record (records are assembled from the columns once, at the end of `populate()`). (3) `assembleRecords()` appended, so it was not idempotent; it now replaces. (4) A duplicate column name half-registered via `??=`, leaving `getColumns()` holding a column `getColumn()` could never return; `registerColumn()` now throws `DuplicateColumnException`. (5) The only way to query the cursor was to move it — the predicates were `protected` while `getNextRecord()` advanced shared state, so two consumers holding one response interfered. - **`getCurrentRecord()` went too, though the ticket only listed five methods.** With nothing able to move `$recordIndex`, it would have been a permanently frozen alias for `getRecord(0)` — a name that lies about what it does. Removing it makes the interface shrink by six, not five. @@ -33,17 +38,26 @@ Full architectural reference for the PHP SDK. - **Note for consumers, worth not re-deriving:** the old `getNextRecord()` pre-incremented, so a `while ($rec = $r->getNextRecord())` loop never saw record 0. A `foreach` ported from one legitimately sees **one more row**. Flagged in MIGRATION.md. - **Revisit** only on a concrete need to build a response incrementally from outside the brand's `populate()` — which does not exist today: responses are born from `AbstractClient::newResponse()`/`AbstractResponseTemplateManager::createResponse()`, each handing a whole raw payload to a constructor, and a substitute `ResponseParserInterface` already controls what a response contains without any mutator. **Guard:** `tests/ResponseSealSeamTest.php`. - **Records use an output factory hook, columns hand a finished instance to a shared registrar — do not "symmetrise" them (RSRMID-2899, RSRMID-2923).** `AbstractResponse::addRecord()` delegates to abstract `newRecord(array $row): RecordInterface`. Columns cannot use that shape, so each brand's `addColumn()` builds its own correctly-typed Column and passes it to `AbstractResponse::registerColumn(ColumnInterface $col): static`, which owns the `$columns`/`$columnKeys`/`$columnIndex` bookkeeping once. Keep the `registerColumn()` shape. Both `addRecord()` and `addColumn()` are `protected` since RSRMID-2939 — the seam shape is unchanged, only its visibility. - - **Rejected, and infeasible rather than merely unattractive:** a param-typed `newColumn(string, array): ColumnInterface`. A CNR column takes `string[]` and an IBS column mixed JSON values, so the base factory would have to narrow `array` into CNR's `string`-bound constructor — rejected by both PHPStan L9 (`argument.type`) and Psalm L1 (`MixedArgumentTypeCoercion`), and the toolchain forbids silencing either. Sharing a column *base* is a different question from a param-typed *factory*; the value types still diverge, which is why the shared column is templated instead. - - **No guard test** — this one is enforced by the analysers, and is listed in CLAUDE.md for that reason. -- **The record/column layer is one class each — do not re-split it per brand (RSRMID-2923, breaking, v25.0.0).** One concrete `CNIC\Record` and one concrete, templated `CNIC\Column`. `CNR\Column` is the only surviving brand column and earns its place solely by binding `TValue` to `string` and narrowing `getDataByIndex(): ?string`. + - **Rejected, and infeasible rather than merely unattractive:** a param-typed `newColumn(string, array): ColumnInterface`. A CNR column took `string[]` and an IBS column mixed JSON values, so the base factory would have had to narrow `array` into CNR's `string`-bound constructor — rejected by both PHPStan L9 (`argument.type`) and Psalm L1 (`MixedArgumentTypeCoercion`), and the toolchain forbids silencing either. Sharing a column *base* was a different question from a param-typed *factory*. + - **⚠ The premise changed under RSRMID-2942 (v32.0.0) — re-derive before citing this.** That ticket deleted `CNR\Column`, so both brands now construct `CNIC\Column` from `array` and the constructor divergence the infeasibility argument rested on no longer exists. The directive still stands in CLAUDE.md because nobody has re-tested it, **not** because the original reasoning still holds. Anyone re-opening it must re-run the analysers rather than quoting the paragraph above; anyone leaving it closed should record a current reason here. + - **No guard test** — this one was enforced by the analysers, and is listed in CLAUDE.md for that reason. +- **The record/column layer is one class each — do not re-split it per brand (RSRMID-2923, breaking, v25.0.0).** One concrete `CNIC\Record` and one concrete `CNIC\Column`, used directly by every brand. `CNR\Column` survived this collapse as the one brand column, binding `TValue` to `string`; RSRMID-2942 below removed it too, so the rule is now simply **no brand declares a Record or a Column**. - **`newRecord()` stays an abstract per-brand hook** even though both brands return the same class: it is the seam a brand with genuinely different row behaviour would implement, and hard-coding `new Record()` on the base would close it. A hook body *is* the brand's declaration of its record type — unlike the empty `class Record extends AbstractRecord {}` markers this deleted, which carried no decision. - - **Empirical, and worth not re-deriving:** a covariant `?string` override against the base's `mixed` return is accepted by **both** PHPStan L9 and Psalm L1 with no suppression, and `@extends CNIC\Column` is genuinely enforced — `new CNR\Column("X", [1, 2, 3])` is rejected by both. Net **loss** of one suppression: the hand-rolled `CNR\Column` needed `@psalm-suppress MoreSpecificImplementedParamType`, the template does not. + - **Empirical, and worth not re-deriving:** a covariant `?string` override against the base's `mixed` return is accepted by **both** PHPStan L9 and Psalm L1 with no suppression, and `@extends CNIC\Column` is genuinely enforced — `new CNR\Column("X", [1, 2, 3])` was rejected by both. All of that was true and none of it was the problem: see RSRMID-2942 below for why an *enforced* binding still reached no consumer. - **Guard:** `tests/RecordColumnSeamTest.php`. +- **A value-type narrowing belongs in a native return type on the interface, never in a generic (RSRMID-2942, breaking, 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 declared on `ResponseInterface`. 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. `CNR\Column` and `@template TValue` are gone; `ColumnInterface::getStringByIndex(): ?string` and `RecordInterface::getStringByKey(): ?string` replace them. + - **The measure of a public narrowing is what an integrator gets without configuring anything.** A docblock generic means nothing unless the integrator runs PHPStan or Psalm themselves; it is invisible in a plain IDE and at runtime. The repo proved the erasure twice before the fix — `examples/datetime.php` needed a hand-written `@var Column`, and `CNR\Response::columnInt()` re-narrowed with `is_scalar()` at runtime precisely because the static type was gone. + - **Making `ColumnInterface` generic was the rejected alternative, and it is worse than it sounds.** The erasure is at `getColumn()`, so `ColumnInterface` alone fixes nothing — `ResponseInterface` would have to become generic too, pushing `ResponseInterface` into every integrator's own type hints. That trades one annotation for a heavier one. + - **Same shape as the date/time accessors (RSRMID-2318/2926), on purpose** — one-line, opt-in, declared on both the class and its interface, `null` rather than a throw for a value of the wrong type. Raw access stays on `getDataByIndex(): mixed` / `getDataByKey(): mixed`. + - **`CNR\Response::stringCells()` was proposed for deletion and deliberately kept.** The ticket argued it existed only to satisfy the string binding. That was half right: the binding is gone, but the check independently makes a substitute parser's contract violation fail loudly, and keeps CNR cells string-guaranteed so `getStringByIndex()` cannot answer null-for-wrong-type on this brand. Deleting it would have weakened the very accessor this ticket added. Full reasoning under RSRMID-2924 above, where the check lives. **What is still open** is that it borrows `UnsupportedFeatureException` — documented as "capability absent on this platform" — for a malformed cell. The hierarchy is additive, so a dedicated type is a non-breaking follow-up, not a reason to remove the check. + - **Guard:** `tests/RecordColumnSeamTest.php`, extended from "only CNR declares a Column" to "no brand declares one", plus a structural check that the two interfaces declare the narrowing accessors. Both halves are structural by necessity: re-adding a brand column, or demoting the accessor back to a generic, is behaviour-preserving on the day it lands. - **The parse step is a seam, not a static call (RSRMID-2924, breaking, v26.0.0).** `CNIC\ResponseParserInterface` declares one signature, `parse(string $raw, array $cmd = []): array`, and both brand parsers are instantiable classes implementing it. Previously `populate()` called `RP::parse()` statically with **different signatures** per brand, so no shared contract was even expressible and nothing in the Response tree could be exercised without a full raw wire payload. - **The shape is the `TransportInterface` one, deliberately:** an abstract `newResponseParser()` hook supplies the brand default, and an optional trailing `?ResponseParserInterface $parser = null` constructor argument overrides it. Adding an optional parameter is legal widening, so no implementation stops satisfying anything. - **CNR takes `$cmd` and ignores it, on purpose.** A contract covering only IBS's shape, or two contracts, buys nothing — CNR's wire format is self-describing, and a uniform signature is what lets one seam serve both brands. - **`AbstractResponseTemplateManager::parseResponse()` is gone**, replaced by the same `newResponseParser()`. It still parses with **no** command (a template is not tied to one) while `populate()` passes the command it was built with; for IBS that selects the JSON versus plain-text branch. **Trap when asserting on that divergence:** the command must be one *without* `ResponseFormat` — one carrying `ResponseFormat=JSON` takes the same branch as the empty one and makes the assertion vacuous. - **A non-string cell — and equally a column entry that is not a list — throws** `UnsupportedFeatureException` naming the column, from `CNR\Response::stringCells()`. Skipping would be the silent no-op RSRMID-2919/RSRMID-2920 ruled out; coercing would invent a value the wire could never carry. The container half is the one that is easy to forget: `foreach ((array)$values …)` turns a bare string into a one-cell column while a bare `int` still throws a line later. Deliberately asymmetric with the level above: a **missing or non-array `PROPERTY` block** yields no columns rather than throwing, because most CNR responses legitimately have none. + - **RSRMID-2942 proposed deleting this and it was kept — the reasons are no longer the original ones.** The check was introduced to back `CNR\Column`'s `string` binding, and that class is gone. It stays because two independent reasons survived: only a *substitute* parser can reach it, so a violation is a programming error that should fail at construction rather than as a null three calls later; and CNR cells being string-guaranteed is exactly what stops `getStringByIndex()` answering null-for-wrong-type on this brand — an accessor is only as self-explaining as the data behind it. **Do not re-raise the deletion on the "it only serves the generic" argument; that argument is spent.** The live complaint is the borrowed exception type (see below), fixable additively without touching the check. + - **Guard:** `tests/ResponseParserSeamTest::testCNRRejectsANonStringColumnCellFromASubstituteParser()` and `::testCNRRejectsAColumnThatIsNotAListAtAll()`. Their inline comments still cite the `string` binding as the premise — read this entry, not those comments, for why the checks stand. - **`resetTemplates()` was `addTemplate()`'s counterpart, not a general undo — and it is gone.** It restored what the container held the first time `addTemplate()` ran for a class, so it was a no-op when the class was never added to, and a direct assignment to the public `$templates` property escaped it entirely: the RSRMID-2921 lesson that a guard on one writer does not protect state with several. The separate change it pointed to is RSRMID-2941 below, which removed the second writer by removing the shared container. - **Build-then-populate remains impossible by design.** The seam **adds a sixth** constructor dispatch rather than removing any, because injection makes the parse step substitutable without touching the constructor's load-bearing shape. **Revisit** as a ticket about the constructor, not about parsing. - **Guard:** `tests/ResponseParserSeamTest.php` (behavioural half via `CNICTEST\Support\SpyResponseParser`, structural half against a re-inlined `new RP()`). @@ -114,7 +128,7 @@ Full architectural reference for the PHP SDK. - **`ts` and `dateTime` are null together for date-only values; `date` is always populated.** `dateTime` deliberately does **not** fall back to `date`: `strtotime($dt->dateTime)` on a fallback returns midnight UTC, precisely the fictitious instant `ts === null` exists to refuse, whereas null fails loudly (`strtotime(null)` is a TypeError on PHP 8.1+). `tz` stays `"UTC"` even then: it describes the source *declaration*, not an instant. `castDate()`'s `tzAbbr` is **not** carried over — it held the abbreviation *at that instant* (`CET`/`CEST`), a distinction that cannot exist in a UTC-only type with no DST, so it would duplicate `tz` forever. - **Separator tolerance, both sides consistent (RSRMID-2926).** `PATTERN` accepts `-` or `/` via a captured `sep` group plus a `\k` backreference, so `2026-02/20` and `2026/02-20` are still rejected — as strict about shape as before, over a wider accept-set. `$date`/`$dateTime` always emit `-`, normalised before `createFromFormat()`, so the struct's shape never depends on which brand sent it. Still a parser: no new public method. - **`$raw` exists because the accessors closed the gap that used to guarantee the caller still had it.** Before `getDateTimeByKey()`/`getDateTimeByIndex()`, the only route was `ApiDateTime::tryFrom($hash["paiduntil"])`, so a caller necessarily still held the original string. It is populated from the constructor's `$value` at both `from()` call sites — **never** from the separator-normalised subject — and is the only place the fractional precision `$dateTime` discards survives. **Display/logging/round-trip only:** comparing or sorting on `$raw` reintroduces exactly the separator bug `$date`'s normalisation prevents (`"2026/02/20"` sorts wrong against `"2026-03-01"`). No getter, no `withRaw()`. - - **`Record`/`Column` gained one opt-in accessor each, not a shared helper.** Each is a one-line narrowing (`is_string($v) ? ApiDateTime::tryFrom($v) : null`) declared on both the class and its interface. The line is duplicated deliberately: `Record` and `Column` have no composition relationship, so sharing it would either invent one or add an indirection with nothing behind it. Both interface declarations widen the public contract, which is what makes this breaking (v28.0.0) independent of the separator change. The accessor is **`CNIC\Column`-only** — `CNR\Column` gained nothing, guarded by `RecordColumnSeamTest::testCnrColumnNarrowsNothingButGetDataByIndex`. + - **`Record`/`Column` gained one opt-in accessor each, not a shared helper.** Each is a one-line narrowing (`is_string($v) ? ApiDateTime::tryFrom($v) : null`) declared on both the class and its interface. The line is duplicated deliberately: `Record` and `Column` have no composition relationship, so sharing it would either invent one or add an indirection with nothing behind it. Both interface declarations widen the public contract, which is what makes this breaking (v28.0.0) independent of the separator change. **RSRMID-2942 (v32.0.0) generalised the shape** — the same one-line, interface-declared narrowing now covers strings too, and it is what replaced the erased `CNR\Column` generic. - **The IBS date-separator rewrite is deleted, and deliberately not guarded.** `IBS\ResponseParser` used to `array_walk_recursive()` and rewrite `/` to `-` in values whose key matched `/(date|paiduntil|expiration)$/i` — the only place in `src/` that mutated raw response data, and over-broad enough to turn `n/a` under an `updatedate` key into `n-a`. No guard is needed because re-adding it is **not** behaviour-preserving: it changes `getHash()`/`getPlain()`/`getListHash()` for every IBS/Moniker date field, so the ordinary parser/response/record/column assertions already catch it. - **The interface declarations are protected by `#[\Override]`, not by a test** — dropping them from `RecordInterface`/`ColumnInterface` while leaving the methods on the classes would otherwise be behaviour-preserving for the whole suite. `#[\Override]` makes it a declaration-time fatal, which is why no `*SeamTest.php` was added. **The corollary: stripping `#[\Override]` from either implementation silently removes the only thing protecting the interface declaration.** - **Failures throw from one hierarchy, and growing it is not a breaking change (RSRMID-2895).** Every exception extends `CNIC\Exception\CnicException extends \Exception`. Reuse the existing types rather than duplicating them: `UnsupportedFeatureException` (a capability this platform or response does not have — since RSRMID-2919 also a cURL option or HTTP header the transport owns), `PaginationException`, `InvalidConfigurationException` (a configuration value out of range), `InvalidDateTimeException`. diff --git a/examples/datetime.php b/examples/datetime.php index 9d60b508..30efcf17 100644 --- a/examples/datetime.php +++ b/examples/datetime.php @@ -70,7 +70,6 @@ var_dump($rec->getDateTimeByKey("note")); // null — not parsable, not thrown var_dump($rec->getDateTimeByKey("missing")); // null — key absent -/** @var Column $col */ $col = new Column("expirationdate", ["2030/07/17"]); var_dump($col->getDateTimeByIndex(0)?->isDateOnly()); // true var_dump($col->getDateTimeByIndex(1)); // null — out of range diff --git a/src/AbstractResponse.php b/src/AbstractResponse.php index 5b291132..e4ca6702 100644 --- a/src/AbstractResponse.php +++ b/src/AbstractResponse.php @@ -30,17 +30,22 @@ * (getCode/getDescription/isError/isSuccess) — each reads a different wire * shape, * - the pagination primitives, likewise declared on {@see ResponseInterface} - * (getCurrentPageNumber, getFirstRecordIndex, getLastRecordIndex, - * getRecordsTotalCount, getRecordsLimitation, hasNextPage, hasPreviousPage), - * which this base deliberately does NOT implement — not even as single-page - * defaults — so a brand that forgets pagination fails at declaration time - * instead of silently answering "one page, no next page". + * (getFirstRecordIndex, getLastRecordIndex, getRecordsTotalCount, + * getRecordsLimitation) — the four methods that read a brand's own + * pagination columns — which this base deliberately does NOT implement — + * not even as single-page defaults — so a brand that forgets pagination + * fails at declaration time instead of silently answering "one page, no + * next page". The seam is drawn at the wire: a brand answers only what its + * columns say, and this base does every arithmetic derivation from those + * four answers (getCurrentPageNumber, hasNextPage, hasPreviousPage, + * getNextPageNumber, getPreviousPageNumber, getNumberOfPages included — + * RSRMID-2943 moved them here because they read no column of their own). * * None of the members in those last two groups is declared abstract *here*: they * are interface methods this base simply never implements, so every concrete - * brand must supply them. Do not add base defaults for the pagination primitives - * — see docs/agents/architecture.md for why the seam is drawn there, and - * tests/ResponsePaginationSeamTest.php, which refuses it. + * brand must supply them. Do not add base defaults for the four pagination + * primitives — see docs/agents/architecture.md for why the seam is drawn there, + * and tests/ResponsePaginationSeamTest.php, which refuses it. * * CNR\Response and IBS\Response both extend this as siblings — mirroring the * AbstractClient / AbstractSocketConfig / AbstractResponseTemplateManager / @@ -336,12 +341,14 @@ public function getHash(): array * Register an already-constructed column into the list bookkeeping. * * The bookkeeping ($columns/$columnKeys/$columnIndex) is identical for every - * brand; what differs is the column's value type. Rather than a param-typed - * newColumn() factory — which cannot stay type-clean under PHPStan L9 / Psalm - * L1, because CNR columns take string[] while IBS columns take mixed[] and a - * shared factory would have to narrow one into the other — each brand's - * addColumn() builds its own correctly-typed Column locally and hands the - * finished instance here, so this shared helper never sees the brand types. + * brand, and both brands build the same shared CNIC\Column: CNR responses + * are plaintext (always strings) and IBS/Moniker responses are JSON + * (arbitrary values, nested arrays and objects included), a difference + * expressed as a native return type on ColumnInterface::getStringByIndex() + * rather than a per-brand Column subclass. Each brand's addColumn() still + * builds its Column locally and hands the finished instance here, so this + * shared helper never has to construct one itself — see + * IBS\Response::addColumn()/CNR\Response::addColumn(). * * A repeated column name is refused rather than half-registered * (RSRMID-2939). The three lists are one data structure: $columns/$columnKeys @@ -378,7 +385,7 @@ protected function registerColumn(ColumnInterface $col): static * Add a record to the record list. * * Protected since RSRMID-2939: a record added after construction changed - * getRecordsCount() and, through it, the four pagination getters IBS derives + * getRecordsCount() and, through it, the pagination getters IBS derives * from it (getRecordsTotalCount/getRecordsLimitation/getLastRecordIndex/ * getNumberOfPages) — so a caller could silently repaginate a finished * response. Only {@see assembleRecords()} calls this. @@ -456,31 +463,158 @@ public function getCommandPlain(): string } /** - * Get Page Number of next list query + * Get Page Number of current List Query, derived from the offset grid. + * + * A pure function of {@see getFirstRecordIndex()} and + * {@see getRecordsLimitation()} — both wire-column primitives every brand + * answers for itself — so this needs no brand override (RSRMID-2943). + * `null` when either is unavailable, or when the limit is non-positive: a + * non-positive window size has no meaningful page number. + */ + #[\Override] + public function getCurrentPageNumber(): ?int + { + $first = $this->getFirstRecordIndex(); + $limit = $this->getRecordsLimitation(); + if ($first === null || $limit === null || $limit <= 0) { + return null; + } + return intdiv($first, $limit) + 1; + } + + /** + * Check if this list query has a next page. + * + * Answered from the offset grid directly — `LAST + 1 < TOTAL` — rather than + * from page numbers, so it agrees with {@see getNextPageNumber()} even when + * the current window is not aligned to a page boundary (e.g. FIRST=50, + * LIMIT=100 is "page 1" but its next request starts at 150, not 200). + * + * An **empty** window is the case to keep in mind: CNR answers one by + * echoing `LAST = FIRST` (with `COUNT = 0`), rather than by omitting LAST or + * reporting a row index. Observed shapes, all `QueryDomainList`: + * + * FIRST=0, LIMIT=0 -> count=0, first=0, last=0, total=1825820 + * FIRST=2000000, LIMIT=0 -> count=0, first=2000000, last=2000000, total=1825824 + * FIRST=20000000, LIMIT=10 -> count=0, first=20000000, last=20000000, total=1825824 + * + * The third self-terminates on the arithmetic below, because LAST echoes an + * offset far past TOTAL. The first two do not: `LAST + 1 < TOTAL` holds, and + * without a gate `CNR\Client::requestNextResponsePage()` would advance to + * `FIRST = 1` and re-walk the list from near the start. What stops them is + * the **non-positive LIMIT** gate — the older of the two guards here, which + * `CNR\Client` has relied on to terminate since before the offset grid + * existed (see tests/CNR/ClientTest.php testRequestNextResponsePageZeroLimit). + * + * The `LAST < FIRST` gate is **defensive only** — no observed CNR response + * does it, precisely because an empty window echoes `LAST = FIRST`. It pins + * the invariant the client's advance depends on: since `LAST >= FIRST` + * always, `FIRST = LAST + 1` strictly increases and the walk is monotonic. + * A future wire change (or a substitute parser) that broke that would send + * pagination backwards rather than failing, so it is refused here. + * + * Note that {@see getRecordsCount()} is NOT a usable gate, however much "an + * empty window has no next page" sounds like the same statement: + * {@see assembleRecords()} sizes the record list across *every* column, + * pagination columns included, so a CNR response carrying nothing but + * COLUMN/COUNT/FIRST/LAST/LIMIT/TOTAL still reports one record. + */ + #[\Override] + public function hasNextPage(): bool + { + $limit = $this->getRecordsLimitation(); + if ($limit === null || $limit <= 0) { + return false; + } + $first = $this->getFirstRecordIndex(); + $last = $this->getLastRecordIndex(); + $total = $this->getRecordsTotalCount(); + if ($first === null || $last === null || $total === null) { + return false; + } + if ($last < $first) { + return false; + } + return $last + 1 < $total; + } + + /** + * Check if this list query has a previous page. + * + * Answered from the offset grid directly — `FIRST > 0` — for the same + * reason as {@see hasNextPage()}: an unaligned window still has a + * well-defined "before it" even though it does not sit on a page boundary. + * + * The same LIMIT<=0 gate as {@see hasNextPage()} applies, for the same + * reason: a non-positive window size cannot page backward either. + */ + #[\Override] + public function hasPreviousPage(): bool + { + $limit = $this->getRecordsLimitation(); + if ($limit === null || $limit <= 0) { + return false; + } + $first = $this->getFirstRecordIndex(); + return $first !== null && $first > 0; + } + + /** + * Get Page Number of next list query. + * + * Computed from the *offset* the next request will actually start at + * (`getLastRecordIndex() + 1`) rather than from `getCurrentPageNumber() + 1`. + * The two agree on every window this can be asked about — for a full window + * LAST + 1 is FIRST + LIMIT, so `intdiv(LAST + 1, LIMIT) + 1` reduces to + * `intdiv(FIRST, LIMIT) + 2`; a short window only occurs at the tail, where + * {@see hasNextPage()} is already false. The offset form is used anyway for + * two reasons: it is the same grid {@see hasNextPage()} answers from, so the + * predicate and this getter cannot drift apart under a later edit, and it + * mirrors {@see getPreviousPageNumber()}, where the offset form and + * `getCurrentPageNumber() - 1` genuinely do differ on an unaligned window. + * + * The value is a page number over the aligned grid, which stays a derived + * view: for an unaligned window (FIRST=50, LIMIT=100) the request offsets + * are exact and the page number is the page that offset lands on. + * + * The `$limit`/`$last` null-checks below are unreachable while + * {@see hasNextPage()} holds true — it already required both to be + * non-null and `$limit` positive — but PHPStan level 9 cannot see across + * that method boundary, so they stay to keep the return type honestly + * `?int` rather than asserting past the analyser. Do not "simplify" them + * away. */ #[\Override] public function getNextPageNumber(): ?int { - $cp = $this->getCurrentPageNumber(); - if ($cp === null) { + if (!$this->hasNextPage()) { return null; } - $page = $cp + 1; - if ($page > $this->getNumberOfPages()) { + $limit = $this->getRecordsLimitation(); + $last = $this->getLastRecordIndex(); + if ($limit === null || $limit <= 0 || $last === null) { return null; } - return $page; + return intdiv($last + 1, $limit) + 1; } /** - * Get the number of pages available for this list query + * Get the number of pages available for this list query. + * + * `0` when either total or limit is unavailable and this response holds no + * records (nothing to page through); `1` when it holds records but is not + * itself a paginated list (an implicit single page, mirroring IBS's + * always-one-page model). Otherwise the ceiling of total/limit. */ #[\Override] public function getNumberOfPages(): int { $t = $this->getRecordsTotalCount(); $limit = $this->getRecordsLimitation(); - if ($t && $limit) { + if ($t === null || $limit === null) { + return $this->getRecordsCount() === 0 ? 0 : 1; + } + if ($t > 0 && $limit > 0) { return (int)ceil($t / $limit); } return 0; @@ -507,20 +641,32 @@ public function getPagination(): array } /** - * Get Page Number of previous list query + * Get Page Number of previous list query. + * + * Computed from the offset the previous request would start at + * (`max(0, FIRST - LIMIT)`), not from `getCurrentPageNumber() - 1`: for an + * unaligned window the two disagree the same way {@see getNextPageNumber()}'s + * do, and the offset form is the one that matches what would actually be + * requested. For an aligned FIRST both forms reduce to the same classic + * value. + * + * The null-checks below are unreachable while {@see hasPreviousPage()} + * holds true — it already required both to be non-null and `$limit` + * positive — but stay for the same PHPStan-level-9 reason documented on + * {@see getNextPageNumber()}. Do not "simplify" them away. */ #[\Override] public function getPreviousPageNumber(): ?int { - $cp = $this->getCurrentPageNumber(); - if ($cp === null) { + if (!$this->hasPreviousPage()) { return null; } - $cp -= 1; - if ($cp === 0) { + $first = $this->getFirstRecordIndex(); + $limit = $this->getRecordsLimitation(); + if ($first === null || $limit === null || $limit <= 0) { return null; } - return $cp; + return intdiv(max(0, $first - $limit), $limit) + 1; } /** diff --git a/src/CNR/Client.php b/src/CNR/Client.php index eca3d254..b9fa5c95 100644 --- a/src/CNR/Client.php +++ b/src/CNR/Client.php @@ -192,18 +192,23 @@ public function requestNextResponsePage(Response $currentPage): ?Response // "is there a next page?" lives in one place (Response::hasNextPage()) // rather than being re-derived from total/limit arithmetic here. This // also subsumes the former LIMIT<=0 guard: a non-positive page size makes - // getCurrentPageNumber() null, so hasNextPage() returns false and - // requestAllResponsePages() terminates instead of re-requesting the same - // page forever (see testRequestNextResponsePageZeroLimit). + // hasNextPage() return false, so requestAllResponsePages() terminates + // instead of re-requesting the same page forever (see + // testRequestNextResponsePageZeroLimit). + // + // The advance itself is the response's own next offset — LAST + 1 — + // rather than command FIRST + LIMIT: identical to the old arithmetic for + // an aligned page, but correct for an unaligned one, and it no longer + // depends on the caller having sent FIRST at all. if (!$currentPage->hasNextPage()) { return null; } - $first = 0; - if (array_key_exists("FIRST", $mycmd)) { - $first = (int) $mycmd["FIRST"]; - } $limit = $currentPage->getRecordsLimitation(); - $mycmd["FIRST"] = $first + $limit; + $last = $currentPage->getLastRecordIndex(); + if ($limit === null || $limit <= 0 || $last === null) { + return null; + } + $mycmd["FIRST"] = $last + 1; $mycmd["LIMIT"] = $limit; return $this->request($mycmd); } diff --git a/src/CNR/Column.php b/src/CNR/Column.php deleted file mode 100644 index e76ceb50..00000000 --- a/src/CNR/Column.php +++ /dev/null @@ -1,36 +0,0 @@ - - * @psalm-api - * @package CNIC\CNR - */ -class Column extends BaseColumn -{ - /** - * Get column data at given index - */ - #[\Override] - public function getDataByIndex(int $recordIndex): string|null - { - return parent::getDataByIndex($recordIndex); - } -} diff --git a/src/CNR/Response.php b/src/CNR/Response.php index 8d3a182e..5fa75669 100755 --- a/src/CNR/Response.php +++ b/src/CNR/Response.php @@ -10,9 +10,9 @@ namespace CNIC\CNR; use CNIC\AbstractResponse; -use CNIC\CNR\Column; use CNIC\CNR\ResponseParser as RP; use CNIC\CNR\ResponseTranslator as RT; +use CNIC\Column; use CNIC\ColumnInterface; use CNIC\Exception\UnsupportedFeatureException; use CNIC\ExtendedResponseInterface; @@ -24,11 +24,11 @@ * CNR Response * * Extends the shared AbstractResponse with the CNR wire specifics — the - * translate()/populate() hooks, the CODE/DESCRIPTION status accessors, the - * CNR Column type and the column-driven pagination primitives — and adds the - * richer CNR-only capabilities declared on {@see ExtendedResponseInterface} - * (telemetry, transient/pending status and the list-hash projection) that flat - * platforms like IBS/Moniker do not provide. + * translate()/populate() hooks, the CODE/DESCRIPTION status accessors and the + * column-driven pagination primitives — and adds the richer CNR-only + * capabilities declared on {@see ExtendedResponseInterface} (telemetry, + * transient/pending status and the list-hash projection) that flat platforms + * like IBS/Moniker do not provide. * * @psalm-api * @package CNIC\CNR @@ -98,22 +98,29 @@ protected function populate(string $raw, ResponseParserInterface $parser, array } /** - * Narrow one parsed PROPERTY entry to the string list a CNR Column takes. + * Narrow one parsed PROPERTY entry to the string list a CNR column takes. * * The CNR wire format is textual, so every cell of a real response is already * a string and this rejects nothing. It exists because the parse step is a - * seam: the contract returns array, so the brand's own shape has - * to be re-established here rather than read off the concrete parser's return - * type — which is also why CNR\Column binds its value type to string. + * seam: the contract returns array, so the brand's own shape + * has to be re-established here rather than read off the concrete parser's + * return type. * - * A parser handing CNR anything else is contradicting that binding, so both - * checks **throw** rather than skipping or coercing: silently dropping data - * would be the no-op this project rules out, and coercing would invent a value - * the wire could never carry. Keep the container check as strict as the cells — - * casting it with (array) instead quietly turns a bare string into a one-cell - * column while a bare int still throws one line later, coercing a bad container - * while refusing a bad cell. Unreachable with either brand parser; reachable - * only from a substitute, where it is a programming error and should say so. + * **This outlived the type it was introduced to defend (RSRMID-2942).** It + * once backed `CNR\Column`'s `string` binding; that class is gone, and the + * check stays for two reasons that never depended on it. A parser handing CNR + * a non-string is a *programming error*, and the only parsers that can are + * substitutes — so failing loudly at construction beats surfacing as a null + * three calls later. And keeping CNR cells string-guaranteed is what lets + * {@see \CNIC\ColumnInterface::getStringByIndex()} never answer null-for-wrong-type + * on this brand: the accessor is only as self-explaining as the data behind it. + * + * Both checks **throw** rather than skipping or coercing: silently dropping + * data would be the no-op this project rules out, and coercing would invent a + * value the wire could never carry. Keep the container check as strict as the + * cells — casting it with (array) instead quietly turns a bare string into a + * one-cell column while a bare int still throws one line later, coercing a bad + * container while refusing a bad cell. * * Note the deliberate asymmetry with populate(): a *missing or non-array* * PROPERTY block yields no columns rather than throwing, because most CNR @@ -125,7 +132,7 @@ protected function populate(string $raw, ResponseParserInterface $parser, array * the loop leaves only the one MixedAssignment below to suppress — the same * trade already made in AbstractResponse::assembleRecords(). * @return string[] - * @throws UnsupportedFeatureException if the entry is not a list, or a cell is not a string + * @throws UnsupportedFeatureException if the entry is not an array, or a cell is not a string */ private static function stringCells(string $key, mixed $values): array { @@ -231,14 +238,19 @@ public function isPending(): bool /** * Add a column to the column list * - * Protected since RSRMID-2939 — see IBS\Response::addColumn() for why, and - * AbstractResponse::registerColumn() for why each brand builds its own Column - * here rather than through a shared factory. Note the - * `@psalm-suppress MoreSpecificImplementedParamType` this carried is gone with - * the interface declaration: with no parent naming addColumn(), the - * string-valued `$data` narrows nothing and the suppression would never fire - * (a never-applied suppression fails the lint). - * @param string[] $data array of column data + * CNR responses are plaintext, so column values are always strings — + * stringCells() guarantees it before this is called. The shared CNIC\Column + * is nonetheless used as-is, exactly like IBS\Response::addColumn(): the + * value type is expressed to consumers by getStringByIndex(), not by a + * column subclass (RSRMID-2942). See IBS\Response::addColumn() for why each + * brand builds its Column locally and hands the finished instance to the + * shared registerColumn() bookkeeping rather than to a shared factory. + * + * Protected since RSRMID-2939, and called only from populate(): records are + * assembled from the columns once, at the end of construction, so a column + * added afterwards was absent from every record — present in getColumns() and + * getColumnKeys(), invisible to getRecord()/getRecords() and to iteration. + * @param string[] $data array of column data, already narrowed by stringCells() */ protected function addColumn(string $columnName, array $data): static { @@ -264,31 +276,17 @@ protected function newResponseParser(): ResponseParserInterface return new RP(); } - /** - * Get Page Number of current List Query - */ - #[\Override] - public function getCurrentPageNumber(): ?int - { - $first = $this->getFirstRecordIndex(); - $limit = $this->getRecordsLimitation(); - if ($first !== null && $limit) { - return intdiv($first, $limit) + 1; - } - return null; - } - /** * Coerce a raw pagination column value to a base-10 integer. * - * getDataByIndex() is typed mixed on ColumnInterface (IBS columns may - * carry nested arrays/objects); the pagination columns FIRST/LAST/TOTAL/ - * LIMIT always hold a scalar numeric string, so anything non-scalar (incl. - * a missing value) yields null and lets the caller fall back. + * getStringByIndex() already narrows to ?string (CNR cells are always + * strings, unlike IBS's, which may carry nested arrays/objects); this + * just re-narrows a missing/absent value to null so the caller can fall + * back. */ - private function columnInt(mixed $value): ?int + private function columnInt(?string $value): ?int { - return is_scalar($value) ? intval($value, 10) : null; + return $value === null ? null : intval($value, 10); } /** @@ -299,7 +297,7 @@ public function getFirstRecordIndex(): ?int { $col = $this->getColumn("FIRST"); if ($col instanceof ColumnInterface) { - return $this->columnInt($col->getDataByIndex(0)) ?? 0; + return $this->columnInt($col->getStringByIndex(0)) ?? 0; } if ($this->getRecordsCount() !== 0) { return 0; @@ -315,7 +313,7 @@ public function getLastRecordIndex(): ?int { $col = $this->getColumn("LAST"); if ($col instanceof ColumnInterface) { - $l = $this->columnInt($col->getDataByIndex(0)); + $l = $this->columnInt($col->getStringByIndex(0)); if ($l !== null) { return $l; } @@ -355,61 +353,33 @@ public function getListHash(): array } /** - * Get total count of records available for the list query + * Get total count of records available for the list query, or `null` when + * this response carries no TOTAL column (a non-list response). + * + * No `getRecordsCount()` fallback (RSRMID-2943): a non-list response now + * reports "no total" honestly instead of a count that only happened to + * equal the record count. */ #[\Override] - public function getRecordsTotalCount(): int + public function getRecordsTotalCount(): ?int { $col = $this->getColumn("TOTAL"); - if ($col instanceof ColumnInterface) { - $t = $this->columnInt($col->getDataByIndex(0)); - if ($t !== null) { - return $t; - } - } - return $this->getRecordsCount(); + return $col instanceof ColumnInterface ? $this->columnInt($col->getStringByIndex(0)) : null; } /** - * Get limit(ation) setting of the current list query - * This is the count of requested rows + * Get limit(ation) setting of the current list query — the count of + * requested rows — or `null` when this response carries no LIMIT column. + * + * No `getRecordsCount()` fallback (RSRMID-2943), for the same reason as + * {@see getRecordsTotalCount()}: `0` is a real, requested limit and must + * stay distinguishable from "this response carries no LIMIT column at + * all". */ #[\Override] - public function getRecordsLimitation(): int + public function getRecordsLimitation(): ?int { $col = $this->getColumn("LIMIT"); - if ($col instanceof ColumnInterface) { - $l = $this->columnInt($col->getDataByIndex(0)); - if ($l !== null) { - return $l; - } - } - return $this->getRecordsCount(); - } - - /** - * Check if this list query has a next page - */ - #[\Override] - public function hasNextPage(): bool - { - $cp = $this->getCurrentPageNumber(); - if ($cp === null) { - return false; - } - return ($cp + 1 <= $this->getNumberOfPages()); - } - - /** - * Check if this list query has a previous page - */ - #[\Override] - public function hasPreviousPage(): bool - { - $cp = $this->getCurrentPageNumber(); - if ($cp === null) { - return false; - } - return ($cp - 1 > 0); + return $col instanceof ColumnInterface ? $this->columnInt($col->getStringByIndex(0)) : null; } } diff --git a/src/CNR/SessionCapable.php b/src/CNR/SessionCapable.php index de7dad57..63914093 100644 --- a/src/CNR/SessionCapable.php +++ b/src/CNR/SessionCapable.php @@ -36,8 +36,7 @@ public function login(): Response $this->getSocketConfig()->setPersistent(true); $rr = $this->request(); if ($rr->isSuccess()) { - $col = $rr->getColumn("SESSIONID"); - $this->setSession($col instanceof Column ? $col->getData()[0] : ""); + $this->setSession($rr->getColumn("SESSIONID")?->getStringByIndex(0) ?? ""); } $this->getSocketConfig()->setPersistent(false); return $rr; diff --git a/src/Column.php b/src/Column.php index c5f49c15..d46bbedc 100644 --- a/src/Column.php +++ b/src/Column.php @@ -17,11 +17,13 @@ * is instantiated directly by each Response's addColumn(). What differs between * brands is only the *value type*: CNR responses are plaintext and carry strings, * IBS/Moniker responses are JSON and carry arbitrary values (nested arrays and - * objects included). That difference is expressed by the TValue template - * parameter, so `CNR\Column extends Column` gains the narrower - * `getDataByIndex(): ?string` without re-implementing anything. + * objects included). That difference used to be expressed as a `TValue` template + * parameter narrowed by a brand subclass, but the narrowing never survived + * {@see ColumnInterface} — which is not generic — so no consumer holding the + * interface (every reachable path: getColumn()/getColumns()) ever saw it. It is + * expressed instead as a native return type on the interface itself, exactly as + * {@see self::getDateTimeByIndex()} already narrows: {@see self::getStringByIndex()}. * - * @template TValue * @psalm-api * @package CNIC */ @@ -35,7 +37,7 @@ class Column implements ColumnInterface /** * Constructor * - * @param array $data Column Data + * @param array $data Column Data */ public function __construct( private readonly string $columnName, @@ -55,7 +57,7 @@ public function getKey(): string /** * Get column data - * @return array + * @return array */ #[\Override] public function getData(): array @@ -65,7 +67,6 @@ public function getData(): array /** * Get column data at given index - * @return TValue|null */ #[\Override] public function getDataByIndex(int $recordIndex): mixed @@ -81,6 +82,22 @@ private function hasDataIndex(int $recordIndex): bool return ($recordIndex >= 0 && $recordIndex < $this->length); } + /** + * Get column data at given index, narrowed to a string. + * + * Returns `null` for an out-of-range index or a non-string value. CNR + * cells are always strings; IBS/Moniker JSON cells may be nested arrays + * or objects, which yield null here — use {@see self::getDataByIndex()} + * for the raw value in that case. + */ + #[\Override] + public function getStringByIndex(int $recordIndex): ?string + { + /** @psalm-suppress MixedAssignment getDataByIndex() returns mixed by design; is_string() narrows it below */ + $value = $this->getDataByIndex($recordIndex); + return is_string($value) ? $value : null; + } + /** * Get column data at given index, parsed as a date/time value. * @@ -93,6 +110,7 @@ private function hasDataIndex(int $recordIndex): bool #[\Override] public function getDateTimeByIndex(int $recordIndex): ?ApiDateTime { + /** @psalm-suppress MixedAssignment getDataByIndex() returns mixed by design; is_string() narrows it below */ $value = $this->getDataByIndex($recordIndex); return is_string($value) ? ApiDateTime::tryFrom($value) : null; } diff --git a/src/ColumnInterface.php b/src/ColumnInterface.php index b6e5da44..88442d91 100644 --- a/src/ColumnInterface.php +++ b/src/ColumnInterface.php @@ -42,6 +42,16 @@ public function getData(): array; */ public function getDataByIndex(int $recordIndex): mixed; + /** + * Get column data at given index, narrowed to a string. + * + * Returns `null` for an out-of-range index or a non-string value. CNR + * cells are always strings; IBS/Moniker JSON cells may be nested arrays + * or objects, which yield null here — use {@see self::getDataByIndex()} + * for the raw value in that case. + */ + public function getStringByIndex(int $recordIndex): ?string; + /** * Get column data at given index, parsed as a date/time value. * diff --git a/src/IBS/Response.php b/src/IBS/Response.php index 7d2f3130..8413c85d 100755 --- a/src/IBS/Response.php +++ b/src/IBS/Response.php @@ -195,11 +195,10 @@ public function isSuccess(): bool * Add a column to the column list * * IBS responses are JSON, so column values are arbitrary (nested arrays and - * objects included) and the shared CNIC\Column is used as-is. CNR narrows - * the same class to string values; that divergence in the *constructor* - * value type is why a param-typed newColumn() factory would not stay - * type-clean, and why this builds its column locally and hands the finished - * instance to the shared registerColumn() bookkeeping — see registerColumn(). + * objects included); the shared CNIC\Column is used as-is, exactly like + * CNR\Response::addColumn(). Both brands build their column locally and hand + * the finished instance to the shared registerColumn() bookkeeping — see + * registerColumn(). * * Protected since RSRMID-2939, and called only from populate(): records are * assembled from the columns once, at the end of construction, so a column @@ -231,15 +230,6 @@ protected function newResponseParser(): ResponseParserInterface return new RP(); } - /** - * Get Page Number of current List Query - */ - #[\Override] - public function getCurrentPageNumber(): ?int - { - return 1; - } - /** * Get Index of first row in this response */ @@ -274,39 +264,32 @@ public function getLastRecordIndex(): ?int } /** - * Get total count of records available for the list query + * Get total count of records available for the list query. + * + * IBS does not paginate — it returns one full result set — so there is no + * TOTAL column to fall back from. `getRecordsCount()` IS the true total + * here, not a fallback standing in for an absent one: total == limit == + * count is the whole truth for this brand. Declared `?int` only because + * {@see ResponseInterface} does; this brand never answers `null`. */ #[\Override] - public function getRecordsTotalCount(): int + public function getRecordsTotalCount(): ?int { return $this->getRecordsCount(); } /** - * Get limit(ation) setting of the current list query - * This is the count of requested rows + * Get limit(ation) setting of the current list query — the count of + * requested rows. + * + * Same reasoning as {@see getRecordsTotalCount()}: IBS has no limit/offset + * concept, so the record count is the genuine answer rather than a stand-in + * for a missing LIMIT column. Declared `?int` only because + * {@see ResponseInterface} does; this brand never answers `null`. */ #[\Override] - public function getRecordsLimitation(): int + public function getRecordsLimitation(): ?int { return $this->getRecordsCount(); } - - /** - * Check if this list query has a next page - */ - #[\Override] - public function hasNextPage(): bool - { - return false; - } - - /** - * Check if this list query has a previous page - */ - #[\Override] - public function hasPreviousPage(): bool - { - return false; - } } diff --git a/src/Record.php b/src/Record.php index 664cad3e..ae7b98e2 100644 --- a/src/Record.php +++ b/src/Record.php @@ -70,6 +70,22 @@ private function hasData(string $columnName): bool return array_key_exists($columnName, $this->data); } + /** + * Get row data for given column, narrowed to a string. + * + * Returns `null` for a missing key or a non-string value. CNR cells are + * always strings; IBS/Moniker JSON cells may be nested arrays or + * objects, which yield null here — use {@see self::getDataByKey()} for + * the raw value in that case. + */ + #[\Override] + public function getStringByKey(string $columnName): ?string + { + /** @psalm-suppress MixedAssignment getDataByKey() returns mixed by design; is_string() narrows it below */ + $value = $this->getDataByKey($columnName); + return is_string($value) ? $value : null; + } + /** * Get row data for given column, parsed as a date/time value. * diff --git a/src/RecordInterface.php b/src/RecordInterface.php index 90c7592c..aa56a25d 100644 --- a/src/RecordInterface.php +++ b/src/RecordInterface.php @@ -35,6 +35,16 @@ public function getData(): array; */ public function getDataByKey(string $columnName): mixed; + /** + * Get row data for given column, narrowed to a string. + * + * Returns `null` for a missing key or a non-string value. CNR cells are + * always strings; IBS/Moniker JSON cells may be nested arrays or + * objects, which yield null here — use {@see self::getDataByKey()} for + * the raw value in that case. + */ + public function getStringByKey(string $columnName): ?string; + /** * Get row data for given column, parsed as a date/time value. * diff --git a/src/ResponseInterface.php b/src/ResponseInterface.php index e138caac..b838d152 100755 --- a/src/ResponseInterface.php +++ b/src/ResponseInterface.php @@ -202,24 +202,41 @@ public function getIterator(): \Traversable; public function getRecordsCount(): int; /** - * Get total count of records available for the list query, or the count of - * records for a non-list response + * Get total count of records available for the list query, or `null` when + * the response carries no TOTAL column (a non-list response) */ - public function getRecordsTotalCount(): int; + public function getRecordsTotalCount(): ?int; /** * Get limit(ation) setting of the current list query — the count of - * requested rows + * requested rows — or `null` when the response carries no LIMIT column. + * `0` is a distinct, meaningful value (LIMIT=0 was requested); it no + * longer collides with "absent" */ - public function getRecordsLimitation(): int; + public function getRecordsLimitation(): ?int; /** - * Check if this list query has a next page + * Check if this list query has a next page. + * + * Answered from record offsets — `getLastRecordIndex() + 1 < getRecordsTotalCount()` + * — not from page arithmetic, so it agrees with {@see getNextPageNumber()} even + * when the current window is not aligned to a page boundary. + * + * That comparison assumes a window that actually holds rows. An empty one + * reports a last index that is not a row index — CNR echoes + * `LAST = FIRST` — so with a non-positive limit `LAST + 1 < TOTAL` can still + * hold and the "next" offset walks back to the start of the list. Refuse a + * non-positive limit before the arithmetic runs. `AbstractResponse` handles + * this for every brand; an implementation writing its own must too. */ public function hasNextPage(): bool; /** - * Check if this list query has a previous page + * Check if this list query has a previous page. + * + * Answered from record offsets — `getFirstRecordIndex() > 0` — not from page + * arithmetic, so an unaligned window (e.g. FIRST=50, LIMIT=100) correctly + * reports true instead of hiding behind a whole-page-number comparison. */ public function hasPreviousPage(): bool; } diff --git a/tests/CNR/ClientTest.php b/tests/CNR/ClientTest.php index 2b34979d..68701f93 100644 --- a/tests/CNR/ClientTest.php +++ b/tests/CNR/ClientTest.php @@ -802,6 +802,31 @@ public function testRequestNextResponsePageLastPage(): void $this->assertNull(self::$cl->requestNextResponsePage($r)); } + public function testRequestNextResponsePagePastTheEnd(): void + { + // RSRMID-2943 regression guard, consumer-facing half. A caller can hand + // this method a window it requested past the end of the list itself — + // requestAllResponsePages() never builds one, since it stops while + // LAST+1 < TOTAL still holds. Verbatim capture: CNR answers such a + // window with COUNT=0 and LAST echoing FIRST, which is what makes the + // offset comparison terminate (20000001 < 1825824 is false) with a + // POSITIVE limit, where the LIMIT<=0 guard does not apply. + $tpls = (new RTM())->addTemplate( + "listPastTheEnd", + "[RESPONSE]\r\nPROPERTY[COLUMN][0]=domain\r\nPROPERTY[COUNT][0]=0\r\nPROPERTY[FIRST][0]=20000000\r\n" + . "PROPERTY[LAST][0]=20000000\r\nPROPERTY[LIMIT][0]=10\r\nPROPERTY[TOTAL][0]=1825824\r\n" + . "DESCRIPTION=Command completed successfully\r\nCODE=200\r\nQUEUETIME=0\r\nRUNTIME=15.892\r\nEOF\r\n" + ); + $r = new R("listPastTheEnd", [ + "COMMAND" => "QueryDomainList", + "FIRST" => "20000000", + "LIMIT" => "10" + ], templates: $tpls); + $this->assertTrue($r->isSuccess()); + $this->assertFalse($r->hasNextPage()); + $this->assertNull(self::$cl->requestNextResponsePage($r)); + } + public function testRequestAllResponsePagesOk(): void { self::$tape->useCassette("all-pages"); diff --git a/tests/CNR/ColumnTest.php b/tests/CNR/ColumnTest.php deleted file mode 100644 index 0f42fc79..00000000 --- a/tests/CNR/ColumnTest.php +++ /dev/null @@ -1,65 +0,0 @@ -assertSame("DOMAIN", $col->getKey()); - } - - public function testGetData(): void - { - $col = new Column("DOMAIN", self::DOMAINS); - $this->assertSame(self::DOMAINS, $col->getData()); - $this->assertSame(3, $col->length); - } - - /** - * The narrowed return type is the entire reason this subclass exists: a - * CNR-typed column yields ?string where the shared base yields mixed. - */ - public function testGetDataByIndexReturnsStringOrNull(): void - { - $col = new Column("DOMAIN", self::DOMAINS); - $this->assertSame("mydomain1.com", $col->getDataByIndex(0)); - $this->assertSame("mydomain3.com", $col->getDataByIndex(2)); - $this->assertNull($col->getDataByIndex(3)); - $this->assertNull($col->getDataByIndex(-1)); - } - - /** - * Guards the addColumn() wiring: CNR\Response must build the CNR column - * type, not the shared base, so consumers keep the narrowed return type. - */ - public function testResponseBuildsCnrColumnType(): void - { - $raw = "[RESPONSE]\r\nPROPERTY[DOMAIN][0]=mydomain1.com\r\n" - . "PROPERTY[DOMAIN][1]=mydomain2.com\r\nDESCRIPTION=Command completed successfully\r\n" - . "CODE=200\r\nEOF\r\n"; - $col = (new R($raw))->getColumn("DOMAIN"); - $this->assertInstanceOf(Column::class, $col); - $this->assertSame(["mydomain1.com", "mydomain2.com"], $col->getData()); - $this->assertSame("mydomain2.com", $col->getDataByIndex(1)); - $this->assertNull($col->getDataByIndex(2)); - } -} diff --git a/tests/CNR/ResponseTest.php b/tests/CNR/ResponseTest.php index 6e742463..3ccb19f9 100644 --- a/tests/CNR/ResponseTest.php +++ b/tests/CNR/ResponseTest.php @@ -364,6 +364,159 @@ public function testGetPreviousPageNumberRows(): void $this->assertNull($r->getPreviousPageNumber()); } + public function testUnalignedOffsetWindowPredicatesAndPageNumbersAgree(): void + { + // RSRMID-2943: FIRST=50 is not a multiple of LIMIT=100, so "page 1" is + // an unaligned window (offsets 50..149) inside a 1858-record list. Pins + // that hasNextPage()/hasPreviousPage() and the page-number getters are + // all derived from the same offset grid, so they cannot disagree: the + // next request starts at LAST+1=150, which lands on page + // intdiv(150,100)+1=2, and the previous one starts at + // max(0,50-100)=0, landing on page 1. + $tpls = (new RTM())->addTemplate( + "unalignedWindow", + "[RESPONSE]\r\nPROPERTY[TOTAL][0]=1858\r\nPROPERTY[FIRST][0]=50\r\n" + . "PROPERTY[COUNT][0]=100\r\nPROPERTY[LAST][0]=149\r\nPROPERTY[LIMIT][0]=100\r\n" + . "DESCRIPTION=Command completed successfully\r\nCODE=200\r\nQUEUETIME=0\r\nRUNTIME=0.023\r\nEOF\r\n" + ); + $r = new R("unalignedWindow", templates: $tpls); + $this->assertTrue($r->hasNextPage()); + $this->assertEquals(2, $r->getNextPageNumber()); + $this->assertTrue($r->hasPreviousPage()); + $this->assertEquals(1, $r->getPreviousPageNumber()); + $this->assertEquals(1, $r->getCurrentPageNumber()); + } + + public function testWindowAlreadyHoldingTheTailHasNoNextPage(): void + { + // The wasted-round-trip fix (RSRMID-2943): FIRST=50, LIMIT=100, + // LAST=149, TOTAL=150 — this window already holds the tail of the + // list (LAST+1 === TOTAL). The old predicate compared whole page + // numbers (getCurrentPageNumber() + 1 <= getNumberOfPages()) and said + // "true" here, because ceil(150/100) = 2 pages exist even though this + // window already covers every row — costing an empty follow-up + // request. The offset-grid predicate answers false directly. + $tpls = (new RTM())->addTemplate( + "windowHoldsTail", + "[RESPONSE]\r\nPROPERTY[TOTAL][0]=150\r\nPROPERTY[FIRST][0]=50\r\n" + . "PROPERTY[COUNT][0]=100\r\nPROPERTY[LAST][0]=149\r\nPROPERTY[LIMIT][0]=100\r\n" + . "DESCRIPTION=Command completed successfully\r\nCODE=200\r\nQUEUETIME=0\r\nRUNTIME=0.023\r\nEOF\r\n" + ); + $r = new R("windowHoldsTail", templates: $tpls); + $this->assertFalse($r->hasNextPage()); + $this->assertNull($r->getNextPageNumber()); + } + + public function testAbsentPaginationColumnsAreNowRepresentable(): void + { + // RSRMID-2943: a non-list response (no FIRST/LAST/LIMIT/TOTAL columns + // at all) that still carries rows used to have its total/limit fall + // back to the record count, indistinguishable from a real list whose + // total/limit genuinely equalled that count. Now it reports "absent" + // honestly, and derives a single implicit page from the record list. + $raw = implode("\r\n", [ + "[RESPONSE]", + "CODE=200", + "DESCRIPTION=Command completed successfully", + "PROPERTY[DOMAIN][0]=mydomain1.com", + "PROPERTY[DOMAIN][1]=mydomain2.com", + "EOF" + ]); + $r = new R($raw); + $this->assertNull($r->getRecordsTotalCount()); + $this->assertNull($r->getRecordsLimitation()); + $this->assertEquals(1, $r->getNumberOfPages()); + $this->assertFalse($r->hasNextPage()); + $this->assertNull($r->getCurrentPageNumber()); + } + + public function testZeroLimitIsDistinctFromAbsentLimit(): void + { + // RSRMID-2943: LIMIT=0 is a real, requested value (a caller explicitly + // asked for a zero-row window) and must stay distinguishable from "no + // LIMIT column at all" — the two used to collide because + // getRecordsLimitation() fell back to getRecordsCount() whenever the + // column was missing, and 0 was also what an empty record list + // produced. + // Verbatim capture of `QueryDomainList` with FIRST=0/LIMIT=0, COLUMN row + // included. The window is empty and CNR answers LAST = FIRST (= 0 here), + // not a row index — so LAST+1 < TOTAL holds and only the LIMIT<=0 gate + // stops requestNextResponsePage() advancing to offset 1. + $tpls = (new RTM())->addTemplate( + "zeroLimit", + "[RESPONSE]\r\nPROPERTY[COLUMN][0]=domain\r\nPROPERTY[COUNT][0]=0\r\nPROPERTY[FIRST][0]=0\r\n" + . "PROPERTY[LAST][0]=0\r\nPROPERTY[LIMIT][0]=0\r\nPROPERTY[TOTAL][0]=1825820\r\n" + . "DESCRIPTION=Command completed successfully\r\nCODE=200\r\nQUEUETIME=0\r\nRUNTIME=0.377\r\nEOF\r\n" + ); + $r = new R("zeroLimit", templates: $tpls); + $this->assertSame(0, $r->getRecordsLimitation()); + $this->assertFalse($r->hasNextPage()); + + $absent = new R("OK", templates: self::$tpls); + $this->assertNull($absent->getRecordsLimitation()); + } + + public function testPastTheEndWindowWithZeroLimitHasNoNextPage(): void + { + // Verbatim capture: FIRST past the end AND LIMIT=0. CNR echoes + // LAST = FIRST for the empty window, so LAST+1 < TOTAL is false here on + // the arithmetic alone — but TOTAL is the only thing making that true, + // and the LIMIT<=0 gate is what the client actually relies on. Pinned + // next to the FIRST=0/LIMIT=0 capture above because the two differ only + // in FIRST, which is exactly what shows LAST tracks FIRST rather than + // being a flat floor. + $tpls = (new RTM())->addTemplate( + "pastTheEndZeroLimit", + "[RESPONSE]\r\nPROPERTY[COLUMN][0]=domain\r\nPROPERTY[COUNT][0]=0\r\nPROPERTY[FIRST][0]=2000000\r\n" + . "PROPERTY[LAST][0]=2000000\r\nPROPERTY[LIMIT][0]=0\r\nPROPERTY[TOTAL][0]=1825824\r\n" + . "DESCRIPTION=Command completed successfully\r\nCODE=200\r\nQUEUETIME=0\r\nRUNTIME=18.906\r\nEOF\r\n" + ); + $r = new R("pastTheEndZeroLimit", templates: $tpls); + $this->assertSame(0, $r->getRecordsLimitation()); + $this->assertFalse($r->hasNextPage()); + $this->assertNull($r->getNextPageNumber()); + } + + public function testPastTheEndWindowWithPositiveLimitHasNoNextPage(): void + { + // Verbatim capture: FIRST=20000000 (past the end) with LIMIT=10, so the + // LIMIT<=0 gate does NOT apply and the offset arithmetic has to carry + // this one by itself. It does, because CNR echoes LAST = FIRST for the + // empty window: 20000001 < 1825824 is false. This is the shape that + // would break a predicate derived from whole page numbers instead. + $tpls = (new RTM())->addTemplate( + "pastTheEndLimited", + "[RESPONSE]\r\nPROPERTY[COLUMN][0]=domain\r\nPROPERTY[COUNT][0]=0\r\nPROPERTY[FIRST][0]=20000000\r\n" + . "PROPERTY[LAST][0]=20000000\r\nPROPERTY[LIMIT][0]=10\r\nPROPERTY[TOTAL][0]=1825824\r\n" + . "DESCRIPTION=Command completed successfully\r\nCODE=200\r\nQUEUETIME=0\r\nRUNTIME=15.892\r\nEOF\r\n" + ); + $r = new R("pastTheEndLimited", templates: $tpls); + $this->assertFalse($r->hasNextPage()); + $this->assertNull($r->getNextPageNumber()); + } + + public function testWindowEndingBeforeItStartsHasNoNextPage(): void + { + // Synthetic, and deliberately so: NO observed CNR response answers + // LAST < FIRST — an empty window echoes LAST = FIRST (see the two + // captures above). This pins the invariant the client's advance rests + // on rather than a shape the API produces: because LAST >= FIRST + // always, requestNextResponsePage()'s FIRST = LAST+1 strictly + // increases and the walk is monotonic. A wire change or a substitute + // parser that broke that would send pagination BACKWARD — re-listing + // the account from near the start — instead of failing, which is why + // hasNextPage() refuses it rather than trusting the arithmetic. + $tpls = (new RTM())->addTemplate( + "backwardWindow", + "[RESPONSE]\r\nPROPERTY[COLUMN][0]=domain\r\nPROPERTY[COUNT][0]=0\r\nPROPERTY[FIRST][0]=2000000\r\n" + . "PROPERTY[LAST][0]=0\r\nPROPERTY[LIMIT][0]=100\r\nPROPERTY[TOTAL][0]=1825824\r\n" + . "DESCRIPTION=Command completed successfully\r\nCODE=200\r\nQUEUETIME=0\r\nRUNTIME=0.377\r\nEOF\r\n" + ); + $r = new R("backwardWindow", templates: $tpls); + $this->assertFalse($r->hasNextPage()); + $this->assertNull($r->getNextPageNumber()); + } + public function testIteratingAResponseWithoutRecordsYieldsNothing(): void { $r = new R("OK", templates: self::$tpls); diff --git a/tests/ColumnTest.php b/tests/ColumnTest.php index 02d39b0b..9e898702 100644 --- a/tests/ColumnTest.php +++ b/tests/ColumnTest.php @@ -12,11 +12,12 @@ /** * Shared column behaviour, covered once for every brand. * - * CNIC\Column is what IBS/Moniker responses instantiate directly and what - * CNR\Column inherits, so these assertions are the single source of coverage - * for the key/data/length/bounds contract. Brand test classes only cover what - * is genuinely brand-specific (CNR's narrowed return type, the per-brand - * Response wiring). + * CNIC\Column is what every brand's Response instantiates directly — there is + * no per-brand Column subclass — so these assertions are the single source of + * coverage for the key/data/length/bounds contract, plus the getStringByIndex()/ + * getDateTimeByIndex() opt-in narrowing accessors declared on + * {@see \CNIC\ColumnInterface}. Brand test classes only cover what is + * genuinely brand-specific (the per-brand Response wiring). */ final class ColumnTest extends TestCase { @@ -42,10 +43,6 @@ public function testLength(): void public function testLengthOfEmptyColumn(): void { - // bound explicitly: an empty literal would otherwise infer TValue as - // never, which makes the out-of-bounds null a static certainty rather - // than the runtime behaviour under test - /** @var Column $col */ $col = new Column("empty", []); $this->assertSame(0, $col->length); $this->assertSame([], $col->getData()); @@ -91,11 +88,31 @@ public function testMixedScalarAndNestedData(): void $this->assertSame("another-scalar", $col->getDataByIndex(2)); } + // --- getStringByIndex() --- + + public function testGetStringByIndexOfStringValue(): void + { + $col = new Column("nameserver", self::NAMESERVERS); + $this->assertSame("ns1.ispapi.net", $col->getStringByIndex(0)); + } + + public function testGetStringByIndexOfNonStringValueIsNull(): void + { + $col = new Column("contacts", [["firstname" => "Middle", "lastname" => "Ware"]]); + $this->assertNull($col->getStringByIndex(0)); + } + + public function testGetStringByIndexOutOfRangeIsNull(): void + { + $col = new Column("nameserver", self::NAMESERVERS); + $this->assertNull($col->getStringByIndex(2)); + $this->assertNull($col->getStringByIndex(-1)); + } + // --- getDateTimeByIndex() --- public function testGetDateTimeByIndexParsesDashSeparatedValue(): void { - /** @var Column $col */ $col = new Column("expirationdate", ["2026-07-25 07:46:34"]); $dt = $col->getDateTimeByIndex(0); $this->assertInstanceOf(ApiDateTime::class, $dt); @@ -104,7 +121,6 @@ public function testGetDateTimeByIndexParsesDashSeparatedValue(): void public function testGetDateTimeByIndexParsesSlashSeparatedValue(): void { - /** @var Column $col */ $col = new Column("expirationdate", ["2030/07/17"]); $dt = $col->getDateTimeByIndex(0); $this->assertInstanceOf(ApiDateTime::class, $dt); @@ -113,7 +129,6 @@ public function testGetDateTimeByIndexParsesSlashSeparatedValue(): void public function testGetDateTimeByIndexOfDateOnlyValueHasNullTs(): void { - /** @var Column $col */ $col = new Column("expirationdate", ["2030/07/17"]); $dt = $col->getDateTimeByIndex(0); $this->assertInstanceOf(ApiDateTime::class, $dt); @@ -122,7 +137,6 @@ public function testGetDateTimeByIndexOfDateOnlyValueHasNullTs(): void public function testGetDateTimeByIndexOutOfRangeIsNull(): void { - /** @var Column $col */ $col = new Column("expirationdate", ["2030/07/17"]); $this->assertNull($col->getDateTimeByIndex(1)); $this->assertNull($col->getDateTimeByIndex(-1)); @@ -150,7 +164,6 @@ public function testGetDateTimeByIndexOfNonStringValueIsNull(mixed $value): void public function testGetDateTimeByIndexOfUnparsableStringIsNull(): void { - /** @var Column $col */ $col = new Column("expirationdate", ["not a date"]); $this->assertNull($col->getDateTimeByIndex(0)); } diff --git a/tests/InterfaceCoverageSeamTest.php b/tests/InterfaceCoverageSeamTest.php index 5d7fff80..d63069df 100644 --- a/tests/InterfaceCoverageSeamTest.php +++ b/tests/InterfaceCoverageSeamTest.php @@ -4,7 +4,6 @@ namespace CNICTEST; -use CNIC\CNR\Column as CNRColumn; use CNIC\CNR\Logger as CNRLogger; use CNIC\CNR\Response as CNRResponse; use CNIC\CNR\ResponseParser as CNRResponseParser; @@ -112,7 +111,7 @@ * find a subject at all. * {@see self::testTheSweepActuallyExaminesTheKnownImplementors()} closes it * by pinning that discovery still finds a fixed, independently-verified set - * of 13 real implementors (see {@see self::KNOWN_TOTAL_IMPLEMENTORS}) and + * of 12 real implementors (see {@see self::KNOWN_TOTAL_IMPLEMENTORS}) and * still walks a plausible number of files under `src/`. It asserts * **containment**, not equality — {@see self::KNOWN_TOTAL_IMPLEMENTORS} is a * floor, not a snapshot — so a newly added brand class is swept @@ -169,7 +168,6 @@ final class InterfaceCoverageSeamTest extends TestCase * @var class-string[] */ private const array KNOWN_TOTAL_IMPLEMENTORS = [ - CNRColumn::class, CNRLogger::class, CNRResponse::class, CNRResponseParser::class, diff --git a/tests/RecordColumnSeamTest.php b/tests/RecordColumnSeamTest.php index eb0c5f9e..da5aaf1e 100644 --- a/tests/RecordColumnSeamTest.php +++ b/tests/RecordColumnSeamTest.php @@ -4,7 +4,6 @@ namespace CNICTEST; -use CNIC\CNR\Column as CNRColumn; use CNIC\CNR\Response as CNRResponse; use CNIC\Column; use CNIC\ColumnInterface; @@ -17,22 +16,38 @@ use ReflectionNamedType; /** - * Locks the record/column seam collapsed in RSRMID-2923. + * Locks the record/column seam collapsed in RSRMID-2923 and, on top of it, the + * value-typing seam moved onto the interface in RSRMID-2942. * - * There is exactly one Record (CNIC\Record) and one Column (CNIC\Column). The - * brand `Record` classes were byte-identical empty subclasses and are gone; the - * brand `Column` classes duplicated ~35 lines of the same field, constructor and - * accessors, and only CNR\Column survives — solely to bind the value type to - * string and narrow getDataByIndex() to ?string. + * There is exactly one Record (CNIC\Record) and one Column (CNIC\Column); no + * brand may declare its own. CNR used to own a `CNR\Column extends Column` + * solely to narrow `getDataByIndex()` to `?string` via a generic template + * parameter — but `ColumnInterface` is not generic, so that narrowing was + * erased on every reachable path (getColumn()/getColumns(), declared on + * ResponseInterface) before a consumer holding the interface ever saw it. + * RSRMID-2942 replaced it with a native return type declared directly on + * ColumnInterface/RecordInterface: getStringByIndex()/getStringByKey(). The + * brand subclass is gone. * - * This is a structural test by necessity, not by preference: re-adding an empty - * `CNR\Record`/`IBS\Record`/`IBS\Column` marker, or re-inlining the shared - * column body into a brand, is behaviour-preserving on the day it lands, so no - * behavioural test can detect the erosion — only reflection can. The same - * reasoning as tests/ResponsePaginationSeamTest.php. + * This is a structural test by necessity, not by preference, for two distinct + * failure modes, both invisible to any behavioural test: * - * Deleting or weakening this test is a deliberate act: it re-opens the decision - * recorded in docs/agents/architecture.md. + * - A brand re-growing its own Record/Column marker (even an empty + * pass-through) is behaviour-preserving on the day it lands — the same + * reasoning as tests/ResponsePaginationSeamTest.php. + * - The narrowing sliding back into a `@template`/`@var Column` + * docblock generic is *also* behaviour-preserving: every existing runtime + * call still returns the same value, because Psalm/PHPStan generics are + * erased at runtime. Only reflecting the interface's declared return type + * can tell a native `?string` apart from a docblock-only one that a + * consumer holding `ColumnInterface`/`RecordInterface` can never see. + * + * Deleting or weakening this test is a deliberate act: it re-opens the + * decision recorded in docs/agents/architecture.md. Revisit it only if a + * brand genuinely needs record/column behaviour the shared classes cannot + * express (at which point that brand implements RecordInterface/ColumnInterface + * directly, per their class docblocks), or if the string-narrowing accessor + * itself is replaced by some other mechanism. */ final class RecordColumnSeamTest extends TestCase { @@ -57,6 +72,25 @@ public function testNoBrandDeclaresItsOwnRecord(): void } } + /** + * No brand may own a Column class either. CNR's used to exist solely to + * narrow getDataByIndex() to ?string via a generic template parameter that + * ColumnInterface (not generic) erased before any consumer saw it + * (RSRMID-2942) — the narrowing now lives as a native return type on + * ColumnInterface::getStringByIndex(), so the brand subclass has nothing + * left to exist for. + */ + public function testNoBrandDeclaresItsOwnColumn(): void + { + foreach (self::BRAND_NAMESPACES as $ns) { + $this->assertClassAbsent( + $ns . "\\Column", + "columns are shared as CNIC\\Column (RSRMID-2942); the value type is expressed via " + . "ColumnInterface::getStringByIndex(), not a per-brand subclass" + ); + } + } + /** * The shared Record must stay instantiable — the point of the collapse was * that no abstract base plus empty leaf is needed to build a row. @@ -93,22 +127,8 @@ public function testBothBrandsBuildTheSharedRecord(): void } /** - * IBS/Moniker carry arbitrary JSON values and need no narrowing, so they use - * the shared Column directly; a brand Column there would be a pass-through. - */ - public function testOnlyCnrDeclaresAColumn(): void - { - $this->assertClassAbsent("CNIC\\IBS\\Column", "IBS uses CNIC\\Column as-is (RSRMID-2923)"); - $this->assertClassAbsent( - "CNIC\\MONIKER\\Column", - "MONIKER reuses IBS's response layer and must not own a Column" - ); - $this->assertTrue(class_exists(CNRColumn::class)); - } - - /** - * The shared Column must stay instantiable (IBS builds it directly) and must - * own the whole key/data/length/bounds body. + * The shared Column must stay instantiable and must own the whole + * key/data/length/bounds body — both brands build it directly. */ public function testSharedColumnOwnsTheSharedBody(): void { @@ -124,25 +144,21 @@ public function testSharedColumnOwnsTheSharedBody(): void } /** - * CNR\Column earns its existence with exactly one thing: the narrowed - * return type. Anything else declared there is duplication creeping back. + * The value-typing seam (RSRMID-2942): ColumnInterface/RecordInterface must + * each declare a native `?string` return type on their string-narrowing + * accessor. This is the mechanism that replaced the erased generic — + * asserting it via reflection is the only way to tell a native return type + * (visible to every interface-typed consumer) apart from a docblock-only + * `@return TValue|null`/`@var Column` generic (invisible to them). */ - public function testCnrColumnNarrowsNothingButGetDataByIndex(): void + public function testStringNarrowingIsDeclaredNativelyOnTheInterfaces(): void { - $declared = array_map( - static fn(ReflectionMethod $m): string => $m->getName(), - array_filter( - (new ReflectionClass(CNRColumn::class))->getMethods(), - static fn(ReflectionMethod $m): bool => $m->getDeclaringClass()->getName() === CNRColumn::class - ) - ); - $this->assertSame( - ["getDataByIndex"], - array_values($declared), - "CNR\\Column exists only to narrow getDataByIndex() to ?string; everything else is shared" - ); + $type = (new ReflectionMethod(ColumnInterface::class, "getStringByIndex"))->getReturnType(); + $this->assertInstanceOf(ReflectionNamedType::class, $type); + $this->assertSame("string", $type->getName()); + $this->assertTrue($type->allowsNull()); - $type = (new ReflectionMethod(CNRColumn::class, "getDataByIndex"))->getReturnType(); + $type = (new ReflectionMethod(RecordInterface::class, "getStringByKey"))->getReturnType(); $this->assertInstanceOf(ReflectionNamedType::class, $type); $this->assertSame("string", $type->getName()); $this->assertTrue($type->allowsNull()); diff --git a/tests/RecordTest.php b/tests/RecordTest.php index 662871b0..39a47bfe 100644 --- a/tests/RecordTest.php +++ b/tests/RecordTest.php @@ -73,6 +73,26 @@ public function testGetDataByKeyOfNullValue(): void $this->assertArrayHasKey("EMPTY", $rec->getData()); } + // --- getStringByKey() --- + + public function testGetStringByKeyOfStringValue(): void + { + $rec = new Record(self::ROW); + $this->assertSame("mydomain.com", $rec->getStringByKey("DOMAIN")); + } + + public function testGetStringByKeyOfNonStringValueIsNull(): void + { + $rec = new Record(["contacts" => ["firstname" => "Middle", "lastname" => "Ware"]]); + $this->assertNull($rec->getStringByKey("contacts")); + } + + public function testGetStringByKeyOfMissingKeyIsNull(): void + { + $rec = new Record(self::ROW); + $this->assertNull($rec->getStringByKey("KEYNOTEXISTING")); + } + // --- getDateTimeByKey() --- public function testGetDateTimeByKeyParsesDashSeparatedValue(): void diff --git a/tests/ResponsePaginationSeamTest.php b/tests/ResponsePaginationSeamTest.php index 451afd46..d0d36818 100644 --- a/tests/ResponsePaginationSeamTest.php +++ b/tests/ResponsePaginationSeamTest.php @@ -12,30 +12,53 @@ use ReflectionMethod; /** - * Locks the pagination seam of AbstractResponse (RSRMID-2912, declined). + * Locks the pagination seam of AbstractResponse at the wire (RSRMID-2912 + * declined, RSRMID-2918 delivered, RSRMID-2943 narrowed). * - * The 7 pagination primitives are declared on ResponseInterface and left - * unimplemented on AbstractResponse on purpose, so a brand that forgets them - * fails at declaration time instead of silently reporting "one page, no next - * page" and losing pages 2..N of a list. The counterpart is that the 5 derived - * getters, which are pure functions of those primitives, DO live on the base. + * **Directive.** The seam is drawn at the wire: a brand Response declares + * exactly the four methods that read its own pagination columns + * (getFirstRecordIndex, getLastRecordIndex, getRecordsTotalCount, + * getRecordsLimitation) and nothing else. AbstractResponse owns every + * derivation from those four answers — including getCurrentPageNumber(), + * hasNextPage() and hasPreviousPage(), which read no column of their own and + * therefore do not belong in the brand-primitive set at all. * - * This is a structural test by necessity, not by preference: hoisting - * single-page defaults onto the base is behaviour-preserving (the defaults - * would return exactly what IBS\Response returns today), so no behavioural - * test can ever detect the erosion — only reflection can. + * **Failure mode prevented.** A brand that silently inherits column readers — + * whether through a hoisted base default or a shared trait — reports "one + * page, no next page" for a list that genuinely has more, and a consumer + * paging through it loses pages 2..N with no error anywhere. * - * Deleting or weakening this test is therefore a deliberate act, not a passing - * cleanup: reopen the decision first. It was taken on RSRMID-2912 (declined, - * now closed as Cancelled since the refactor was never implemented) and is - * recorded in full in docs/agents/architecture.md; RSRMID-2918 is the live - * issue that delivered this guard. The one condition that would justify - * revisiting it is a third, genuinely non-paginating brand arriving. + * **Why the guard must be structural.** Hoisting single-page defaults onto the + * base is behaviour-preserving on the day it lands: IBS already returns + * exactly those defaults today, so no behavioural test can distinguish "IBS + * answered this itself" from "IBS silently inherited a base default that + * happens to match its own answer". Only reflection, via getDeclaringClass(), + * can tell the two apart. + * + * **Revisit condition.** A brand whose "more results" signal is a cursor or + * opaque token rather than a record offset. For such a brand hasNextPage() + * genuinely becomes a wire read again (whatever the cursor field is called), + * and it belongs back in PRIMITIVES. + * + * **History.** RSRMID-2912 first proposed hoisting defaults and was declined, + * closed as Cancelled; RSRMID-2918 is the issue that actually delivered this + * guard, with 7 primitives (the four column readers plus + * getCurrentPageNumber/hasNextPage/hasPreviousPage) and 5 derived getters. + * RSRMID-2943 re-examined that split and found it had pinned a + * misclassification: getCurrentPageNumber()/hasNextPage()/hasPreviousPage() + * read no wire column — they are pure functions of the four column readers, + * exactly like getNextPageNumber()/getNumberOfPages() already were — so + * pinning them as "primitives" protected nothing while forcing brands to + * hand-roll arithmetic that could (and, for CNR, did) disagree with the + * equivalent page-number getters on an unaligned offset window. Narrowing to + * 4 primitives / 8 derived puts every predicate and page number on the same + * offset grid, so a predicate and its corresponding getter can no longer + * disagree. Full account in docs/agents/architecture.md. */ final class ResponsePaginationSeamTest extends TestCase { /** - * Pagination primitives that read brand-specific columns/status and must be + * Pagination primitives that read a brand's own wire columns and must be * answered explicitly by every brand. * * Spelled out on purpose rather than derived from ResponseInterface minus @@ -45,13 +68,10 @@ final class ResponsePaginationSeamTest extends TestCase * @var string[] */ private const array PRIMITIVES = [ - "getCurrentPageNumber", "getFirstRecordIndex", "getLastRecordIndex", "getRecordsTotalCount", "getRecordsLimitation", - "hasNextPage", - "hasPreviousPage", ]; /** @@ -59,15 +79,18 @@ final class ResponsePaginationSeamTest extends TestCase * @var string[] */ private const array DERIVED_GETTERS = [ + "getCurrentPageNumber", "getNextPageNumber", "getNumberOfPages", "getPagination", "getPreviousPageNumber", "getRecordsCount", + "hasNextPage", + "hasPreviousPage", ]; /** - * Every brand Response must declare all 7 primitives itself. MONIKER is not + * Every brand Response must declare all 4 primitives itself. MONIKER is not * listed because it reuses IBS\Response verbatim. */ public function testEachBrandDeclaresItsOwnPrimitives(): void From b4e0f6998c40a11de2fb5e0eea237e765a09371b Mon Sep 17 00:00:00 2001 From: Kai Schwarz Date: Mon, 10 Aug 2026 16:29:39 +0200 Subject: [PATCH 4/9] refactor(config): collapse the triplicated socket timeout default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/CNR/SocketConfig.php | 1 - src/IBS/SocketConfig.php | 1 - 2 files changed, 2 deletions(-) diff --git a/src/CNR/SocketConfig.php b/src/CNR/SocketConfig.php index 7b05d47b..5b3a7262 100644 --- a/src/CNR/SocketConfig.php +++ b/src/CNR/SocketConfig.php @@ -35,7 +35,6 @@ final class SocketConfig extends AbstractSocketConfig { protected string $oteUrl = "https://api-ote.rrpproxy.net/"; protected string $liveUrl = "https://api.rrpproxy.net/"; - protected int $socketTimeout = 300; /** * Separator between the account id and the role user id in a role login diff --git a/src/IBS/SocketConfig.php b/src/IBS/SocketConfig.php index a5faa434..9ca9e2a4 100644 --- a/src/IBS/SocketConfig.php +++ b/src/IBS/SocketConfig.php @@ -21,7 +21,6 @@ class SocketConfig extends AbstractSocketConfig { protected string $oteUrl = "https://testapi.internet.bs/"; protected string $liveUrl = "https://api.internet.bs/"; - protected int $socketTimeout = 300; /** * IBS carries sensitive data under lower-/camel-case command keys. Declared From 360142a020259211c0cb0168bf5469589d162fa7 Mon Sep 17 00:00:00 2001 From: Kai Schwarz Date: Mon, 10 Aug 2026 16:29:53 +0200 Subject: [PATCH 5/9] fix(config): name the owning class in the managed cURL option rejection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/AbstractSocketConfig.php | 39 ++++++++++++++++++++----- tests/AbstractClientCurlOptionsTest.php | 35 +++++++++++++++++++--- 2 files changed, 62 insertions(+), 12 deletions(-) diff --git a/src/AbstractSocketConfig.php b/src/AbstractSocketConfig.php index 5ed9c64b..66a69879 100644 --- a/src/AbstractSocketConfig.php +++ b/src/AbstractSocketConfig.php @@ -57,6 +57,12 @@ abstract class AbstractSocketConfig * while the wire carried the bag's value. Throw, naming constant and setter; * never silently pick a winner. * + * Each entry carries the **class** that owns the setter, and the message + * renders it, because the four are not all on one object: `setUserAgent()` + * lives on {@see AbstractClient} (the SDK's identity is versioned with that + * class), so a bare `setUserAgent()` would point a caller holding only the + * config at a method it cannot reach. + * * Rejection is **eager** here, unlike {@see HttpTransport::PROTECTED_OPTIONS} * which is checked on the next request: the config knows immediately, and the * error belongs where the mistake is. Options the SDK does *not* model @@ -67,13 +73,29 @@ abstract class AbstractSocketConfig * takes away legitimate tuning, narrowing it re-opens a second home. Pinned by * AbstractClientCurlOptionsTest::testManagedOptionsAreExactlyTheOnesWithTheirOwnSetter(). * - * @var array + * @var array */ public const array MANAGED_OPTIONS = [ - CURLOPT_TIMEOUT => ["option" => "CURLOPT_TIMEOUT", "setter" => "setSocketTimeout()"], - CURLOPT_USERAGENT => ["option" => "CURLOPT_USERAGENT", "setter" => "setUserAgent()"], - CURLOPT_PROXY => ["option" => "CURLOPT_PROXY", "setter" => "setProxy()"], - CURLOPT_REFERER => ["option" => "CURLOPT_REFERER", "setter" => "setReferer()"], + CURLOPT_TIMEOUT => [ + "option" => "CURLOPT_TIMEOUT", + "owner" => self::class, + "setter" => "setSocketTimeout()", + ], + CURLOPT_USERAGENT => [ + "option" => "CURLOPT_USERAGENT", + "owner" => AbstractClient::class, + "setter" => "setUserAgent()", + ], + CURLOPT_PROXY => [ + "option" => "CURLOPT_PROXY", + "owner" => self::class, + "setter" => "setProxy()", + ], + CURLOPT_REFERER => [ + "option" => "CURLOPT_REFERER", + "owner" => self::class, + "setter" => "setReferer()", + ], ]; /** @@ -488,14 +510,15 @@ private static function rejectManagedOptions(array $opts): void return; } $named = array_map( - static fn(array $entry): string => $entry["option"] . " (use " . $entry["setter"] . ")", + static fn(array $entry): string => $entry["option"] + . " (use " . $entry["owner"] . "::" . $entry["setter"] . ")", array_values($rejected) ); throw new UnsupportedFeatureException( "cURL option(s) the SDK models as configuration cannot be set through the option bag: " . implode(", ", $named) - . ". Setting one both ways would leave the getter and the wire disagreeing; use the setter," - . " which is the single home for that value." + . ". Setting one both ways would leave the getter and the wire disagreeing; use the setter" + . " named above, which is the single home for that value." ); } diff --git a/tests/AbstractClientCurlOptionsTest.php b/tests/AbstractClientCurlOptionsTest.php index 6ccdb864..0d08d5a2 100644 --- a/tests/AbstractClientCurlOptionsTest.php +++ b/tests/AbstractClientCurlOptionsTest.php @@ -196,7 +196,14 @@ public function testManagedOptionsAreExactlyTheOnesWithTheirOwnSetter(): void /** * The constant doubles as the lookup used to build the rejection message, so * every entry must carry its own constant's name — a mismatch would point the - * caller at the wrong option — and a setter that actually exists. + * caller at the wrong option — and a setter that actually exists **on the class + * the entry names as its owner**. + * + * The owner is checked rather than "either the config or the client" (RSRMID-2944): + * the four setters do not all live on one object, and a message naming a bare + * `setUserAgent()` sent a caller holding only the config to a method it cannot + * reach. Accepting a match on either class would let that recur — and would let + * a setter silently move class without the message following it. */ public function testManagedOptionsNameTheirConstantAndAnExistingSetter(): void { @@ -206,9 +213,29 @@ public function testManagedOptionsNameTheirConstantAndAnExistingSetter(): void $setter = rtrim($entry["setter"], "()"); $this->assertTrue( - method_exists(AbstractSocketConfig::class, $setter) || method_exists(AbstractClient::class, $setter), - "{$entry['setter']} is named as the owner of {$entry['option']} but exists on neither the " - . "config nor the client" + method_exists($entry["owner"], $setter), + "{$entry['owner']}::{$entry['setter']} is named as the owner of {$entry['option']} " + . "but that class has no such method" + ); + } + } + + /** + * The owner is only useful to a caller if the thrown message actually carries + * it — the point of RSRMID-2944 was a reachable method name, not a more + * detailed constant. + */ + public function testRejectionMessageNamesTheClassOwningTheSetter(): void + { + $cl = $this->cnr(); + try { + $cl->setExtraCurlOptions([CURLOPT_USERAGENT => "ua"]); + $this->fail("expected UnsupportedFeatureException"); + } catch (UnsupportedFeatureException $e) { + $this->assertStringContainsString( + AbstractClient::class . "::setUserAgent()", + $e->getMessage(), + "the user agent lives on the client, and the message must say so" ); } } From 4020bbb19e608e6706e3d053a84e96db33ddf567 Mon Sep 17 00:00:00 2001 From: Kai Schwarz Date: Mon, 10 Aug 2026 16:30:02 +0200 Subject: [PATCH 6/9] docs(architecture): correct the config forwarder count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/agents/architecture.md | 4 ++-- src/AbstractClient.php | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/agents/architecture.md b/docs/agents/architecture.md index abdf6f18..5c9a6ba0 100644 --- a/docs/agents/architecture.md +++ b/docs/agents/architecture.md @@ -101,12 +101,12 @@ Full architectural reference for the PHP SDK. - **`MONIKER\Client` stays non-final on purpose.** Psalm's `ClassMustBeFinal` fired once nothing extended it, but that rule only ever fires on leaf classes — `CNR\Client`/`IBS\Client` escape it purely because something inside the repo extends them, so sealing one brand on that accident would make consumer extensibility differ per brand for an inexplicable reason. Suppressed at the class with that rationale. - **`CNR\Client::getSocketConfig()` is the single narrowing point** from `AbstractSocketConfig` to `CNR\SocketConfig`, because PHP typed properties are invariant — a subclass cannot re-declare `$socketConfig` narrower, so the covariant `newSocketConfig()` factory cannot inform the property's type. It uses an `instanceof`-guard-with-throw, never `assert()`. There is deliberately **no alias**: two methods narrowing the same property would be two places to keep in step. - **Guard:** `tests/ClientSessionSeamTest.php`. Absence cannot be tested behaviourally — calling a method that is gone is a fatal `Error` — which is exactly why the stubs survived from v11 to v21. -- **Connection configuration has exactly one home, and two invariants keep it that way (RSRMID-2921, breaking, v23.0.0).** State was split between `AbstractClient` and `AbstractSocketConfig` with no invariant tying the copies together. The root cause was structural: **there was no accessor for `$socketConfig` on the client**, so every value needed a hand-written forwarder or was unreachable — which is why ~26 accumulated and why `AbstractClient` was the repo's highest-churn file (43 touches in 120 commits). +- **Connection configuration has exactly one home, and two invariants keep it that way (RSRMID-2921, breaking, v23.0.0).** State was split between `AbstractClient` and `AbstractSocketConfig` with no invariant tying the copies together. The root cause was structural: **there was no accessor for `$socketConfig` on the client**, so every value needed a hand-written forwarder or was unreachable — which is why 18 accumulated and why `AbstractClient` was the repo's highest-churn file (43 touches in 120 commits). - **The division of labour.** The config owns *the connection*: where to connect (`$url`, `$oteUrl`/`$liveUrl`, `$highPerformance`), how to authenticate, and how the transport behaves (`$socketTimeout`, `$proxy`, `$referer`, `$curlOptions`). The client owns *client behaviour*: logger and `$debugMode`, `$context`, the transport instance, and the SDK's identity (`VERSION`/`$userAgent` — kept on the client because semantic-release rewrites `VERSION` in that file). - **Invariant 1 — the system is derived, never stored.** `getSystem(): ?System` compares `$url` to the two endpoints. The nullable return is the point: after `setURL("https://staging.example/")` there is no honest OT&E-or-LIVE answer, and reporting the last selection is exactly how the `useOTESystem()->setURL($custom)` drift happened. Taken in the direction that keeps `setURL()` authoritative. - **Invariant 2 — a value the SDK models cannot also come from the cURL bag.** `AbstractSocketConfig::MANAGED_OPTIONS` (`CURLOPT_TIMEOUT`/`USERAGENT`/`PROXY`/`REFERER`) is rejected by `setExtraCurlOptions()` with an exception naming the owning setter — each already has one, and a bag value silently outranked it while the getter reported the setter's. Rejection is **eager** here (the config knows immediately) whereas `PROTECTED_OPTIONS` is checked on the next request (the transport is injectable, and which options it owns is its own business). Options the SDK has no opinion about — `CONNECTTIMEOUT`, `IPRESOLVE`, `HTTPHEADER` — stay caller-owned. **Guard:** `AbstractClientCurlOptionsTest::testManagedOptionsAreExactlyTheOnesWithTheirOwnSetter()`. - **High-performance routing is a flag applied inside `getURL()`, not a URL rewrite.** The eager rewrite was itself a drift: the loopback URL no longer matched the OT&E endpoint, so switching the mode on silently cost the caller `isOTE()`. Routing is *how* to reach the endpoint, not *which* one, so the selected system now survives it and it survives a later system switch. - - **The ~26 forwarders were deliberately kept**, against the ticket's "collapse them" option: they are the documented consumer surface, and deleting them would rewrite every integration to fix nothing — a forwarder is one home with one answer, and the invariant is "no value readable from two places **with two possible answers**". What `getSocketConfig()` changes is that a *new* setting no longer needs one — so do not add one; `getSocketConfig()->getOTEUrl()` is the answer. **Rejected** in the opposite direction: absorbing the config into the client would cost the "configure without constructing a client" property that motivated the ticket. + - **The 18 forwarders were deliberately kept**, against the ticket's "collapse them" option: they are the documented consumer surface, and deleting them would rewrite every integration to fix nothing — a forwarder is one home with one answer, and the invariant is "no value readable from two places **with two possible answers**". What `getSocketConfig()` changes is that a *new* setting no longer needs one — so do not add one; `getSocketConfig()->getOTEUrl()` is the answer. **Rejected** in the opposite direction: absorbing the config into the client would cost the "configure without constructing a client" property that motivated the ticket. - **`getCurlOptions()` puts dedicated proxy/referer state on the *left* of the union, and that is load-bearing.** `setExtraCurlOptions()`'s guard is not the only writer of `$curlOptions` — `getDefaultCurlOpts()` seeds it and `resetCurlOptions()` re-seeds it, neither passing the guard. A brand default of `CURLOPT_PROXY` — exactly the protocol-mandatory case that hook is kept for — would bring the drift back **inverted**: the getter reporting the setter's value while the request used the default. `executeCurl()`'s third `$extraCurlOpts` parameter was removed for the same reason: an option route skipping `MANAGED_OPTIONS` is a way to put a second answer behind `getProxy()`. **Guard:** `AbstractClientConfigDriftTest::testDedicatedProxyStateBeatsABrandDefaultOnTheWire()`, which writes out the brand subclass that does not yet exist. - **Deliberately not addressed:** with debug mode on, `performRequest()` calls `getPOSTData()` twice to build the masked log line. It is not a second *home* — both calls read the same config and cannot disagree — and a masked variant genuinely needs a second pass; the alternatives (a `getPOSTDataPair()`-shaped API, or masking the encoded string) are not worth a debug-only saving. - **Guards:** `tests/ClientConfigSeamTest.php`, `tests/AbstractClientConfigDriftTest.php`. diff --git a/src/AbstractClient.php b/src/AbstractClient.php index 689f6ccf..38f18412 100644 --- a/src/AbstractClient.php +++ b/src/AbstractClient.php @@ -36,7 +36,7 @@ * the documented ergonomic surface (`$cl->useOTESystem()->setCredentials(...)`) * and they read and write the config's state rather than a copy of it, so a * forwarder cannot disagree with the config. A *new* setting needs no forwarder — - * `getSocketConfig()` is the accessor whose absence let ~26 of these accumulate. + * `getSocketConfig()` is the accessor whose absence let these 18 accumulate. * * Only capabilities every brand can actually honour live here. In particular * `getSession()`/`setSession()` do **not** — API sessions are a CNR concept and From 00ede5d1ec122e2d65fb04b4e506520773b25cab Mon Sep 17 00:00:00 2001 From: Kai Schwarz Date: Mon, 10 Aug 2026 16:41:13 +0200 Subject: [PATCH 7/9] docs(architecture): retire the CNR\Column claim and record the option-owner decision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CLAUDE.md still described `CNR\Column extends CNIC\Column` 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. --- CLAUDE.md | 4 ++-- docs/agents/architecture.md | 3 ++- tests/ClientConfigSeamTest.php | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 7a670661..522ec625 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -12,7 +12,7 @@ Facts below; the class inventory is derivable from `src/` and the **full deep di - **Namespace root:** `CNIC\` mapped to `src/` (PSR-4). Brand sub-namespaces: `CNR`, `IBS`, `MONIKER`. - **Shared abstracts (in `CNIC\`):** `AbstractClient`, `AbstractSocketConfig`, `HttpTransport`, `AbstractResponseTemplateManager`, `AbstractResponseTranslator`, `AbstractResponse`. Shared **concretes:** `Record`, `Column` (templated on its value type), `ApiDateTime`. Enum: `System` (`OTE`/`LIVE`) — derived from the configured URL, never stored. -- **Brands are siblings, not parent/child:** `CNR\Response`/`IBS\Response` both extend `AbstractResponse`. `MONIKER\Client extends IBS\Client` (same platform; only `SocketConfig` differs) and reuses IBS's Response. There is no brand `Record`, and `CNR\Column extends CNIC\Column` is the only brand `Column`. +- **Brands are siblings, not parent/child:** `CNR\Response`/`IBS\Response` both extend `AbstractResponse`. `MONIKER\Client extends IBS\Client` (same platform; only `SocketConfig` differs) and reuses IBS's Response. No brand declares a `Record` or a `Column` — both are shared concretes, and a value-type narrowing goes in a native return type on the interface (`getStringByIndex()`/`getStringByKey()`), never in a generic. - **Response construction is a template method:** brands implement the `translate()`/`populate()`/`newRecord()`/`newResponseParser()` hooks — never reimplement `AbstractResponse::__construct()`. - **The `request()` lifecycle is a template method too** (`AbstractClient::performRequest()`), and public `request(array $cmd = [], string $path = "")` is symmetric across brands. Vary a brand only through `buildCommand()`/`newResponse()`/`newSocketConfig()`. - **Config-driven:** each `SocketConfig` (extends `AbstractSocketConfig`) carries endpoints/params/flags as typed properties (no `config.json`). @@ -27,7 +27,7 @@ Facts below; the class inventory is derivable from `src/` and the **full deep di Three directives have no guard test and therefore live here: -- Do **not** "symmetrise" columns onto a `newColumn()` factory like records — infeasible under PHPStan L9 / Psalm L1; keep the `registerColumn(ColumnInterface)` shape. (RSRMID-2899) +- Do **not** "symmetrise" columns onto a `newColumn()` factory like records — keep the `registerColumn(ColumnInterface)` shape. The original "infeasible under PHPStan L9 / Psalm L1" argument died with `CNR\Column`; re-run the analysers before reopening, and do not quote it. (RSRMID-2899, RSRMID-2942) - Do **not** rewrite date columns in the response data. Do **not** grow `CNIC\ApiDateTime` beyond an opt-in UTC-only **parser** — accepting both `-` and `/` separators is in scope, but no `in($tz)`, no locale formatting, no `ext-intl`, no markup helpers. (RSRMID-2318; RSRMID-2926) - Do **not** make `dateTime` fall back to `date` for date-only values — `ts` and `dateTime` are null _together_. (RSRMID-2318) diff --git a/docs/agents/architecture.md b/docs/agents/architecture.md index 5c9a6ba0..01b7035f 100644 --- a/docs/agents/architecture.md +++ b/docs/agents/architecture.md @@ -104,7 +104,8 @@ Full architectural reference for the PHP SDK. - **Connection configuration has exactly one home, and two invariants keep it that way (RSRMID-2921, breaking, v23.0.0).** State was split between `AbstractClient` and `AbstractSocketConfig` with no invariant tying the copies together. The root cause was structural: **there was no accessor for `$socketConfig` on the client**, so every value needed a hand-written forwarder or was unreachable — which is why 18 accumulated and why `AbstractClient` was the repo's highest-churn file (43 touches in 120 commits). - **The division of labour.** The config owns *the connection*: where to connect (`$url`, `$oteUrl`/`$liveUrl`, `$highPerformance`), how to authenticate, and how the transport behaves (`$socketTimeout`, `$proxy`, `$referer`, `$curlOptions`). The client owns *client behaviour*: logger and `$debugMode`, `$context`, the transport instance, and the SDK's identity (`VERSION`/`$userAgent` — kept on the client because semantic-release rewrites `VERSION` in that file). - **Invariant 1 — the system is derived, never stored.** `getSystem(): ?System` compares `$url` to the two endpoints. The nullable return is the point: after `setURL("https://staging.example/")` there is no honest OT&E-or-LIVE answer, and reporting the last selection is exactly how the `useOTESystem()->setURL($custom)` drift happened. Taken in the direction that keeps `setURL()` authoritative. - - **Invariant 2 — a value the SDK models cannot also come from the cURL bag.** `AbstractSocketConfig::MANAGED_OPTIONS` (`CURLOPT_TIMEOUT`/`USERAGENT`/`PROXY`/`REFERER`) is rejected by `setExtraCurlOptions()` with an exception naming the owning setter — each already has one, and a bag value silently outranked it while the getter reported the setter's. Rejection is **eager** here (the config knows immediately) whereas `PROTECTED_OPTIONS` is checked on the next request (the transport is injectable, and which options it owns is its own business). Options the SDK has no opinion about — `CONNECTTIMEOUT`, `IPRESOLVE`, `HTTPHEADER` — stay caller-owned. **Guard:** `AbstractClientCurlOptionsTest::testManagedOptionsAreExactlyTheOnesWithTheirOwnSetter()`. + - **Invariant 2 — a value the SDK models cannot also come from the cURL bag.** `AbstractSocketConfig::MANAGED_OPTIONS` (`CURLOPT_TIMEOUT`/`USERAGENT`/`PROXY`/`REFERER`) is rejected by `setExtraCurlOptions()` with an exception naming the owning **class and** setter — each already has one, and a bag value silently outranked it while the getter reported the setter's. Rejection is **eager** here (the config knows immediately) whereas `PROTECTED_OPTIONS` is checked on the next request (the transport is injectable, and which options it owns is its own business). Options the SDK has no opinion about — `CONNECTTIMEOUT`, `IPRESOLVE`, `HTTPHEADER` — stay caller-owned. **Guard:** `AbstractClientCurlOptionsTest::testManagedOptionsAreExactlyTheOnesWithTheirOwnSetter()`. + - **Each entry carries the class that owns the setter, and the message renders it (RSRMID-2944, non-breaking, v32.0.0).** The four setters are not all on one object — `setUserAgent()` lives on `AbstractClient`, because the SDK's identity is versioned with that class — so the original bare `"setUserAgent()"` pointed a caller holding only the config at a method it cannot reach. **Do not "simplify" the `owner` key away, and do not drop `CURLOPT_USERAGENT` from the list instead**: dropping it re-opens the second home the constant exists to close. The guard had accepted the setter existing on *either* class (`method_exists(config) || method_exists(client)`), which is precisely what let the unreachable name pass review; it now checks against the class the entry names, plus a behavioural test that the thrown message carries it. - **High-performance routing is a flag applied inside `getURL()`, not a URL rewrite.** The eager rewrite was itself a drift: the loopback URL no longer matched the OT&E endpoint, so switching the mode on silently cost the caller `isOTE()`. Routing is *how* to reach the endpoint, not *which* one, so the selected system now survives it and it survives a later system switch. - **The 18 forwarders were deliberately kept**, against the ticket's "collapse them" option: they are the documented consumer surface, and deleting them would rewrite every integration to fix nothing — a forwarder is one home with one answer, and the invariant is "no value readable from two places **with two possible answers**". What `getSocketConfig()` changes is that a *new* setting no longer needs one — so do not add one; `getSocketConfig()->getOTEUrl()` is the answer. **Rejected** in the opposite direction: absorbing the config into the client would cost the "configure without constructing a client" property that motivated the ticket. - **`getCurlOptions()` puts dedicated proxy/referer state on the *left* of the union, and that is load-bearing.** `setExtraCurlOptions()`'s guard is not the only writer of `$curlOptions` — `getDefaultCurlOpts()` seeds it and `resetCurlOptions()` re-seeds it, neither passing the guard. A brand default of `CURLOPT_PROXY` — exactly the protocol-mandatory case that hook is kept for — would bring the drift back **inverted**: the getter reporting the setter's value while the request used the default. `executeCurl()`'s third `$extraCurlOpts` parameter was removed for the same reason: an option route skipping `MANAGED_OPTIONS` is a way to put a second answer behind `getProxy()`. **Guard:** `AbstractClientConfigDriftTest::testDedicatedProxyStateBeatsABrandDefaultOnTheWire()`, which writes out the brand subclass that does not yet exist. diff --git a/tests/ClientConfigSeamTest.php b/tests/ClientConfigSeamTest.php index e2347035..c35b7f00 100644 --- a/tests/ClientConfigSeamTest.php +++ b/tests/ClientConfigSeamTest.php @@ -223,7 +223,7 @@ public function testWritesThroughEitherRouteAgree(\Closure $factory): void /** * The accessor is the point of the change: without it every configuration - * value needed a hand-written forwarder or was unreachable, which is why ~26 + * value needed a hand-written forwarder or was unreachable, which is why 18 * of them accumulated and why this was the repo's highest-churn file. * * @param \Closure(): AbstractClient $factory From c19145bd28a812544572fa43f02aad2fee31b2de Mon Sep 17 00:00:00 2001 From: Kai Schwarz Date: Mon, 10 Aug 2026 16:43:33 +0200 Subject: [PATCH 8/9] docs(architecture): pin the total-interface count to the guard, not the prose --- docs/agents/architecture.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/agents/architecture.md b/docs/agents/architecture.md index 01b7035f..f464c55d 100644 --- a/docs/agents/architecture.md +++ b/docs/agents/architecture.md @@ -71,7 +71,7 @@ Full architectural reference for the PHP SDK. - **`IBS\Response::getStatus()` was removed rather than promoted to a capability interface (RSRMID-2927, breaking, v28.0.0).** It returned the raw `status` hash key with zero callers in `src/` — every internal consumer already reads that key through protected `getHashString()`, and external consumers can reach it through the universal interface via `getHash()["status"]`. - **The judgment worth reusing:** a single method that only duplicates an already-interface-reachable hash lookup does not justify a new one-method capability interface plus the `instanceof` ceremony — unlike the CNR-only 5-method cluster that did justify `ExtendedResponseInterface`. This was a plain unused-API removal, not a repeat of the absence-vs-throw-vs-no-op policy question. - **Deliberately not guarded specifically.** Nothing pins "this method must stay gone"; a re-added public method not on any interface is caught by the sweep below, which is stronger than a one-off regression test naming `getStatus()`. Consumer path: [MIGRATION.md → v28.0.0](../../MIGRATION.md#-v2800), including the one case (`Domain/Check`'s `AVAILABLE`/`UNAVAILABLE`) where reading that key is the only way to get the answer — `isError()`/`isSuccess()` report only whether the *command* succeeded. -- **A sweep guards that every public method is reachable through the interfaces CLAUDE.md mandates typing against (RSRMID-2927).** `tests/InterfaceCoverageSeamTest.php` derives each concrete class's FQCN from its PSR-4 path and, for those implementing a "total" interface (the 7 meant to fully describe their implementors — `ResponseInterface`, `RecordInterface`, `ColumnInterface`, `TransportInterface`, `ResponseParserInterface`, `LoggerInterface`, `LogSinkInterface`), checks every public method for two defect shapes: **stray** (on none of them — the `getStatus()` shape) and **widening** (more parameters than the interface declares — the `getColumnKeys(bool)` defect fixed in `4b3ff7b`). +- **A sweep guards that every public method is reachable through the interfaces CLAUDE.md mandates typing against (RSRMID-2927).** `tests/InterfaceCoverageSeamTest.php` derives each concrete class's FQCN from its PSR-4 path and, for those implementing a "total" interface (the ones meant to fully describe their implementors — `ResponseInterface`, `RecordInterface`, `ColumnInterface`, `TransportInterface`, `ResponseParserInterface`, `LoggerInterface`, `LogSinkInterface`; **RSRMID-2941 added an eighth, `ResponseTemplateManagerInterface` — see below, and read `TOTAL_INTERFACES` rather than counting this sentence**), checks every public method for two defect shapes: **stray** (on none of them — the `getStatus()` shape) and **widening** (more parameters than the interface declares — the `getColumnKeys(bool)` defect fixed in `4b3ff7b`). - **`ExtendedResponseInterface`/`RoleCredentialsInterface` are excluded from the "total" role** because they are additive capability interfaces, not descriptions of everything their implementor exposes; treating either as total would flag `CNR\Client`/`CNR\SessionClient`'s ~20 legitimate methods as stray. They still count as declaring interfaces, so `CNR\Response`'s 5 extended methods resolve rather than being reported themselves. - **The allow-list is empty and stays that way.** A future stray or widened method is a defect to fix at the source, not a reason to grow an exception list. - **`tests/ResponseInterfaceConsumerTest.php` was deliberately kept, not folded in.** The sweep is a *mechanism* check; the consumer test additionally pins *behaviour* through the interface (`getColumnKeys(true)` genuinely strips pagination keys) and the no-constructor rule. Replacing one with the other drops a behavioural pin. From d1a26ab7a284ae363c9c8729a51aeac6c4a55987 Mon Sep 17 00:00:00 2001 From: Kai Schwarz Date: Mon, 10 Aug 2026 16:52:20 +0200 Subject: [PATCH 9/9] test(pagination): cover the two null gates RSRMID-2943's ?int widening added MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- tests/CNR/ClientTest.php | 35 +++++++++++++++++++++++++++++++++++ tests/CNR/ResponseTest.php | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/tests/CNR/ClientTest.php b/tests/CNR/ClientTest.php index 68701f93..7d013d06 100644 --- a/tests/CNR/ClientTest.php +++ b/tests/CNR/ClientTest.php @@ -827,6 +827,41 @@ public function testRequestNextResponsePagePastTheEnd(): void $this->assertNull(self::$cl->requestNextResponsePage($r)); } + public function testAdvanceIsRefusedWhenHasNextPageDisagreesWithTheOffsets(): void + { + // Unreachable through any real response, and deliberately so. Since + // RSRMID-2943 the predicate and the offsets are derived from the same + // four readers, so hasNextPage() === true already implies a positive + // LIMIT and a non-null LAST — which is exactly why the re-check below + // it reads like dead code and would survive a "simplification" review. + // + // It is not dead: it is what stops the derivation being re-split. The + // subclass here is the response that does not exist yet — a brand, or a + // consumer subclass, whose hasNextPage() answers from something other + // than the offsets and so can say "yes" over a window with no LIMIT to + // advance by. Without the guard the very next lines compute FIRST from + // null and re-request the list from the top, which is the forever-loop + // RSRMID-2943 set out to make impossible. Drop the guard and this test + // fails; drop hasNextPage()'s own null checks and it still passes, + // because the two protect different halves of the same invariant. + $tpls = (new RTM())->addTemplate( + "listWithoutLimit", + "[RESPONSE]\r\nPROPERTY[COLUMN][0]=domain\r\nPROPERTY[COUNT][0]=2\r\nPROPERTY[FIRST][0]=0\r\n" + . "PROPERTY[LAST][0]=1\r\nPROPERTY[TOTAL][0]=1825824\r\n" + . "DESCRIPTION=Command completed successfully\r\nCODE=200\r\nQUEUETIME=0\r\nRUNTIME=0.377\r\nEOF\r\n" + ); + $r = new class ("listWithoutLimit", ["COMMAND" => "QueryDomainList"], templates: $tpls) extends R { + #[\Override] + public function hasNextPage(): bool + { + return true; + } + }; + $this->assertTrue($r->hasNextPage()); + $this->assertNull($r->getRecordsLimitation()); + $this->assertNull(self::$cl->requestNextResponsePage($r)); + } + public function testRequestAllResponsePagesOk(): void { self::$tape->useCassette("all-pages"); diff --git a/tests/CNR/ResponseTest.php b/tests/CNR/ResponseTest.php index 3ccb19f9..50e5a1eb 100644 --- a/tests/CNR/ResponseTest.php +++ b/tests/CNR/ResponseTest.php @@ -517,6 +517,40 @@ public function testWindowEndingBeforeItStartsHasNoNextPage(): void $this->assertNull($r->getNextPageNumber()); } + public function testAWindowWithNoTotalHasNoNextPage(): void + { + // Synthetic, like the gate above, but read the scope of this one + // carefully before treating it as a behavioural guard, because it is + // not one and a later reader should not have to re-derive that. + // + // hasNextPage() null-checks FIRST, LAST and TOTAL together. On CNR only + // TOTAL can actually be null by the time that line runs: reaching it + // needs a positive LIMIT, a LIMIT column means getRecordsCount() >= 1, + // and both getFirstRecordIndex() and getLastRecordIndex() fall back to + // a record-derived value rather than null once there is a record. The + // other two clauses are there for the analysers — `$last < $first` and + // `$last + 1 < $total` need int, not ?int — and for a future brand + // whose readers have no such fallback. + // + // The TOTAL clause is inert as well: PHP coerces the null to 0, so + // `2 < null` is false and the answer would be the same without it. So + // what this test pins is the RSRMID-2943 ?int contract — an absent + // pagination column reads as null, not as 0 — and that hasNextPage() + // answers false for a response that cannot say how long the list is. + // Do not "prove" it by deleting a clause and expecting red. + $tpls = (new RTM())->addTemplate( + "listWithoutTotal", + "[RESPONSE]\r\nPROPERTY[COLUMN][0]=domain\r\nPROPERTY[COUNT][0]=2\r\nPROPERTY[FIRST][0]=0\r\n" + . "PROPERTY[LAST][0]=1\r\nPROPERTY[LIMIT][0]=100\r\n" + . "DESCRIPTION=Command completed successfully\r\nCODE=200\r\nQUEUETIME=0\r\nRUNTIME=0.377\r\nEOF\r\n" + ); + $r = new R("listWithoutTotal", templates: $tpls); + $this->assertSame(100, $r->getRecordsLimitation()); + $this->assertNull($r->getRecordsTotalCount()); + $this->assertFalse($r->hasNextPage()); + $this->assertNull($r->getNextPageNumber()); + } + public function testIteratingAResponseWithoutRecordsYieldsNothing(): void { $r = new R("OK", templates: self::$tpls);