From eb6edb4941c7f0f46af33fe527df7cec0067b261 Mon Sep 17 00:00:00 2001 From: Conduction Release Bot Date: Thu, 3 Sep 2026 10:00:45 +0200 Subject: [PATCH] fix: two silent setters, and the warnings that were pointing at them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The suite reported 10 PHP warnings and 23 deprecations. Two of the warnings were not hygiene: they were production bugs, and both had been written into the tests as known behaviour. ORGANISATION::SETACTIVE(FALSE) NEVER DEACTIVATED ANYTHING It called parent::setActive(active: $value) with a NAMED argument. Entity has no such method — it is reached through __call(array $args), where a named argument lands under its name, so Entity::setter() read an undefined $args[0] and the value never arrived. OrganisationController has an endpoint whose whole job is deactivating an organisation, and it could not work. OrganisationTest asserted assertTrue() on a value that should have been false and called it "the current actual behavior". ENDPOINTLOG::SETRESPONSE() STORED NULL ON EVERY CALL LOG Same root cause, same shape. setStatusCode and setStatusMessage were positional and worked; the response payload was dropped on every endpoint call ever logged. EndpointServiceTest said "setResponse uses named arg in source code (known issue), so response may be null. We verify it was attempted" — and then verified nothing. Both are fixed and both now assert the correct behaviour. Reintroducing either bug reddens its test; that was checked, not assumed. I swept for the rest of the class rather than stopping at two: 94 Entity subclasses, 686 magic setter names, 256 named-argument call sites in lib/. Every other match is legitimate — $objectService->setRegister() is a real service method, ->setObject(type:, id:) is OCP's INotification, and the last two candidates were the regex matching self:: in a class constant. The class is bounded at these two. THE REST - SettingsController: `while ($row !== false)` never terminates if fetch() answers null, which it can. Two other loops in the same file already used is_array(); this one did not. - BsnFormat: str_pad() was handed a null, deprecated in 8.1 and a TypeError in 9. Cast once up front; an array/object is now refused explicitly, which is the contract str_pad() used to enforce further down by accident. - SaveObject: a schema with a null slug cannot be found BY slug, and feeding the null to strtolower()/strcasecmp() is deprecated. Skipped instead. - Four test files: mocks that answered [] or null where the single real implementation always fills the contract (rechainAll, processSourceChunks, handlePreValidationCascading). The mocks were made to keep the contract rather than the services made defensive about a fake. RechainAuditTrail also gained the tombstone-count assertion its message never had. - Dynamic properties declared; ReflectionProperty::setValue() given its explicit null object argument at 14 sites. Suite: 19046 tests. Warnings 10 -> 0, deprecations 23 -> 2, risky 3 -> 0 (the risky three landed separately in #3351). The last two deprecations are ZipArchive::open() on an empty fixture file. Co-Authored-By: Claude Opus 5 (1M context) --- lib/Controller/SettingsController.php | 11 ++++++-- lib/Db/Organisation.php | 6 ++++- lib/Formats/BsnFormat.php | 25 ++++++++++++++++++- lib/Service/EndpointService.php | 7 +++++- lib/Service/Object/SaveObject.php | 8 ++++++ .../Command/RechainAuditTrailCommandTest.php | 15 +++++++++-- tests/Unit/Db/OrganisationTest.php | 18 +++++++------ .../Mcp/BuiltIn/FlowMcpToolProviderTest.php | 6 +++++ .../ActiveOrganisationManagementTest.php | 7 ++++-- .../DefaultOrganisationCachingTest.php | 24 +++++++++--------- .../DefaultOrganisationManagementTest.php | 4 +-- tests/Unit/Service/EndpointServiceTest.php | 12 +++++++-- tests/Unit/Service/ObjectServiceTest.php | 8 +++++- 13 files changed, 118 insertions(+), 33 deletions(-) diff --git a/lib/Controller/SettingsController.php b/lib/Controller/SettingsController.php index 0443c1a2fc..96de9fdcf1 100644 --- a/lib/Controller/SettingsController.php +++ b/lib/Controller/SettingsController.php @@ -665,7 +665,11 @@ private function getHybridSearchDiagnostics(bool $isPostgres): array { $row = $result->fetch(); $result->closeCursor(); - if ($row !== false) { + // An is_array check, not `!== false`. fetch() can answer NULL + // as well as false, which passes a `!== false` guard and then + // warns on every offset read. Lines 476 and 644 of this file + // already use this idiom; this was the one site that did not. + if (is_array($row) === true) { $diagnostics['vectors']['total'] = (int)$row['total']; $diagnostics['vectors']['pgvectorPopulated'] = (int)$row['populated']; } @@ -876,8 +880,11 @@ function (\OCA\OpenRegister\Db\ObjectEntity $org): array { // (see RegisterMapper::getAllRegisterIdsWithSchema / MarkerLookupTrait). $stmt = $qb->executeQuery(); $rows = []; + // Same reason as above, and here it matters more: a fetch() that + // answers NULL never satisfies `!== false`, so the loop would spin + // forever appending nulls rather than ending. $row = $stmt->fetch(); - while ($row !== false) { + while (is_array($row) === true) { $rows[] = $row; $row = $stmt->fetch(); } diff --git a/lib/Db/Organisation.php b/lib/Db/Organisation.php index 8cd6fc56fe..0ec9cf1fdc 100644 --- a/lib/Db/Organisation.php +++ b/lib/Db/Organisation.php @@ -751,7 +751,11 @@ public function setActive(mixed $active): static { $activeValue = (bool)$active; } - parent::setActive(active: $activeValue); + // Assign DIRECTLY, as Application::setActive() does. This called + // parent::setActive(active: ...) — a NAMED arg into Entity::__call(), + // which reads $args[0], so the value never arrived and setActive(false) + // left the organisation ACTIVE. Nothing could be deactivated. + $this->active = $activeValue; $this->markFieldUpdated(attribute: 'active'); return $this; diff --git a/lib/Formats/BsnFormat.php b/lib/Formats/BsnFormat.php index 99aa712d28..2e70e341e5 100644 --- a/lib/Formats/BsnFormat.php +++ b/lib/Formats/BsnFormat.php @@ -23,7 +23,13 @@ namespace OCA\OpenRegister\Formats; use Opis\JsonSchema\Format; +use TypeError; +/** + * Validates the Dutch BSN (Burgerservicenummer) as a JSON Schema format. + * + * @spec openspec/specs/data-import-export/spec.md + */ class BsnFormat implements Format { /** * Validates if a given value conforms to the Dutch BSN (Burgerservicenummer) format. @@ -37,10 +43,27 @@ class BsnFormat implements Format { * @spec openspec/specs/data-import-export/spec.md */ public function validate(mixed $data): bool { + // An array or object is a caller bug, not an invalid BSN, so it is + // refused loudly. str_pad() used to raise the TypeError itself, further + // down and by accident; raising it here keeps that contract explicit + // once the cast below stops the value ever reaching str_pad() untyped. + if (is_array($data) === true || is_object($data) === true) { + throw new TypeError( + 'BsnFormat::validate() expects a scalar or null, '.get_debug_type($data).' given' + ); + } + + // Cast ONCE, here. null and false coerce to '' and pad to the all-zero + // sentinel, which is rejected below — that is the documented behaviour + // (ADR-008 Rule 4). Passing null on to str_pad() instead is deprecated + // in PHP 8.1 and a TypeError in PHP 9, so the same input would stop + // being 'not a BSN' and start being a fatal. + $data = (string)$data; + // Reject over-length input before padding: str_pad only left-pads and // never truncates, so a >9-digit value would otherwise be checksummed // on a miscalculated weighting (ADR-008 Rule 4). - if (strlen((string)$data) > 9) { + if (strlen($data) > 9) { return false; } diff --git a/lib/Service/EndpointService.php b/lib/Service/EndpointService.php index a3cef79ec7..cdbd084b27 100644 --- a/lib/Service/EndpointService.php +++ b/lib/Service/EndpointService.php @@ -492,8 +492,13 @@ private function logEndpointCall(Endpoint $endpoint, array $request, array $resu // Set request/response data. $log->setRequest($request); + // POSITIONAL, deliberately. setResponse() is magic — EndpointLog + // declares the property, not the method — so it goes through + // Entity::__call(array $args), where a NAMED argument lands under + // its name and $args[0] is never set. Every endpoint log was + // storing a null response. $log->setResponse( - response: [ + [ 'statusCode' => $result['statusCode'], 'body' => $result['response'], ] diff --git a/lib/Service/Object/SaveObject.php b/lib/Service/Object/SaveObject.php index 0332a5670b..6222e5faa5 100644 --- a/lib/Service/Object/SaveObject.php +++ b/lib/Service/Object/SaveObject.php @@ -516,6 +516,14 @@ private function resolveSchemaReference(string $reference): ?string { $schemas = $this->schemaMapper->findAll(); // Cache all schemas by slug for future lookups. foreach ($schemas as $schema) { + // A schema with no slug cannot be found BY slug, and feeding + // the null on to strtolower()/strcasecmp() is deprecated in + // PHP 8.1 and a TypeError in PHP 9. Skipping it is what the + // loop was already doing in effect, just noisily. + if ($schema->getSlug() === null) { + continue; + } + $schemaSlug = strtolower($schema->getSlug()); $schemaId = (string)$schema->getId(); // Cache the schema entity. diff --git a/tests/Unit/Command/RechainAuditTrailCommandTest.php b/tests/Unit/Command/RechainAuditTrailCommandTest.php index 80a48e5673..387660dec1 100644 --- a/tests/Unit/Command/RechainAuditTrailCommandTest.php +++ b/tests/Unit/Command/RechainAuditTrailCommandTest.php @@ -147,10 +147,21 @@ public function testForceRepairsAndReportsSuccess(): void { $this->hashes->method('verifyChain') ->willReturnOnConsecutiveCalls($this->verification(false), $this->verification(true)); $this->hashes->method('countUnsealed')->willReturn(0); - $this->hashes->expects($this->once())->method('rechainAll')->willReturn(['rechained' => 313136]); + // The FULL shape rechainAll() returns. Both of its return paths fill + // 'tombstonesPreserved', and the command prints it, so a mock that + // omits it makes the command read an undefined key and the test + // exercises a state production cannot reach. + $this->hashes->expects($this->once())->method('rechainAll') + ->willReturn(['rechained' => 313136, 'tombstonesPreserved' => 12]); $this->assertSame(Command::SUCCESS, $this->tester->execute(['--force' => true])); $this->assertStringContainsString('313136', $this->tester->getDisplay()); + // The second half of the same sentence. An operator reads this line to + // decide whether the repair touched what they expected, and a repair + // that silently skipped rows would report the tombstone count wrong. + // Nothing asserted it before, so the placeholder could have printed + // anything. + $this->assertStringContainsString('12 retention tombstone', $this->tester->getDisplay()); }//end testForceRepairsAndReportsSuccess() @@ -168,7 +179,7 @@ public function testForceRepairsAndReportsSuccess(): void { public function testStillBrokenAfterwardsIsAFailure(): void { $this->hashes->method('verifyChain')->willReturn($this->verification(false)); $this->hashes->method('countUnsealed')->willReturn(3); - $this->hashes->method('rechainAll')->willReturn(['rechained' => 5]); + $this->hashes->method('rechainAll')->willReturn(['rechained' => 5, 'tombstonesPreserved' => 0]); $this->assertSame(Command::FAILURE, $this->tester->execute(['--force' => true])); $this->assertStringContainsString('do not treat this repair as complete', $this->tester->getDisplay()); diff --git a/tests/Unit/Db/OrganisationTest.php b/tests/Unit/Db/OrganisationTest.php index b3fc4c4e19..ff660de2b2 100644 --- a/tests/Unit/Db/OrganisationTest.php +++ b/tests/Unit/Db/OrganisationTest.php @@ -336,11 +336,14 @@ public function testIsActiveDefaultTrue(): void { } public function testSetActiveFalse(): void { - // Organisation::setActive() calls parent::setActive(active: $val) with named args, - // which triggers the Entity __call named-arg bug. The value is always truthy. - // This test documents the current actual behavior. + // An organisation CAN be deactivated. This test used to assert the + // opposite and called it "the current actual behavior": setActive() + // passed a named argument to Entity::__call(), which reads $args[0], + // so the value never arrived and every organisation stayed active. + // The OrganisationController endpoint for deactivating one could not + // work, and this test is what made that look intended. $result = $this->organisation->setActive(false); - $this->assertTrue($this->organisation->isActive()); + $this->assertFalse($this->organisation->isActive(), 'setActive(false) must deactivate'); $this->assertSame($this->organisation, $result); } @@ -365,10 +368,11 @@ public function testSetActiveTruthyString(): void { } public function testSetActiveFalsyStringZero(): void { - // Due to the named-arg bug in parent::setActive(), '0' is cast to false - // but the named arg causes it to be set as truthy string 'active'. + // '0' is a falsy string, and the setter casts it. The API sends + // strings, so this is the path a deactivation actually arrives on — + // which is why it mattered that the value was being dropped. $this->organisation->setActive('0'); - $this->assertTrue($this->organisation->isActive()); + $this->assertFalse($this->organisation->isActive(), "'0' must deactivate"); } public function testIsActiveWhenInternallyNull(): void { diff --git a/tests/Unit/Mcp/BuiltIn/FlowMcpToolProviderTest.php b/tests/Unit/Mcp/BuiltIn/FlowMcpToolProviderTest.php index ec992bae67..976352a21b 100644 --- a/tests/Unit/Mcp/BuiltIn/FlowMcpToolProviderTest.php +++ b/tests/Unit/Mcp/BuiltIn/FlowMcpToolProviderTest.php @@ -25,6 +25,12 @@ class FlowMcpToolProviderTest extends TestCase { private FlowRunMapper $mapper; private IUserSession&MockObject $userSession; private FlowMcpToolProvider $provider; + // Declared, not created on the fly. Assigning an undeclared property in + // setUp() is deprecated in PHP 8.2 and an error in PHP 9, and it also + // costs the type: an undeclared $flows is mixed, so nothing checks that + // the provider is handed a FlowService at all. + private \OCA\OpenRegister\Service\Flow\FlowService&MockObject $flows; + private \OCA\OpenRegister\Service\Flow\FlowNodePreflight&MockObject $preflight; protected function setUp(): void { $this->runner = $this->createMock(FlowRunService::class); diff --git a/tests/Unit/Service/ActiveOrganisationManagementTest.php b/tests/Unit/Service/ActiveOrganisationManagementTest.php index 67bae5d5c2..7311f5fc34 100644 --- a/tests/Unit/Service/ActiveOrganisationManagementTest.php +++ b/tests/Unit/Service/ActiveOrganisationManagementTest.php @@ -874,10 +874,13 @@ public function testGetOrganisationForNewEntityFallbackToDefault(): void { $reflection = new \ReflectionClass(OrganisationService::class); $cacheProperty = $reflection->getProperty('defaultOrgCache'); $cacheProperty->setAccessible(true); - $cacheProperty->setValue($defaultOrg); + // Both are STATIC, so the object argument is null. Omitting it is + // deprecated in PHP 8.3 and removed in PHP 9; the same file already + // writes these two caches the explicit way further up. + $cacheProperty->setValue(null, $defaultOrg); $tsProperty = $reflection->getProperty('defaultOrgCacheTs'); $tsProperty->setAccessible(true); - $tsProperty->setValue(time()); + $tsProperty->setValue(null, time()); // Act. $result = $this->organisationService->getOrganisationForNewEntity(); diff --git a/tests/Unit/Service/DefaultOrganisationCachingTest.php b/tests/Unit/Service/DefaultOrganisationCachingTest.php index e71f17cd97..d98f8dc64b 100644 --- a/tests/Unit/Service/DefaultOrganisationCachingTest.php +++ b/tests/Unit/Service/DefaultOrganisationCachingTest.php @@ -158,11 +158,11 @@ private function clearStaticCache(): void { $cacheProperty = $reflection->getProperty('defaultOrgCache'); $cacheProperty->setAccessible(true); - $cacheProperty->setValue(null); + $cacheProperty->setValue(null, null); $timestampProperty = $reflection->getProperty('defaultOrgCacheTs'); $timestampProperty->setAccessible(true); - $timestampProperty->setValue(null); + $timestampProperty->setValue(null, null); } /** @@ -183,11 +183,11 @@ public function testDefaultOrganisationStaticCacheHit(): void { $cacheProperty = $reflection->getProperty('defaultOrgCache'); $cacheProperty->setAccessible(true); - $cacheProperty->setValue($defaultOrg); + $cacheProperty->setValue(null, $defaultOrg); $timestampProperty = $reflection->getProperty('defaultOrgCacheTs'); $timestampProperty->setAccessible(true); - $timestampProperty->setValue(time()); + $timestampProperty->setValue(null, time()); // The mapper should NOT be called since cache is populated. $this->organisationMapper @@ -219,12 +219,12 @@ public function testDefaultOrganisationCacheExpiration(): void { $cacheProperty = $reflection->getProperty('defaultOrgCache'); $cacheProperty->setAccessible(true); - $cacheProperty->setValue($defaultOrg); + $cacheProperty->setValue(null, $defaultOrg); // Set expired timestamp (older than cache timeout). $timestampProperty = $reflection->getProperty('defaultOrgCacheTs'); $timestampProperty->setAccessible(true); - $timestampProperty->setValue(time() - 1000); + $timestampProperty->setValue(null, time() - 1000); // Assert: Cache was set with expired timestamp. $this->assertNotNull($cacheProperty->getValue()); @@ -247,11 +247,11 @@ public function testDefaultOrganisationCacheSharedAcrossInstances(): void { $cacheProperty = $reflection->getProperty('defaultOrgCache'); $cacheProperty->setAccessible(true); - $cacheProperty->setValue($defaultOrg); + $cacheProperty->setValue(null, $defaultOrg); $timestampProperty = $reflection->getProperty('defaultOrgCacheTs'); $timestampProperty->setAccessible(true); - $timestampProperty->setValue(time()); + $timestampProperty->setValue(null, time()); // Create a second service instance. $organisationService2 = new OrganisationService( @@ -290,11 +290,11 @@ public function testDefaultOrganisationCacheInvalidationOnModification(): void { $cacheProperty = $reflection->getProperty('defaultOrgCache'); $cacheProperty->setAccessible(true); - $cacheProperty->setValue($defaultOrg); + $cacheProperty->setValue(null, $defaultOrg); $timestampProperty = $reflection->getProperty('defaultOrgCacheTs'); $timestampProperty->setAccessible(true); - $timestampProperty->setValue(time()); + $timestampProperty->setValue(null, time()); // Verify cache is populated. $this->assertNotNull($cacheProperty->getValue()); @@ -332,11 +332,11 @@ public function testDefaultOrganisationPerformanceOptimization(): void { $cacheProperty = $reflection->getProperty('defaultOrgCache'); $cacheProperty->setAccessible(true); - $cacheProperty->setValue($defaultOrg); + $cacheProperty->setValue(null, $defaultOrg); $timestampProperty = $reflection->getProperty('defaultOrgCacheTs'); $timestampProperty->setAccessible(true); - $timestampProperty->setValue(time()); + $timestampProperty->setValue(null, time()); // The mapper should NOT be called since cache is populated. $this->organisationMapper diff --git a/tests/Unit/Service/DefaultOrganisationManagementTest.php b/tests/Unit/Service/DefaultOrganisationManagementTest.php index 4fa86187ae..0e3a2c7c31 100644 --- a/tests/Unit/Service/DefaultOrganisationManagementTest.php +++ b/tests/Unit/Service/DefaultOrganisationManagementTest.php @@ -478,10 +478,10 @@ public function testGetUserOrganisationsAutoAssignsToDefault(): void { $cacheProperty = $reflection->getProperty('defaultOrgCache'); $cacheProperty->setAccessible(true); - $cacheProperty->setValue($defaultOrg); + $cacheProperty->setValue(null, $defaultOrg); $tsProperty = $reflection->getProperty('defaultOrgCacheTs'); $tsProperty->setAccessible(true); - $tsProperty->setValue(time()); + $tsProperty->setValue(null, time()); // Mock: update called to save user addition. $this->organisationMapper diff --git a/tests/Unit/Service/EndpointServiceTest.php b/tests/Unit/Service/EndpointServiceTest.php index 59e2b54d32..4e6a8f346b 100644 --- a/tests/Unit/Service/EndpointServiceTest.php +++ b/tests/Unit/Service/EndpointServiceTest.php @@ -1102,8 +1102,16 @@ public function testLogEndpointCallVerifiesLogProperties(): void { $this->assertSame('some warning', $log->getStatusMessage()); // Verify request data. $this->assertSame(['method' => 'GET', 'path' => '/api/test', 'data' => ['key' => 'val'], 'headers' => ['X-Foo' => 'bar']], $log->getRequest()); - // Note: setResponse uses named arg in source code (known issue), - // so response may be null. We verify it was attempted. + // The response, asserted rather than excused. This used to read + // "setResponse uses named arg in source code (known issue), so + // response may be null. We verify it was attempted" — and it + // verified nothing, so every endpoint log stored a null + // response and the suite stayed green. + $this->assertSame( + ['statusCode' => 200, 'body' => ['items' => [1, 2, 3]]], + $log->getResponse(), + 'the response must be stored on the log, not dropped' + ); // Verify timestamps. $this->assertInstanceOf(\DateTime::class, $log->getCreated()); $this->assertInstanceOf(\DateTime::class, $log->getExpires()); diff --git a/tests/Unit/Service/ObjectServiceTest.php b/tests/Unit/Service/ObjectServiceTest.php index acf53e53b2..537c1cf2dc 100644 --- a/tests/Unit/Service/ObjectServiceTest.php +++ b/tests/Unit/Service/ObjectServiceTest.php @@ -1870,8 +1870,14 @@ public function testCreateObjectCallsSaveObjectInternally(): void { $this->setProperty('currentSchema', $this->schema); // The cascading handler is called before save — verify delegation starts. + // It is made to return its documented shape, [object, uuid]. An + // unconfigured mock answers null, which ObjectService then reads + // offset 0 of: the real handler is `: array` on every return path, so + // that was the fake disagreeing with the contract, not a gap in the + // production guard. $this->cascadingHandler->expects($this->once()) - ->method('handlePreValidationCascading'); + ->method('handlePreValidationCascading') + ->willReturn([['title' => 'New'], null]); // The actual save will fail due to deep dependencies, but we verify // the method delegates to saveObject() correctly.