From ee1ddc25977c2fc68be3e9a4ed6f24fb678bf594 Mon Sep 17 00:00:00 2001 From: WilcoLouwerse Date: Mon, 19 Oct 2026 18:29:34 +0200 Subject: [PATCH 1/7] =?UTF-8?q?fix(review):=20#1269=20sq-f1=20=E2=80=94=20?= =?UTF-8?q?UID=20lookup=20relies=20on=20unverified=20contacts-manager=20UI?= =?UTF-8?q?D=20search?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thread: https://github.com/ConductionNL/stackiq/pull/1269#discussion_r4316061889 Nextcloud indexes UID (CardDavBackend::INDEXED_PROPERTIES) and searches it with a LIKE, documented on contactByUid(). As a fallback, syncToNamedAddressBook() now returns a card that already has the fixed UID (getCard by .vcf) instead of letting createCard throw UidConflict and dropping the owner. The test backend refuses duplicate URIs like Nextcloud. Co-Authored-By: Claude Opus 5.5 --- lib/Service/CmdbExportImportService.php | 6 +++ lib/Service/StackiqContactSyncService.php | 7 ++++ .../Service/StackiqContactSyncServiceTest.php | 40 +++++++++++++++++++ 3 files changed, 53 insertions(+) diff --git a/lib/Service/CmdbExportImportService.php b/lib/Service/CmdbExportImportService.php index 564d2d0d2..5c5aab526 100644 --- a/lib/Service/CmdbExportImportService.php +++ b/lib/Service/CmdbExportImportService.php @@ -1653,6 +1653,12 @@ private function existingOwnerContact(?string $numberUid, string $email, string /** * The contact in the owners' address book with exactly this UID. * + * Nextcloud indexes the UID property of every card (CardDavBackend + * INDEXED_PROPERTIES) and AddressBookImpl::search() matches it with a + * LIKE on that index, so a search on ['UID'] finds the card. Should the + * index lag, the second createCard with the same UID is tolerated in + * StackiqContactSyncService::syncToNamedAddressBook(). + * * @param string $uid The contact UID. * * @return string|null The contact UID, or null. diff --git a/lib/Service/StackiqContactSyncService.php b/lib/Service/StackiqContactSyncService.php index b5fc363bc..6ad66be58 100644 --- a/lib/Service/StackiqContactSyncService.php +++ b/lib/Service/StackiqContactSyncService.php @@ -243,6 +243,13 @@ public function syncToNamedAddressBook( displayName: $displayName ); $contactUid = ($fixedUid ?? self::newUid()); + // Nextcloud refuses a second card with the same UID (UidConflict), and + // the contacts search that found no card by it may lag or skip it: a card + // that already has the fixed UID is the contact, not an error. + if ($fixedUid !== null && is_array($backend->getCard($addressBookId, $contactUid . '.vcf')) === true) { + return $contactUid; + } + $backend->createCard($addressBookId, $contactUid . '.vcf', self::serialiseVCard(uid: $contactUid, properties: $properties)); } catch (Throwable $e) { $this->logger->warning( diff --git a/tests/Unit/Service/StackiqContactSyncServiceTest.php b/tests/Unit/Service/StackiqContactSyncServiceTest.php index 368fba5c3..231f6281d 100644 --- a/tests/Unit/Service/StackiqContactSyncServiceTest.php +++ b/tests/Unit/Service/StackiqContactSyncServiceTest.php @@ -64,7 +64,21 @@ public function createAddressBook(string $principal, string $uri, array $propert return $id; } + public function getCard(int $addressBookId, string $cardUri): array|false { + foreach ($this->cards as $card) { + if ($card[0] === $addressBookId && $card[1] === $cardUri) { + return ['uri' => $cardUri, 'carddata' => $card[2]]; + } + } + + return false; + } + public function createCard(int $addressBookId, string $cardUri, string $cardData): string { + if ($this->getCard($addressBookId, $cardUri) !== false) { + throw new \RuntimeException('UidConflict'); + } + $this->cards[] = [$addressBookId, $cardUri, $cardData]; return 'etag'; } @@ -221,6 +235,32 @@ public function testANewContactTakesTheGivenUid(): void { $this->assertStringContainsString("\r\nEMAIL:owner@example.org\r\n", $backend->cards[0][2]); }//end testANewContactTakesTheGivenUid() + /** + * A card that already has the given UID is the contact: a second call returns it and creates nothing. + * + * Nextcloud's createCard refuses a duplicate UID; the owners' import must + * not lose the owner when the contacts search does not show the card. + * + * @return void + * + * @spec openspec/changes/cmdb-import-owner-identity/tasks.md#task-2 + */ + public function testAnExistingCardWithTheGivenUidIsReused(): void { + $backend = $this->backend(); + $service = $this->service(backend: $backend); + $call = fn (): ?string => $service->syncToNamedAddressBook( + objectType: 'contactPerson', + record: ['voornaam' => 'Voornaam', 'achternaam' => 'Achternaam'], + addressBookUri: 'stackiq-cmdb-owners', + displayName: 'Stackiq CMDB owners', + fixedUid: 'stackiq-cmdb-owner-abc' + ); + + $this->assertSame('stackiq-cmdb-owner-abc', $call()); + $this->assertSame('stackiq-cmdb-owner-abc', $call()); + $this->assertCount(1, $backend->cards); + }//end testAnExistingCardWithTheGivenUidIsReused() + /** * With a given UID the e-mail address matches no existing contact, so a shared mailbox does not merge two owners. * From 9a89a8ff7cbc04c85547e55646fa7f391138b8e0 Mon Sep 17 00:00:00 2001 From: WilcoLouwerse Date: Mon, 19 Oct 2026 18:29:42 +0200 Subject: [PATCH 2/7] =?UTF-8?q?docs(review):=20#1269=20sq-f2=20=E2=80=94?= =?UTF-8?q?=20Missing=20secret=20'fail-closed'=20degrades=20to=20the=20nam?= =?UTF-8?q?esake=20merge?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thread: https://github.com/ConductionNL/stackiq/pull/1269#discussion_r4316061898 The docblock now says the fail mode degrades to e-mail/name matching; docs/features/cmdb-import.md states the missing-secret fallback too (same commit series, see sq-f3). Co-Authored-By: Claude Opus 5.5 --- lib/Service/Cmdb/CmdbOwnerIdentity.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/Service/Cmdb/CmdbOwnerIdentity.php b/lib/Service/Cmdb/CmdbOwnerIdentity.php index 794bb42fe..b37710533 100644 --- a/lib/Service/Cmdb/CmdbOwnerIdentity.php +++ b/lib/Service/Cmdb/CmdbOwnerIdentity.php @@ -74,8 +74,10 @@ public function __construct( * e-mail address or the name instead of sharing one UID with every * other owner whose cell holds the same placeholder. * - * Fail-mode (ADR-102): closed. Without a `secret` the number is not used - * (null, logged once), never hashed with an empty key. A changed secret + * Fail-mode (ADR-102): degrades. Without a `secret` the employee number is + * not used (null, logged once), never hashed with an empty key, and the + * owner is matched on the e-mail address or the name like before the + * number existed, so two namesakes become one contact. A changed secret * gives every owner a new UID, so a new contact on the next import. * * @param string $employeeNumber The employee number from the owner pack. From 23741f7415d4fe205d8df615560bf0faef05fc0f Mon Sep 17 00:00:00 2001 From: WilcoLouwerse Date: Mon, 19 Oct 2026 18:29:42 +0200 Subject: [PATCH 3/7] =?UTF-8?q?docs(review):=20#1269=20sq-f3=20=E2=80=94?= =?UTF-8?q?=20Docs=20omit=20that=20adding=20an=20e-mail=20also=20splits=20?= =?UTF-8?q?a=20name-only=20contact?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thread: https://github.com/ConductionNL/stackiq/pull/1269#discussion_r4316061907 Also documents the missing-secret fallback (sq-f2) in the same paragraph. Co-Authored-By: Claude Opus 5.5 --- docs/features/cmdb-import.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/docs/features/cmdb-import.md b/docs/features/cmdb-import.md index d7b5c4461..ae0e12c14 100644 --- a/docs/features/cmdb-import.md +++ b/docs/features/cmdb-import.md @@ -377,7 +377,12 @@ placeholder names only: once you rename the columns, an export that has them on only one of the two CMDB sheets warns that the column is missing on the other. Once you add the employee number, an owner who was found by name or e-mail address until then gets a new contact; the old contact -stays in **Stackiq CMDB owners**. +stays in **Stackiq CMDB owners**. The same happens when an owner first +imported by name alone arrives with an e-mail address: the name-only +contact is not found by that address, so a second contact is created next +to it. An instance without a `secret` in `config.php` cannot use the +employee number: owners are then matched on e-mail address or name, so +two namesakes become one contact (the log warns once). When the Contacts app is disabled, applications and usages are still imported; the owners are skipped and each affected row carries a warning. From ed7cea90539ace54f06544332a5fe15f697031fd Mon Sep 17 00:00:00 2001 From: WilcoLouwerse Date: Mon, 19 Oct 2026 18:33:50 +0200 Subject: [PATCH 4/7] =?UTF-8?q?fix(review):=20#1269=20sq-f1=20=E2=80=94=20?= =?UTF-8?q?keep=20the=20fixed-UID=20card=20lookup=20out=20of=20syncToNamed?= =?UTF-8?q?AddressBook?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thread: https://github.com/ConductionNL/stackiq/pull/1269#discussion_r4316061889 Moves the existing-card-by-URI check into namedAddressBookContactByEmail() so syncToNamedAddressBook() stays under the phpmd complexity limit and phpcs inline-IF rule. Co-Authored-By: Claude Opus 5.5 --- lib/Service/StackiqContactSyncService.php | 26 +++++++++++++++-------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/lib/Service/StackiqContactSyncService.php b/lib/Service/StackiqContactSyncService.php index 6ad66be58..0428654c9 100644 --- a/lib/Service/StackiqContactSyncService.php +++ b/lib/Service/StackiqContactSyncService.php @@ -243,13 +243,6 @@ public function syncToNamedAddressBook( displayName: $displayName ); $contactUid = ($fixedUid ?? self::newUid()); - // Nextcloud refuses a second card with the same UID (UidConflict), and - // the contacts search that found no card by it may lag or skip it: a card - // that already has the fixed UID is the contact, not an error. - if ($fixedUid !== null && is_array($backend->getCard($addressBookId, $contactUid . '.vcf')) === true) { - return $contactUid; - } - $backend->createCard($addressBookId, $contactUid . '.vcf', self::serialiseVCard(uid: $contactUid, properties: $properties)); } catch (Throwable $e) { $this->logger->warning( @@ -297,8 +290,10 @@ public function searchNamedAddressBook(string $query, string $addressBookUri, ar /** * The contact in the named address book with the record's e-mail address, or null. * - * Always null with a fixed UID: the caller identifies the contact by - * that UID, and a shared e-mail address must not merge two of them. + * With a fixed UID the e-mail address matches nothing: the caller + * identifies the contact by that UID, and a shared e-mail address must + * not merge two of them. Only a card that already has that UID (by URI) + * is returned. * * @param array $record The relationship record. * @param string $addressBookUri The address book's URI. @@ -310,6 +305,19 @@ public function searchNamedAddressBook(string $query, string $addressBookUri, ar */ private function namedAddressBookContactByEmail(array $record, string $addressBookUri, ?string $fixedUid): ?string { if ($fixedUid !== null) { + // A card that already has the fixed UID is the contact: Nextcloud + // refuses a second one (UidConflict), and the contacts search may not + // show it yet. Looked up by URI, not by search. + $key = $this->existingNamedAddressBookKey(addressBookUri: $addressBookUri); + $backend = null; + if ($key !== null) { + $backend = $this->container?->get(static::CARDDAV_BACKEND_CLASS); + } + + if (is_object($backend) === true && is_array($backend->getCard((int)$key, $fixedUid . '.vcf')) === true) { + return $fixedUid; + } + return null; } From ff0516d864d8406c803fba4ed998a58a090a1899 Mon Sep 17 00:00:00 2001 From: WilcoLouwerse Date: Mon, 19 Oct 2026 18:35:55 +0200 Subject: [PATCH 5/7] =?UTF-8?q?docs(review):=20#1269=20sq-f1=20=E2=80=94?= =?UTF-8?q?=20pre-push:=20the=20docblock=20names=20the=20getCard=20reuse,?= =?UTF-8?q?=20not=20a=20tolerated=20createCard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5.5 --- lib/Service/CmdbExportImportService.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/Service/CmdbExportImportService.php b/lib/Service/CmdbExportImportService.php index 5c5aab526..e57fa7fe9 100644 --- a/lib/Service/CmdbExportImportService.php +++ b/lib/Service/CmdbExportImportService.php @@ -1656,8 +1656,10 @@ private function existingOwnerContact(?string $numberUid, string $email, string * Nextcloud indexes the UID property of every card (CardDavBackend * INDEXED_PROPERTIES) and AddressBookImpl::search() matches it with a * LIKE on that index, so a search on ['UID'] finds the card. Should the - * index lag, the second createCard with the same UID is tolerated in - * StackiqContactSyncService::syncToNamedAddressBook(). + * search miss it anyway, StackiqContactSyncService::syncToNamedAddressBook() + * reuses a card found by getCard('.vcf') in the same address book. A + * card with this UID at a different URI is not found that way: createCard + * then throws UidConflict, which is caught and returns null. * * @param string $uid The contact UID. * From 2a6f101f4525afe6bd8c6e905f071de912d5ae0f Mon Sep 17 00:00:00 2001 From: WilcoLouwerse Date: Mon, 19 Oct 2026 19:47:41 +0200 Subject: [PATCH 6/7] fix(ci): construct OpenRegister's pack validator and mapping engine with IL10N when they require it openregister#4669 gave PackDefinitionValidator and MappingEngine a required IL10N constructor parameter, so the production fallback and the tests that built them with no arguments threw ArgumentCountError. OpenRegisterInstance builds them by reflection: no arguments for older OpenRegister, OpenRegister's translations (IFactory from the container) when the constructor asks for an IL10N, null (MAPPING_UNAVAILABLE) otherwise. Tests build them through CmdbTestSupport::newPackClass() with a pass-through IL10N. Co-Authored-By: Claude Opus 5.5 --- lib/Service/Cmdb/CmdbImportProfile.php | 4 +- lib/Service/Cmdb/OpenRegisterInstance.php | 105 ++++++++++++++++++ lib/Service/CmdbExportImportService.php | 5 +- .../Service/Cmdb/CmdbImportProfileTest.php | 6 +- .../Service/Cmdb/OpenRegisterInstanceTest.php | 95 ++++++++++++++++ tests/Unit/Support/CmdbTestSupport.php | 51 +++++++++ 6 files changed, 258 insertions(+), 8 deletions(-) create mode 100644 lib/Service/Cmdb/OpenRegisterInstance.php create mode 100644 tests/Unit/Service/Cmdb/OpenRegisterInstanceTest.php diff --git a/lib/Service/Cmdb/CmdbImportProfile.php b/lib/Service/Cmdb/CmdbImportProfile.php index 797ae0431..ce36506ba 100644 --- a/lib/Service/Cmdb/CmdbImportProfile.php +++ b/lib/Service/Cmdb/CmdbImportProfile.php @@ -1163,8 +1163,8 @@ private function resolveValidator(): object { } if (class_exists($class) === true) { - $validator = new $class(); - if (method_exists($validator, 'validate') === true) { + $validator = (new OpenRegisterInstance(container: $this->container))->build(class: $class); + if ($validator !== null && method_exists($validator, 'validate') === true) { return $validator; } } diff --git a/lib/Service/Cmdb/OpenRegisterInstance.php b/lib/Service/Cmdb/OpenRegisterInstance.php new file mode 100644 index 000000000..58b050174 --- /dev/null +++ b/lib/Service/Cmdb/OpenRegisterInstance.php @@ -0,0 +1,105 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @link https://github.com/ConductionNL/stackiq + * + * @spec openspec/changes/cmdb-export-import/tasks.md#task-3 + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\Stackiq\Service\Cmdb; + +use OCP\IL10N; +use OCP\L10N\IFactory; +use Psr\Container\ContainerInterface; +use ReflectionClass; +use Throwable; + +/** + * Builds an OpenRegister class with the constructor arguments it declares. + * + * @spec openspec/changes/cmdb-export-import/tasks.md#task-3 + */ +class OpenRegisterInstance { + /** + * Replaces the translations lookup; for unit tests that run without a Nextcloud server. + * + * @var (callable(): IL10N)|null + */ + public static $l10nOverride = null; + + /** + * Constructor. + * + * @param ContainerInterface $container The container that resolves the translation factory. + */ + public function __construct( + private readonly ContainerInterface $container, + ) { + }//end __construct() + + /** + * Construct the class: without arguments, or with OpenRegister's translations when its constructor takes an IL10N. + * + * @param string $class The OpenRegister class. + * + * @return object|null The instance, or null when it cannot be built. + * + * @spec openspec/changes/cmdb-export-import/tasks.md#task-3 + */ + public function build(string $class): ?object { + try { + $constructor = (new ReflectionClass($class))->getConstructor(); + $arguments = []; + foreach (($constructor?->getParameters() ?? []) as $parameter) { + if ($parameter->isOptional() === true) { + break; + } + + $type = $parameter->getType(); + if ($type instanceof \ReflectionNamedType && $type->getName() === IL10N::class) { + $arguments[] = $this->l10n(); + continue; + } + + return null; + } + + return new $class(...$arguments); + } catch (Throwable $e) { + return null; + } + }//end build() + + /** + * OpenRegister's translations. + * + * @return IL10N + * + * @spec openspec/changes/cmdb-export-import/tasks.md#task-3 + */ + private function l10n(): IL10N { + if (self::$l10nOverride !== null) { + return (self::$l10nOverride)(); + } + + $factory = $this->container->get(IFactory::class); + + return $factory->get('openregister'); + }//end l10n() +}//end class diff --git a/lib/Service/CmdbExportImportService.php b/lib/Service/CmdbExportImportService.php index e57fa7fe9..f7e8bf8d4 100644 --- a/lib/Service/CmdbExportImportService.php +++ b/lib/Service/CmdbExportImportService.php @@ -69,6 +69,7 @@ use OCA\Stackiq\Service\Cmdb\CmdbImportProfile; use OCA\Stackiq\Service\Cmdb\CmdbImportReport; use OCA\Stackiq\Service\Cmdb\CmdbOwnerIdentity; +use OCA\Stackiq\Service\Cmdb\OpenRegisterInstance; use OCA\Stackiq\Service\Cmdb\CmdbRowNormaliser; use OCA\Stackiq\Service\Cmdb\CmdbWorkbookReader; use OCP\AppFramework\Db\DoesNotExistException; @@ -2919,8 +2920,8 @@ private function resolveEngine(): object { } if (class_exists($class) === true) { - $engine = new $class(); - if (method_exists($engine, 'mapRow') === true) { + $engine = (new OpenRegisterInstance(container: $this->container))->build(class: $class); + if ($engine !== null && method_exists($engine, 'mapRow') === true) { return $engine; } } diff --git a/tests/Unit/Service/Cmdb/CmdbImportProfileTest.php b/tests/Unit/Service/Cmdb/CmdbImportProfileTest.php index b3dd34995..1cc8eec83 100644 --- a/tests/Unit/Service/Cmdb/CmdbImportProfileTest.php +++ b/tests/Unit/Service/Cmdb/CmdbImportProfileTest.php @@ -23,8 +23,6 @@ require_once __DIR__ . '/../../Support/CmdbTestSupport.php'; require_once __DIR__ . '/../../Support/FakeMigrationPackService.php'; -use OCA\OpenRegister\Service\MigrationPack\MappingEngine; -use OCA\OpenRegister\Service\MigrationPack\PackDefinitionValidator; use OCA\Stackiq\Exception\CmdbImportException; use OCA\Stackiq\Service\Cmdb\CmdbImportPackMarker; use OCA\Stackiq\Service\Cmdb\CmdbImportPackStore; @@ -86,7 +84,7 @@ public function testEveryPackIsAValidOpenRegisterPack(): void { $profile = new CmdbImportProfile(container: $this->emptyContainer()); $profile->load(); - $validator = new PackDefinitionValidator(); + $validator = CmdbTestSupport::newPackClass(class: 'PackDefinitionValidator'); foreach (CmdbImportProfile::TARGETS as $target) { $pack = $profile->pack(target: $target); $this->assertSame([], $validator->validate($pack), $target); @@ -176,7 +174,7 @@ public function testTheLookupsMapThroughTheEngine(): void { CmdbTestSupport::loadMigrationPack(); $profile = new CmdbImportProfile(container: $this->emptyContainer()); $profile->load(); - $engine = new MappingEngine(); + $engine = CmdbTestSupport::newPackClass(class: 'MappingEngine'); $usage = $engine->mapRow( $profile->pack(target: 'usage'), diff --git a/tests/Unit/Service/Cmdb/OpenRegisterInstanceTest.php b/tests/Unit/Service/Cmdb/OpenRegisterInstanceTest.php new file mode 100644 index 000000000..0b33efc96 --- /dev/null +++ b/tests/Unit/Service/Cmdb/OpenRegisterInstanceTest.php @@ -0,0 +1,95 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @link https://github.com/ConductionNL/stackiq + * + * @spec openspec/changes/cmdb-export-import/tasks.md#task-3 + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\Stackiq\Tests\Unit\Service\Cmdb; + +use OCA\Stackiq\Service\Cmdb\OpenRegisterInstance; +use OCP\IL10N; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\TestCase; +use Psr\Container\ContainerInterface; + +/** + * An OpenRegister class is built with the arguments its constructor declares. + */ +#[CoversClass(OpenRegisterInstance::class)] +class OpenRegisterInstanceTest extends TestCase { + /** + * Reset the translations override. + * + * @return void + */ + protected function tearDown(): void { + OpenRegisterInstance::$l10nOverride = null; + }//end tearDown() + + /** + * A class without constructor parameters (older OpenRegister) is built as is. + * + * @return void + */ + public function testAClassWithoutParametersIsBuilt(): void { + $object = (new OpenRegisterInstance(container: $this->createMock(ContainerInterface::class)))->build(class: \stdClass::class); + + $this->assertInstanceOf(\stdClass::class, $object); + }//end testAClassWithoutParametersIsBuilt() + + /** + * A class that requires an IL10N (newer OpenRegister) gets OpenRegister's translations. + * + * @return void + */ + public function testAClassRequiringTranslationsGetsThem(): void { + $l10n = $this->createMock(IL10N::class); + OpenRegisterInstance::$l10nOverride = static fn (): IL10N => $l10n; + $class = new class($l10n) { + public function __construct(public readonly IL10N $l10n) { + } + }; + + $object = (new OpenRegisterInstance(container: $this->createMock(ContainerInterface::class)))->build(class: $class::class); + + $this->assertInstanceOf($class::class, $object); + $this->assertSame($l10n, $object->l10n); + }//end testAClassRequiringTranslationsGetsThem() + + /** + * A constructor needing anything else, or translations that cannot be resolved, gives null. + * + * @return void + */ + public function testAnUnbuildableClassGivesNull(): void { + $container = $this->createMock(ContainerInterface::class); + $other = new class('x') { + public function __construct(public readonly string $value) { + } + }; + $this->assertNull((new OpenRegisterInstance(container: $container))->build(class: $other::class)); + + OpenRegisterInstance::$l10nOverride = static function (): IL10N { + throw new \RuntimeException('no factory'); + }; + $needsL10n = new class($this->createMock(IL10N::class)) { + public function __construct(public readonly IL10N $l10n) { + } + }; + $this->assertNull((new OpenRegisterInstance(container: $container))->build(class: $needsL10n::class)); + }//end testAnUnbuildableClassGivesNull() +}//end class diff --git a/tests/Unit/Support/CmdbTestSupport.php b/tests/Unit/Support/CmdbTestSupport.php index b562042ca..e3db45701 100644 --- a/tests/Unit/Support/CmdbTestSupport.php +++ b/tests/Unit/Support/CmdbTestSupport.php @@ -102,6 +102,7 @@ public static function loadMigrationPack(): string { $base = $dir . '/lib/Service/MigrationPack'; } + \OCA\Stackiq\Service\Cmdb\OpenRegisterInstance::$l10nOverride = static fn (): \OCP\IL10N => self::l10n(); foreach (['PackDefinitionValidator', 'MappingEngine'] as $class) { if (class_exists('OCA\\OpenRegister\\Service\\MigrationPack\\' . $class, false) === false) { require_once $base . '/' . $class . '.php'; @@ -112,6 +113,56 @@ public static function loadMigrationPack(): string { return $source; }//end loadMigrationPack() + /** + * A pass-through IL10N. + * + * @return \OCP\IL10N + */ + private static function l10n(): \OCP\IL10N { + return new class implements \OCP\IL10N { + public function t(string $text, $parameters = []): string { + return vsprintf($text, (array)$parameters); + } + + public function n(string $text_singular, string $text_plural, int $count, array $parameters = []): string { + return vsprintf(($count === 1 ? $text_singular : $text_plural), $parameters); + } + + public function l(string $type, $data, array $options = []) { + return (string)$data; + } + + public function getLanguageCode(): string { + return 'en'; + } + + public function getLocaleCode(): string { + return 'en'; + } + }; + }//end l10n() + + /** + * An OpenRegister pack class, built with a pass-through IL10N when its constructor requires one. + * + * Newer OpenRegister versions give PackDefinitionValidator and MappingEngine + * an IL10N parameter, older ones have none; this works against both. + * + * @param string $class PackDefinitionValidator or MappingEngine. + * + * @return object + */ + public static function newPackClass(string $class): object { + self::loadMigrationPack(); + $name = 'OCA\\OpenRegister\\Service\\MigrationPack\\' . $class; + $required = (new \ReflectionClass($name))->getConstructor()?->getNumberOfRequiredParameters() ?? 0; + if ($required === 0) { + return new $name(); + } + + return new $name(self::l10n()); + }//end newPackClass() + /** * Make PhpSpreadsheet loadable from OpenRegister's vendor directory. * From 7d31fc2e1603cc7f7fc02570a70c1b707e5a1f9a Mon Sep 17 00:00:00 2001 From: WilcoLouwerse Date: Mon, 19 Oct 2026 19:51:39 +0200 Subject: [PATCH 7/7] fix(ci): resolve the IL10N through the container only, no static test hook OpenRegisterInstance no longer carries a test-only static override. The tests hand the code under test a container double whose get(IFactory::class) returns a factory double yielding a pass-through IL10N (CmdbTestSupport::emptyContainer() and translationFactory(), and FakeMigrationPackService::container()). OpenRegisterInstanceTest covers the container without IFactory: null, so MAPPING_UNAVAILABLE. Co-Authored-By: Claude Opus 5.5 --- lib/Service/Cmdb/OpenRegisterInstance.php | 11 ----- ...ettingsControllerCmdbImportMappingTest.php | 4 +- tests/Unit/Repair/SeedCmdbImportPacksTest.php | 3 +- .../Service/Cmdb/CmdbImportProfileTest.php | 14 +++--- .../Service/Cmdb/CmdbWorkbookReaderTest.php | 3 +- .../Service/Cmdb/OpenRegisterInstanceTest.php | 26 ++++------- .../Service/CmdbExportImportServiceTest.php | 6 ++- tests/Unit/Support/CmdbTestSupport.php | 46 ++++++++++++++++++- .../Unit/Support/FakeMigrationPackService.php | 2 +- 9 files changed, 70 insertions(+), 45 deletions(-) diff --git a/lib/Service/Cmdb/OpenRegisterInstance.php b/lib/Service/Cmdb/OpenRegisterInstance.php index 58b050174..400075b2f 100644 --- a/lib/Service/Cmdb/OpenRegisterInstance.php +++ b/lib/Service/Cmdb/OpenRegisterInstance.php @@ -36,13 +36,6 @@ * @spec openspec/changes/cmdb-export-import/tasks.md#task-3 */ class OpenRegisterInstance { - /** - * Replaces the translations lookup; for unit tests that run without a Nextcloud server. - * - * @var (callable(): IL10N)|null - */ - public static $l10nOverride = null; - /** * Constructor. * @@ -94,10 +87,6 @@ public function build(string $class): ?object { * @spec openspec/changes/cmdb-export-import/tasks.md#task-3 */ private function l10n(): IL10N { - if (self::$l10nOverride !== null) { - return (self::$l10nOverride)(); - } - $factory = $this->container->get(IFactory::class); return $factory->get('openregister'); diff --git a/tests/Unit/Controller/SettingsControllerCmdbImportMappingTest.php b/tests/Unit/Controller/SettingsControllerCmdbImportMappingTest.php index 49fac6991..8564d9552 100644 --- a/tests/Unit/Controller/SettingsControllerCmdbImportMappingTest.php +++ b/tests/Unit/Controller/SettingsControllerCmdbImportMappingTest.php @@ -89,9 +89,7 @@ protected function tearDown(): void { * @return ContainerInterface */ private function emptyContainer(): ContainerInterface { - $container = $this->createMock(ContainerInterface::class); - $container->method('has')->willReturn(false); - return $container; + return CmdbTestSupport::emptyContainer(test: $this); }//end emptyContainer() /** diff --git a/tests/Unit/Repair/SeedCmdbImportPacksTest.php b/tests/Unit/Repair/SeedCmdbImportPacksTest.php index 0657f34cc..a9add8c7f 100644 --- a/tests/Unit/Repair/SeedCmdbImportPacksTest.php +++ b/tests/Unit/Repair/SeedCmdbImportPacksTest.php @@ -163,8 +163,7 @@ public function testUneditedPacksAreUpdatedAndEditedOnesKept(): void { * @return void */ public function testWithoutTheStoreNothingIsSeeded(): void { - $container = $this->createMock(ContainerInterface::class); - $container->method('has')->willReturn(false); + $container = CmdbTestSupport::emptyContainer(test: $this); $step = new SeedCmdbImportPacks( profile: new CmdbImportProfile(container: $container), store: new CmdbImportPackStore(container: $container), diff --git a/tests/Unit/Service/Cmdb/CmdbImportProfileTest.php b/tests/Unit/Service/Cmdb/CmdbImportProfileTest.php index 1cc8eec83..2b0380180 100644 --- a/tests/Unit/Service/Cmdb/CmdbImportProfileTest.php +++ b/tests/Unit/Service/Cmdb/CmdbImportProfileTest.php @@ -42,9 +42,7 @@ class CmdbImportProfileTest extends TestCase { * @return ContainerInterface */ private function emptyContainer(): ContainerInterface { - $container = $this->createMock(ContainerInterface::class); - $container->method('has')->willReturn(false); - return $container; + return CmdbTestSupport::emptyContainer(test: $this); }//end emptyContainer() /** @@ -702,8 +700,9 @@ public function testWithoutTheStoreBeforeASeedTheShippedFilesAreUsed(): void { $config = []; $container = $this->createMock(ContainerInterface::class); $appConfig = FakeMigrationPackService::appConfig(test: $this, values: $config); - $container->method('has')->willReturnCallback(fn (string $id): bool => $id === \OCP\IAppConfig::class); - $container->method('get')->willReturn($appConfig); + $factory = CmdbTestSupport::translationFactory(test: $this); + $container->method('has')->willReturnCallback(fn (string $id): bool => $id === \OCP\IAppConfig::class || $id === \OCP\L10N\IFactory::class); + $container->method('get')->willReturnCallback(fn (string $id): object => $id === \OCP\L10N\IFactory::class ? $factory : $appConfig); $profile = new CmdbImportProfile(container: $container); $overview = $profile->mappingOverview(); @@ -726,8 +725,9 @@ public function testWithoutTheStoreAfterASeedTheImportFailsClosed(): void { $config = [CmdbImportPackStore::CONFIG_KEY => (string)json_encode(['stackiq-topdesk-module' => 41, 'stackiq-topdesk-usage' => 43])]; $container = $this->createMock(ContainerInterface::class); $appConfig = FakeMigrationPackService::appConfig(test: $this, values: $config); - $container->method('has')->willReturnCallback(fn (string $id): bool => $id === \OCP\IAppConfig::class); - $container->method('get')->willReturn($appConfig); + $factory = CmdbTestSupport::translationFactory(test: $this); + $container->method('has')->willReturnCallback(fn (string $id): bool => $id === \OCP\IAppConfig::class || $id === \OCP\L10N\IFactory::class); + $container->method('get')->willReturnCallback(fn (string $id): object => $id === \OCP\L10N\IFactory::class ? $factory : $appConfig); try { (new CmdbImportProfile(container: $container))->load(); diff --git a/tests/Unit/Service/Cmdb/CmdbWorkbookReaderTest.php b/tests/Unit/Service/Cmdb/CmdbWorkbookReaderTest.php index 39554e1c1..bcf147876 100644 --- a/tests/Unit/Service/Cmdb/CmdbWorkbookReaderTest.php +++ b/tests/Unit/Service/Cmdb/CmdbWorkbookReaderTest.php @@ -43,8 +43,7 @@ class CmdbWorkbookReaderTest extends TestCase { */ private function profile(?string $directory = null): CmdbImportProfile { CmdbTestSupport::loadMigrationPack(); - $container = $this->createMock(ContainerInterface::class); - $container->method('has')->willReturn(false); + $container = CmdbTestSupport::emptyContainer(test: $this); $profile = new CmdbImportProfile(container: $container, directory: $directory); $profile->load(); diff --git a/tests/Unit/Service/Cmdb/OpenRegisterInstanceTest.php b/tests/Unit/Service/Cmdb/OpenRegisterInstanceTest.php index 0b33efc96..ff7a6c857 100644 --- a/tests/Unit/Service/Cmdb/OpenRegisterInstanceTest.php +++ b/tests/Unit/Service/Cmdb/OpenRegisterInstanceTest.php @@ -22,6 +22,7 @@ use OCA\Stackiq\Service\Cmdb\OpenRegisterInstance; use OCP\IL10N; +use OCP\L10N\IFactory; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\TestCase; use Psr\Container\ContainerInterface; @@ -31,15 +32,6 @@ */ #[CoversClass(OpenRegisterInstance::class)] class OpenRegisterInstanceTest extends TestCase { - /** - * Reset the translations override. - * - * @return void - */ - protected function tearDown(): void { - OpenRegisterInstance::$l10nOverride = null; - }//end tearDown() - /** * A class without constructor parameters (older OpenRegister) is built as is. * @@ -58,20 +50,23 @@ public function testAClassWithoutParametersIsBuilt(): void { */ public function testAClassRequiringTranslationsGetsThem(): void { $l10n = $this->createMock(IL10N::class); - OpenRegisterInstance::$l10nOverride = static fn (): IL10N => $l10n; + $factory = $this->createMock(IFactory::class); + $factory->expects($this->once())->method('get')->with('openregister')->willReturn($l10n); + $container = $this->createMock(ContainerInterface::class); + $container->method('get')->with(IFactory::class)->willReturn($factory); $class = new class($l10n) { public function __construct(public readonly IL10N $l10n) { } }; - $object = (new OpenRegisterInstance(container: $this->createMock(ContainerInterface::class)))->build(class: $class::class); + $object = (new OpenRegisterInstance(container: $container))->build(class: $class::class); $this->assertInstanceOf($class::class, $object); $this->assertSame($l10n, $object->l10n); }//end testAClassRequiringTranslationsGetsThem() /** - * A constructor needing anything else, or translations that cannot be resolved, gives null. + * A constructor needing anything else, or a container without the translation factory, gives null. * * @return void */ @@ -83,13 +78,12 @@ public function __construct(public readonly string $value) { }; $this->assertNull((new OpenRegisterInstance(container: $container))->build(class: $other::class)); - OpenRegisterInstance::$l10nOverride = static function (): IL10N { - throw new \RuntimeException('no factory'); - }; + $noFactory = $this->createMock(ContainerInterface::class); + $noFactory->method('get')->willThrowException(new \RuntimeException('not in this container')); $needsL10n = new class($this->createMock(IL10N::class)) { public function __construct(public readonly IL10N $l10n) { } }; - $this->assertNull((new OpenRegisterInstance(container: $container))->build(class: $needsL10n::class)); + $this->assertNull((new OpenRegisterInstance(container: $noFactory))->build(class: $needsL10n::class)); }//end testAnUnbuildableClassGivesNull() }//end class diff --git a/tests/Unit/Service/CmdbExportImportServiceTest.php b/tests/Unit/Service/CmdbExportImportServiceTest.php index 5e7bf0973..93ee15064 100644 --- a/tests/Unit/Service/CmdbExportImportServiceTest.php +++ b/tests/Unit/Service/CmdbExportImportServiceTest.php @@ -1075,9 +1075,13 @@ private function service( [$archiveHandler, $objectMapper] = $this->archiveDoubles(); $container = $this->createMock(ContainerInterface::class); - $container->method('has')->willReturnCallback(fn (string $id): bool => $packs !== false && $id === CmdbImportPackStore::SERVICE_CLASS); + $container->method('has')->willReturnCallback(fn (string $id): bool => $id === \OCP\L10N\IFactory::class || ($packs !== false && $id === CmdbImportPackStore::SERVICE_CLASS)); $container->method('get')->willReturnCallback( function (string $id) use ($objectService, $schemaMapper, $packs, $archiveHandler, $objectMapper) { + if ($id === \OCP\L10N\IFactory::class) { + return CmdbTestSupport::translationFactory(test: $this); + } + if ($id === ObjectServiceInterface::class) { return $objectService; } diff --git a/tests/Unit/Support/CmdbTestSupport.php b/tests/Unit/Support/CmdbTestSupport.php index e3db45701..f2c899a38 100644 --- a/tests/Unit/Support/CmdbTestSupport.php +++ b/tests/Unit/Support/CmdbTestSupport.php @@ -30,6 +30,10 @@ namespace OCA\Stackiq\Tests\Unit\Support; +use OCP\L10N\IFactory; +use PHPUnit\Framework\TestCase; +use Psr\Container\ContainerInterface; + /** * Locates and loads the OpenRegister pieces the CMDB import uses. */ @@ -102,7 +106,6 @@ public static function loadMigrationPack(): string { $base = $dir . '/lib/Service/MigrationPack'; } - \OCA\Stackiq\Service\Cmdb\OpenRegisterInstance::$l10nOverride = static fn (): \OCP\IL10N => self::l10n(); foreach (['PackDefinitionValidator', 'MappingEngine'] as $class) { if (class_exists('OCA\\OpenRegister\\Service\\MigrationPack\\' . $class, false) === false) { require_once $base . '/' . $class . '.php'; @@ -118,7 +121,7 @@ public static function loadMigrationPack(): string { * * @return \OCP\IL10N */ - private static function l10n(): \OCP\IL10N { + public static function l10n(): \OCP\IL10N { return new class implements \OCP\IL10N { public function t(string $text, $parameters = []): string { return vsprintf($text, (array)$parameters); @@ -142,6 +145,45 @@ public function getLocaleCode(): string { }; }//end l10n() + /** + * A container that knows only the translation factory, so a pack class comes from class_exists. + * + * @param TestCase $test The running test. + * + * @return ContainerInterface + */ + public static function emptyContainer(TestCase $test): ContainerInterface { + $builder = new \ReflectionMethod(TestCase::class, 'createMock'); + $container = $builder->invoke($test, ContainerInterface::class); + $container->method('has')->willReturnCallback(static fn (string $id): bool => $id === IFactory::class); + $container->method('get')->willReturnCallback( + static function (string $id) use ($test) { + if ($id !== IFactory::class) { + throw new \RuntimeException('not in this container: ' . $id); + } + + return self::translationFactory(test: $test); + } + ); + + return $container; + }//end emptyContainer() + + /** + * A translation factory that hands out the pass-through IL10N. + * + * @param TestCase $test The running test. + * + * @return IFactory + */ + public static function translationFactory(TestCase $test): IFactory { + $builder = new \ReflectionMethod(TestCase::class, 'createMock'); + $factory = $builder->invoke($test, IFactory::class); + $factory->method('get')->willReturn(self::l10n()); + + return $factory; + }//end translationFactory() + /** * An OpenRegister pack class, built with a pass-through IL10N when its constructor requires one. * diff --git a/tests/Unit/Support/FakeMigrationPackService.php b/tests/Unit/Support/FakeMigrationPackService.php index c79735eb9..5f7abdbdc 100644 --- a/tests/Unit/Support/FakeMigrationPackService.php +++ b/tests/Unit/Support/FakeMigrationPackService.php @@ -259,7 +259,7 @@ public function update(int $id, array $definition, ?int $expectedUpdatedAt = nul public function container(TestCase $test, ?IAppConfig $appConfig = null): ContainerInterface { $builder = new \ReflectionMethod(TestCase::class, 'createMock'); $container = $builder->invoke($test, ContainerInterface::class); - $known = ['OCA\OpenRegister\Service\MigrationPackService' => $this]; + $known = ['OCA\OpenRegister\Service\MigrationPackService' => $this, \OCP\L10N\IFactory::class => CmdbTestSupport::translationFactory(test: $test)]; if ($appConfig !== null) { $known[IAppConfig::class] = $appConfig; }