From b3ca1c0ba187aff9dcf4c7d09bcf2f98f943ab38 Mon Sep 17 00:00:00 2001 From: blaipr Date: Thu, 13 Aug 2026 20:13:39 +0200 Subject: [PATCH] Make the two session accessors return what they hold MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getAccountColor() was declared to return a string and returned whatever setAccountColor(array) had stored — always an array — so the first read after a legitimate write would have raised a TypeError on the way out. The interface carried the same mismatch. Nothing in the application calls it, which is the only reason it went unnoticed. getTheme() had the same shape of problem from the other side: no default, so a session that had not been given a theme returned null from a method declared to return a string. Both are now what they always held: an array of colours keyed by account id, and a string that is empty before a theme is chosen. Doc: the coverage figure is now 96.6% (21648/22402), and the pcov note gains the thing that made a file look like a gap when it is at 100% — the two suites disagree about which lines are statements in nine files, so a merged report overstates them. Confirm per-file before sending anyone to fix one. --- CLAUDE.md | 14 ++++--- src/Domain/Core/Context/SessionContext.php | 6 +-- src/Infrastructure/Context/Session.php | 15 +++++-- .../Infrastructure/Context/SessionTest.php | 39 +++++++++++++++++++ 4 files changed, 62 insertions(+), 12 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 44879a56a..80e8361b5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -127,15 +127,19 @@ gotchas (the image provides these): ### Coverage: what is covered, and what deliberately is not -Line coverage is **95.4%** (`21368/22399`). Measure it by installing pcov in the container +Line coverage is **96.6%** (`21648/22402`). Measure it by installing pcov in the container (`pecl install pcov && docker-php-ext-enable pcov` — the enable step is separate, and a previous session's `pecl install` leaves it *installed but disabled*, so a `pecl list | grep pcov` guard skips it and every file reports zero), running both suites with `--coverage-clover`, and removing the ini again — the image does not ship a coverage driver, and leaving one enabled slows every later run. When merging two clover files, read `//file` by xpath: clover nests files inside -`` elements, so `$xml->project->file` only ever finds the namespace-less ones. +`` elements, so `$xml->project->file` only ever finds the namespace-less ones. The two +runs also disagree about which lines *are* statements in a handful of files (9 of 995, 31 +statements — `Application/Account/Services/AccountSearch.php` worst at 48 vs 61), so a merge that +treats "absent from one report" as uncovered overstates those files. Confirm a per-file gap with a +focused `--coverage-clover` run before sending anyone to fix it: that file is at 100%. -The remaining ~1000 statements are not a backlog to burn down uniformly. What is left: +The remaining ~750 statements are not a backlog to burn down uniformly. What is left: - **Bootstrap (~90 statements).** `Base.php`, `Definitions/CoreDefinitions.php`, `Adapter/In/Cli/Init.php`, `Bootstrap/BootstrapBase.php`. These run *before* the container the tests build — `Base.php` is @@ -189,8 +193,8 @@ A few harness details bite when writing an integration test against a real branc drawing a group or profile id that way fails about one run in a hundred, on CI, in whichever pull request happened to be open. Use `numberBetween(1, …)`. -The rest is genuinely reachable, and is a long tail rather than a few large files: ~1000 -statements across **295** files, averaging under four statements each — individual error branches, +The rest is genuinely reachable, and is a long tail rather than a few large files: ~660 +statements across **270** files, averaging under three statements each — individual error branches, rarely-hit conditionals and unused accessors. Worth picking off when touching the surrounding code; not worth a campaign. diff --git a/src/Domain/Core/Context/SessionContext.php b/src/Domain/Core/Context/SessionContext.php index 9c7895c9a..d694c0a64 100644 --- a/src/Domain/Core/Context/SessionContext.php +++ b/src/Domain/Core/Context/SessionContext.php @@ -127,11 +127,11 @@ public function setLastActivity(int $time): void; public function getStartActivity(): int; /** - * Return the color associated with an account + * Return the colors associated with the accounts, keyed by the id they belong to * - * @return string + * @return array */ - public function getAccountColor(): string; + public function getAccountColor(): array; /** * Set the color associated with an account diff --git a/src/Infrastructure/Context/Session.php b/src/Infrastructure/Context/Session.php index 3dc95fb35..ee2a84c23 100644 --- a/src/Infrastructure/Context/Session.php +++ b/src/Infrastructure/Context/Session.php @@ -72,7 +72,9 @@ public static function close(): void */ public function getTheme(): string { - return $this->getContextKey('theme'); + // Defaulted, because a session that has not been given a theme yet would otherwise return + // null from a method declared to return a string, and raise on the way out. + return $this->getContextKey('theme', ''); } /** @@ -298,11 +300,16 @@ public function getLocale(): ?string } /** - * Returns the color associated with an account + * Returns the colors associated with the accounts, keyed by the id they belong to. + * + * An array, because that is what setAccountColor() stores: declared as a string, this raised + * on its way out the first time anybody read it back. + * + * @return array */ - public function getAccountColor(): string + public function getAccountColor(): array { - return $this->getContextKey('accountcolor'); + return $this->getContextKey('accountcolor', []); } /** diff --git a/tests/Unit/Infrastructure/Context/SessionTest.php b/tests/Unit/Infrastructure/Context/SessionTest.php index 8382a8845..3d877b61f 100644 --- a/tests/Unit/Infrastructure/Context/SessionTest.php +++ b/tests/Unit/Infrastructure/Context/SessionTest.php @@ -482,6 +482,45 @@ public function theConfigLoadTimeIsCarried() self::assertSame(1700000000, $session->getConfigTime()); } + /** + * The account colours go in as an array and come back as one. Declared as a string, this + * raised a TypeError on the way out the first time anybody read back what had been stored — + * nothing in the application did, which is the only reason it went unnoticed. + * + * @throws ContextException + * @throws SPException + */ + #[Test] + public function theAccountColoursAreCarried() + { + $session = $this->givenAStartedSession(); + + self::assertSame([], $session->getAccountColor(), 'a session with none set has none'); + + $session->setAccountColor([100 => '#ff0000', 200 => '#00ff00']); + + self::assertSame([100 => '#ff0000', 200 => '#00ff00'], $session->getAccountColor()); + } + + /** + * And the theme is a string even before one has been chosen — read from a session that has not + * been given one, it used to return null from a method declared to return a string. + * + * @throws ContextException + * @throws SPException + */ + #[Test] + public function theThemeIsCarriedAndIsAlwaysAString() + { + $session = $this->givenAStartedSession(); + + self::assertSame('', $session->getTheme()); + + $session->setTheme('material-blue'); + + self::assertSame('material-blue', $session->getTheme()); + } + /** * @throws ContextException * @throws SPException