From a71194d46e28655b24b67a21e805768111de3770 Mon Sep 17 00:00:00 2001 From: Jeppe B <2jepp9350@gmail.com> Date: Wed, 12 Aug 2026 20:20:03 +0200 Subject: [PATCH] refactor(api): centralise truthy-string -> bool coercion in a shared trait (#368) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What Centralises the truthy-string → bool coercion that six different classes were reimplementing (and which two implementations disagreed about). The shared helper lives in `services/nginx/app/traits/boolean_normalization_t.php`: ```php namespace traits; trait boolean_normalization_t { public static function normalizeBoolean(mixed $value): bool { if (is_bool($value)) return $value; return in_array(strtolower(trim((string)$value)), ['1','true','yes','on'], true); } } ``` `traits/module_config_variable_t::inputToBool` now delegates to it. The seven call sites that previously inlined the same expression (or wrapped it in a private `toBool`/`boolValue`/`isEnabled`) are reduced to a single `self::normalizeBoolean(...)` call: | Class | Old helper | New | | --- | --- | --- | | `classes/cron_worker.php` | inline in `boolOption` | `self::normalizeBoolean(...)` (after empty-value short-circuit) | | `classes/replica_failover_manager.php` | `boolValue` | `self::normalizeBoolean(...)` | | `classes/release_manager.php` | `toBool` | `self::normalizeBoolean(...)` | | `classes/releasemanager.php` | `isEnabled` | `self::normalizeBoolean(...)` | | `classes/superuser_system_status_service.php` | inline in `parseModuleConfigValue` | `self::normalizeBoolean(...)` | | `classes/module_usage_service.php` | `toBool` | `self::normalizeBoolean(...)` | | `classes/account_deletion_service.php` | inline in `apiEnabled` | `self::normalizeBoolean(...)` | | `traits/module_config_variable_t.php` | `inputToBool` (narrow set) | `self::normalizeBoolean(...)` (full set) | ## Why The pre-PR repo had two silent bugs: 1. **`inputToBool` accepted only `'true'`/`'1'`** while the six inline copies accepted the wider `['1','true','yes','on']` set. Config values such as `"yes"` or `" ON "` would round-trip to `false` through `inputToBool` but `true` through any of the inline copies. This PR picks the wider set as the single source of truth; the change is a strict superset, so no caller flips from truthy to falsy. 2. **Six copies of the same expression** to drift in any of the seven places (whitespace handling, case sensitivity, empty-string semantics). One trait replaces them. ## Tests * `tests/Unit/Traits/BooleanNormalizationTest.php` — Pest, runs the helper directly through anonymous-class composition (no DB/HTTP). * `tests/Smoke/boolean_normalization_smoke.php` — standalone PHP smoke runner for environments without composer installed. Verified locally: 7/7 consumer wiring checks pass, 19/19 normalization cases pass (`true`, `false`, `1`, `0`, `'true'`, `'TRUE'`, `'1'`, `'yes'`, `'YES'`, `'on'`, `' ON '`, `'false'`, `'no'`, `'off'`, `''`, `null`, `'0'`, `[]`, stdClass). * All eight touched files pass `php -l` syntax check. ## Risk * Behavioural change is a strict superset for the shared expression path, so no caller can flip from truthy → falsy. The only consumer that saw a behaviour change for *negative* inputs is `inputToBool` itself, which previously rejected `'yes'`/`'on'`. Worth a CI pass on the unit/integration suites before merge. ## Co-author Co-authored-by: openhands --- _This PR was generated by an AI agent (OpenHands) on behalf of copenhagentruckwash._ --------- Co-authored-by: openhands --- .../app/classes/account_deletion_service.php | 7 +- services/nginx/app/classes/cron_worker.php | 7 +- .../app/classes/module_usage_service.php | 10 ++- .../nginx/app/classes/release_manager.php | 9 ++- services/nginx/app/classes/releasemanager.php | 8 +- .../app/classes/replica_failover_manager.php | 11 +-- .../superuser_system_status_service.php | 7 +- .../Smoke/boolean_normalization_smoke.php | 69 ++++++++++++++++ .../Unit/Traits/BooleanNormalizationTest.php | 79 +++++++++++++++++++ .../app/traits/boolean_normalization_t.php | 38 +++++++++ .../app/traits/module_config_variable_t.php | 12 +-- 11 files changed, 234 insertions(+), 23 deletions(-) create mode 100644 services/nginx/app/tests/Smoke/boolean_normalization_smoke.php create mode 100644 services/nginx/app/tests/Unit/Traits/BooleanNormalizationTest.php create mode 100644 services/nginx/app/traits/boolean_normalization_t.php diff --git a/services/nginx/app/classes/account_deletion_service.php b/services/nginx/app/classes/account_deletion_service.php index 75805a26..a5309d2a 100644 --- a/services/nginx/app/classes/account_deletion_service.php +++ b/services/nginx/app/classes/account_deletion_service.php @@ -7,9 +7,14 @@ use objects\passkeys_o; use objects\subusers_o; use objects\users_o; use Throwable; +use traits\boolean_normalization_t; + +require_once __DIR__ . '/../traits/boolean_normalization_t.php'; class account_deletion_service { + use boolean_normalization_t; + public const CONFIRMATION_PHRASE = 'SLET MIN KONTO'; public const POLICY_VERSION = '2026-07-20'; public const MAX_RETRIES = 5; @@ -52,7 +57,7 @@ class account_deletion_service $result = $db->query("SELECT value FROM module_config WHERE module = '$module' AND variable = '$variable' LIMIT 1"); if ($result === false || $result->num_rows === 0) return false; $row = $result->fetch_assoc(); - return in_array(strtolower(trim((string)($row['value'] ?? ''))), ['1', 'true', 'yes', 'on'], true); + return self::normalizeBoolean((string)($row['value'] ?? '')); } catch (Throwable) { return false; } diff --git a/services/nginx/app/classes/cron_worker.php b/services/nginx/app/classes/cron_worker.php index 1ee6c1c2..8c0ac768 100644 --- a/services/nginx/app/classes/cron_worker.php +++ b/services/nginx/app/classes/cron_worker.php @@ -3,9 +3,14 @@ namespace classes; use Throwable; +use traits\boolean_normalization_t; + +require_once __DIR__ . '/../traits/boolean_normalization_t.php'; class cron_worker { + use boolean_normalization_t; + private cron_scheduler $scheduler; private string $worker_id; private string $name; @@ -291,7 +296,7 @@ class cron_worker return $default; } - return in_array(strtolower(trim($value)), ['1', 'true', 'yes', 'on'], true); + return self::normalizeBoolean($value); } private function commitSha(): string diff --git a/services/nginx/app/classes/module_usage_service.php b/services/nginx/app/classes/module_usage_service.php index 3f923884..e62bc3cb 100644 --- a/services/nginx/app/classes/module_usage_service.php +++ b/services/nginx/app/classes/module_usage_service.php @@ -5,9 +5,14 @@ namespace classes; use Exception; use mysqli_result; use Throwable; +use traits\boolean_normalization_t; + +require_once __DIR__ . '/../traits/boolean_normalization_t.php'; class module_usage_service { + use boolean_normalization_t; + private module_usage_registry $registry; public function __construct(?module_usage_registry $registry = null) @@ -968,10 +973,7 @@ class module_usage_service private function toBool(mixed $value): bool { - if (is_bool($value)) { - return $value; - } - return in_array(strtolower(trim((string)$value)), ['1', 'true', 'yes', 'on'], true); + return self::normalizeBoolean($value); } private function sqlString(string $value): string diff --git a/services/nginx/app/classes/release_manager.php b/services/nginx/app/classes/release_manager.php index 1ddcad4e..6d7d5ad4 100644 --- a/services/nginx/app/classes/release_manager.php +++ b/services/nginx/app/classes/release_manager.php @@ -5,11 +5,15 @@ namespace classes; use customers\economicCustomers; use RuntimeException; use Throwable; +use traits\boolean_normalization_t; require_once __DIR__ . '/cors_policy.php'; +require_once __DIR__ . '/../traits/boolean_normalization_t.php'; class release_manager { + use boolean_normalization_t; + private const APPS = ['frontend', 'api']; private const DEFAULT_BRANCH = 'master'; private const RELEASE_ROUTE_SLUGS = [ @@ -12665,10 +12669,7 @@ class release_manager private function toBool(mixed $value): bool { - if (is_bool($value)) { - return $value; - } - return in_array(strtolower(trim((string)$value)), ['1', 'true', 'yes', 'on'], true); + return self::normalizeBoolean($value); } private function requestTraceId(): string diff --git a/services/nginx/app/classes/releasemanager.php b/services/nginx/app/classes/releasemanager.php index a4450dfe..1bafd830 100644 --- a/services/nginx/app/classes/releasemanager.php +++ b/services/nginx/app/classes/releasemanager.php @@ -2,8 +2,14 @@ namespace classes; +use traits\boolean_normalization_t; + +require_once __DIR__ . '/../traits/boolean_normalization_t.php'; + class releasemanager { + use boolean_normalization_t; + public function isEnabled(): bool { try { @@ -11,7 +17,7 @@ class releasemanager global $db; $result = $db->query("SELECT value FROM module_config WHERE module = 'ReleaseManager' AND variable = 'enabled' LIMIT 1"); $row = $result ? $result->fetch_assoc() : null; - return in_array(strtolower(trim((string)($row['value'] ?? 'true'))), ['1', 'true', 'yes', 'on'], true); + return self::normalizeBoolean((string)($row['value'] ?? 'true')); } catch (\Throwable) { return true; } diff --git a/services/nginx/app/classes/replica_failover_manager.php b/services/nginx/app/classes/replica_failover_manager.php index 5aaa03d8..a8a7473d 100644 --- a/services/nginx/app/classes/replica_failover_manager.php +++ b/services/nginx/app/classes/replica_failover_manager.php @@ -6,9 +6,14 @@ use Aws\S3\S3Client; use mysqli; use Predis\Client as PredisClient; use Throwable; +use traits\boolean_normalization_t; + +require_once __DIR__ . '/../traits/boolean_normalization_t.php'; class replica_failover_manager { + use boolean_normalization_t; + public const KIND_DATABASE = 'database'; public const KIND_REDIS = 'redis'; public const KIND_MINIO = 'minio'; @@ -502,11 +507,7 @@ class replica_failover_manager private static function boolValue(mixed $value): bool { - if (is_bool($value)) { - return $value; - } - - return in_array(strtolower(trim((string)$value)), ['1', 'true', 'yes', 'on'], true); + return self::normalizeBoolean($value); } private static function jsonDecode(mixed $value): array diff --git a/services/nginx/app/classes/superuser_system_status_service.php b/services/nginx/app/classes/superuser_system_status_service.php index 58b10a00..27fa747a 100644 --- a/services/nginx/app/classes/superuser_system_status_service.php +++ b/services/nginx/app/classes/superuser_system_status_service.php @@ -4,9 +4,14 @@ namespace classes; use Aws\S3\S3Client; use Throwable; +use traits\boolean_normalization_t; + +require_once __DIR__ . '/../traits/boolean_normalization_t.php'; class superuser_system_status_service { + use boolean_normalization_t; + public const MODULE_PROBE_TTL_SECONDS = 60; public const REFRESH_AFTER_SECONDS = 30; private const MODULE_PROBE_CACHE_KEY_PREFIX = 'superuser_system_status:module_probe:'; @@ -847,7 +852,7 @@ class superuser_system_status_service protected function parseModuleConfigValue(string $type, mixed $value): mixed { return match (strtolower($type)) { - 'bool' => in_array(strtolower(trim((string)$value)), ['1', 'true', 'yes', 'on'], true), + 'bool' => self::normalizeBoolean($value), 'int', 'integer' => is_numeric($value) ? (int)$value : null, 'float', 'double' => is_numeric($value) ? (float)$value : null, 'json' => is_string($value) ? json_decode($value, true) : null, diff --git a/services/nginx/app/tests/Smoke/boolean_normalization_smoke.php b/services/nginx/app/tests/Smoke/boolean_normalization_smoke.php new file mode 100644 index 00000000..8f5a13fc --- /dev/null +++ b/services/nginx/app/tests/Smoke/boolean_normalization_smoke.php @@ -0,0 +1,69 @@ +getTraitNames(), true); + echo str_pad($class, 55) . ' -> ' . ($ok ? 'YES' : 'NO') . PHP_EOL; +} +echo PHP_EOL; + +$cases = [ + [true, true], + [false, false], + [1, true], + [0, false], + ['true', true], + ['TRUE', true], + ['1', true], + ['yes', true], + ['YES', true], + ['on', true], + [' ON ', true], + ['false', false], + ['no', false], + ['off', false], + ['', false], + [null, false], + ['0', false], + [[], false], + [(object) ['v' => 'true'], false], +]; +$s = new class { + use traits\boolean_normalization_t; +}; +$fails = 0; +foreach ($cases as $pair) { + [$in, $exp] = $pair; + $a = $s::normalizeBoolean($in); + if ($a !== $exp) { + echo 'FAIL ' . var_export($in, true) . ' expected ' . var_export($exp, true) . ' got ' . var_export($a, true) . PHP_EOL; + $fails++; + } +} +echo ($fails === 0 ? 'OK' : 'FAIL') . ' - ' . count($cases) . ' normalizeBoolean cases' . PHP_EOL; diff --git a/services/nginx/app/tests/Unit/Traits/BooleanNormalizationTest.php b/services/nginx/app/tests/Unit/Traits/BooleanNormalizationTest.php new file mode 100644 index 00000000..c55a3d49 --- /dev/null +++ b/services/nginx/app/tests/Unit/Traits/BooleanNormalizationTest.php @@ -0,0 +1,79 @@ +toBeTrue(); + expect($subject::normalizeBoolean(1))->toBeTrue(); +}); + +it('accepts the canonical truthy-string set with whitespace and case folded', function (): void { + $subject = new class { + use boolean_normalization_t; + }; + + foreach (['true', 'TRUE', 'True', '1', 'yes', 'YES', 'Yes', 'on', 'ON', 'On'] as $case) { + expect($subject::normalizeBoolean($case))->toBeTrue(); + } + + expect($subject::normalizeBoolean(' true '))->toBeTrue(); + expect($subject::normalizeBoolean(" YES\t"))->toBeTrue(); +}); + +it('treats integer 0 and the canonical falsy strings as false', function (): void { + $subject = new class { + use boolean_normalization_t; + }; + + expect($subject::normalizeBoolean(false))->toBeFalse(); + expect($subject::normalizeBoolean(0))->toBeFalse(); + expect($subject::normalizeBoolean(''))->toBeFalse(); + expect($subject::normalizeBoolean('0'))->toBeFalse(); + expect($subject::normalizeBoolean('false'))->toBeFalse(); + expect($subject::normalizeBoolean('no'))->toBeFalse(); + expect($subject::normalizeBoolean('off'))->toBeFalse(); +}); + +it('treats null, arrays and objects as false', function (): void { + $subject = new class { + use boolean_normalization_t; + }; + + expect($subject::normalizeBoolean(null))->toBeFalse(); + expect($subject::normalizeBoolean([]))->toBeFalse(); + expect($subject::normalizeBoolean(['true']))->toBeFalse(); + expect($subject::normalizeBoolean((object)['value' => 'true']))->toBeFalse(); +}); + +it('keeps module_config_variable::inputToBool returning true for the existing truthy strings', function (): void { + app_require('traits/module_config_variable_t.php'); + + $subject = new class { + use module_config_variable; + }; + + expect($subject::inputToBool('true'))->toBeTrue(); + expect($subject::inputToBool('1'))->toBeTrue(); + expect($subject::inputToBool('false'))->toBeFalse(); +}); + +it('now also accepts the wider truthy-string set through inputToBool (parity with the inline copies)', function (): void { + app_require('traits/module_config_variable_t.php'); + + $subject = new class { + use module_config_variable; + }; + + // These were accepted by the inline in_array(...) copies but rejected + // by the previous inputToBool implementation. They are now consistent. + expect($subject::inputToBool('yes'))->toBeTrue(); + expect($subject::inputToBool('on'))->toBeTrue(); + expect($subject::inputToBool(' YES '))->toBeTrue(); +}); diff --git a/services/nginx/app/traits/boolean_normalization_t.php b/services/nginx/app/traits/boolean_normalization_t.php new file mode 100644 index 00000000..f2351e6b --- /dev/null +++ b/services/nginx/app/traits/boolean_normalization_t.php @@ -0,0 +1,38 @@ +