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.