From 3bf2f5a4266448c2846eef33816626a4e570ed8d Mon Sep 17 00:00:00 2001 From: Conduction Release Bot Date: Sat, 22 Aug 2026 02:09:13 +0200 Subject: [PATCH 1/2] =?UTF-8?q?chore(quality):=20migrate=20to=20PHPStan=20?= =?UTF-8?q?2=20=E2=80=94=2035=20findings=20to=20zero,=20plus=20a=20real=20?= =?UTF-8?q?bug?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bumps `phpstan/phpstan` to ^2.0 and `conduction/hydra-gates` to ^1.8.2, and clears every finding the new major surfaces. ## A missing manager was never detected ContactPersonHandler::setUserManager(): $user = $this->_userManager->get($username); $manager = $this->_userManager->get($managerUsername); if ($user === null || $manager === false) { ...warn and return... } IUserManager::get() returns `?IUser` — it signals "no such user" with NULL and never returns false. So the second half of that guard could never fire: calling setUserManager() with a manager username that does not exist skipped the warning entirely and carried on as if the manager were real. Fixed to `=== null`. PHPStan found it from the other end: inside that branch `$user !== null` was reported as always FALSE, because the only reachable way in was the first clause. ## Dead guards (28) Mostly in the ArchiMate import/export pair, which are near-copies of each other, so nearly every finding came in twos: - `self::PERFORMANCE_OPTIMIZATIONS['memory_cleanup'] !== false` and `['parallel_processing'] === true` — both are class constants set to true, so neither was ever conditional. - `$identifier !== false`, `$versionId !== false`, `$refCompId !== false`, `$amefKey !== false` — all strings; none can be false. - `is_array($sectionData) === false` — the parameter is declared `array`, so PHP rejects anything else at the call boundary first. - `isset($statistics[$sectionKey]) === false` — the branch above pins $sectionKey to a key $statistics always has. - `if ($section !== 'omschrijving')` with the comment "Skip summary section itself" — `omschrijving` is assigned to $statistics on the line AFTER the loop, so the loop can never see it. - `method_exists($this->archiMateService, '...Optimized')` — the method is declared on the class. - Several `isset() && !== null` pairs and non-nullable-entity null tests. ## One scoped ignore OrganizationSyncService's `if ($contactObject !== null)` is provably true — the code a few lines above already dereferences $contactObject unconditionally. It is left in place because the block it wraps is 243 lines: removing the `if` is a pure re-indentation of a quarter of the method, a large review-hostile diff for zero behaviour change. The comment says so and marks it for the next real edit to that method. ## Verification phpstan 0, phpcs clean, phpmd clean. PHPUnit is NOT part of this evidence: the bootstrap requires a booted Nextcloud (`Class "OC_App" not found`) and cannot run standalone. Checked that this is pre-existing by stashing every change in this commit and re-running — byte-identical failure. CI runs the suite inside the container. --- composer.json | 4 +- composer.lock | 23 +++++-- .../OrganisationMembersController.php | 4 +- lib/Controller/SettingsController.php | 8 ++- lib/Service/ArchiMateExportService.php | 2 +- lib/Service/ArchiMateImportService.php | 65 +++++++++---------- lib/Service/ArchiMateService.php | 45 ++++++------- lib/Service/OrganisatieService.php | 18 ++--- lib/Service/OrganizationSyncService.php | 4 ++ lib/Service/SettingsService.php | 19 ++---- .../ContactPersonHandler.php | 6 +- .../SoftwareCatalogue/OrganizationHandler.php | 47 +++++++------- lib/Service/SoftwareCatalogueService.php | 3 +- phpstan.neon | 16 +++++ 14 files changed, 147 insertions(+), 117 deletions(-) diff --git a/composer.json b/composer.json index 289381fa..880aa7be 100644 --- a/composer.json +++ b/composer.json @@ -80,7 +80,7 @@ }, "require-dev": { "conduction/coding-standard": "^1.0", - "conduction/hydra-gates": "^1.0", + "conduction/hydra-gates": "^1.8.2", "cyclonedx/cyclonedx-php-composer": "^6.2", "edgedesign/phpqa": "^1.27", "guzzlehttp/guzzle": "^7.8", @@ -88,7 +88,7 @@ "phpcsstandards/phpcsextra": "^1.4", "phpmd/phpmd": "^2.15", "phpmetrics/phpmetrics": "^2.8", - "phpstan/phpstan": "^1.10", + "phpstan/phpstan": "^2.0", "phpunit/phpunit": "^10.5", "roave/security-advisories": "dev-latest", "squizlabs/php_codesniffer": "^3.9", diff --git a/composer.lock b/composer.lock index d38884fd..9bfbbdf7 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "b1b59434167398888c8cfc94e8cf1f7d", + "content-hash": "8ec5b262f02d9a521ddced66fe163f6a", "packages": [ { "name": "adbario/php-dot-notation", @@ -5836,15 +5836,15 @@ }, { "name": "phpstan/phpstan", - "version": "1.12.33", + "version": "2.2.8", "dist": { "type": "zip", - "url": "https://api.github.com/repos/phpstan/phpstan/zipball/37982d6fc7cbb746dda7773530cda557cdf119e1", - "reference": "37982d6fc7cbb746dda7773530cda557cdf119e1", + "url": "https://api.github.com/repos/phpstan/phpstan/zipball/e285254e60f33c21902efef4a926ca0987c06804", + "reference": "e285254e60f33c21902efef4a926ca0987c06804", "shasum": "" }, "require": { - "php": "^7.2|^8.0" + "php": "^7.4|^8.0" }, "conflict": { "phpstan/phpstan-shim": "*" @@ -5863,6 +5863,17 @@ "license": [ "MIT" ], + "authors": [ + { + "name": "Ondřej Mirtes" + }, + { + "name": "Markus Staab" + }, + { + "name": "Vincent Langlet" + } + ], "description": "PHPStan - PHP Static Analysis Tool", "keywords": [ "dev", @@ -5885,7 +5896,7 @@ "type": "github" } ], - "time": "2026-02-28T20:30:03+00:00" + "time": "2026-08-04T22:21:45+00:00" }, { "name": "phpunit/php-code-coverage", diff --git a/lib/Controller/OrganisationMembersController.php b/lib/Controller/OrganisationMembersController.php index 92fa3893..cc752432 100644 --- a/lib/Controller/OrganisationMembersController.php +++ b/lib/Controller/OrganisationMembersController.php @@ -261,7 +261,9 @@ private function authorizeMaintainer(string $organisationUuid): ?JSONResponse { * * @return \OCA\OpenRegister\Service\OrganisationService The service instance. * - * @throws \Throwable When OpenRegister is unavailable. + * No `@throws`: the body is a plain property read. If OpenRegister is + * unavailable the failure happens in the container while CONSTRUCTING this + * controller, not here. */ private function getOrganisationService(): \OCA\OpenRegister\Service\OrganisationService { return $this->organisationService; diff --git a/lib/Controller/SettingsController.php b/lib/Controller/SettingsController.php index e3eb3e89..5bc0f6af 100644 --- a/lib/Controller/SettingsController.php +++ b/lib/Controller/SettingsController.php @@ -264,7 +264,7 @@ public function index(): JSONResponse { try { $user = $this->userSession->getUser(); - $isAdmin = $user !== null && $this->groupManager->isAdmin($user->getUID()); + $isAdmin = $this->groupManager->isAdmin($user->getUID()); // Delegate all business logic to service. $data = $this->settingsService->getAllSettings(); @@ -1577,10 +1577,12 @@ private function parseArchiMateFileUpload(): ?array { * @spec openspec/changes/method-decomposition/tasks.md#task-3 */ private function resolveArchiMateMethod(array $options): array { + // No method_exists() probe: ArchiMateService declares + // importArchiMateFileFromPathOptimized(), so only the request parameter + // decides which path runs. $useOptimized = $this->request->getParam('useOptimized', 'true') === 'true'; - $hasOptimized = method_exists($this->archiMateService, 'importArchiMateFileFromPathOptimized'); - if ($useOptimized === true && $hasOptimized === true) { + if ($useOptimized === true) { $this->logger->info('Using OPTIMIZED ArchiMate import method.'); return $this->archiMateService->importArchiMateFileFromPathOptimized($options); } diff --git a/lib/Service/ArchiMateExportService.php b/lib/Service/ArchiMateExportService.php index a061ec4b..38d2be00 100644 --- a/lib/Service/ArchiMateExportService.php +++ b/lib/Service/ArchiMateExportService.php @@ -1209,7 +1209,7 @@ private function addObjectDirectlyToXmlWithProperties( $xmlData = $this->cleanObjectDataForXml(object: $object, propDefMap: $propertyDefinitionMap); } - if (is_array($xmlData) === true && empty($xmlData) === false) { + if (empty($xmlData) === false) { if ($sectionName === 'views') { $this->addViewDataToXmlNode(viewNode: $objectNode, viewData: $xmlData); } else { diff --git a/lib/Service/ArchiMateImportService.php b/lib/Service/ArchiMateImportService.php index c7d5fb88..d5e41407 100644 --- a/lib/Service/ArchiMateImportService.php +++ b/lib/Service/ArchiMateImportService.php @@ -368,11 +368,11 @@ public function importArchiMateFileFromPathOptimized(array $options = []): array // PERFORMANCE OPTIMIZATION: Clean up memory after XML parsing. $memoryCleanupTime = 0; - if (self::PERFORMANCE_OPTIMIZATIONS['memory_cleanup'] !== false) { - $memCleanupStart = microtime(true); - $this->cleanupMemory(); - $memoryCleanupTime = microtime(true) - $memCleanupStart; - } + // PERFORMANCE_OPTIMIZATIONS['memory_cleanup'] is a class constant set + // to true, so this was never conditional. + $memCleanupStart = microtime(true); + $this->cleanupMemory(); + $memoryCleanupTime = microtime(true) - $memCleanupStart; // STEP 2: Extract model identifier. $modelIdStartTime = microtime(true); @@ -1254,7 +1254,7 @@ private function createSectionObject(string $section, string $identifier, array // Fallback: Use AMEF identifier as both ID and extract clean UUID for slug. $objectId = $identifier; // Extract clean UUID from AMEF identifier (remove "id-" prefix if present). - if ($identifier !== false && str_starts_with($identifier, 'id-') === true) { + if (str_starts_with($identifier, 'id-') === true) { $slug = substr($identifier, 3); // Remove "id-" prefix. } else { @@ -1735,9 +1735,10 @@ private function saveObjectsInParallelBatches(array $objects, ObjectServiceInter }//end try // Memory cleanup between chunks. - if (self::PERFORMANCE_OPTIMIZATIONS['memory_cleanup'] !== false) { - $this->cleanupMemory(); - } + // PERFORMANCE_OPTIMIZATIONS['memory_cleanup'] is a class constant set + // to true, so this was never conditional. Flip the constant and the + // compiler will point you back here. + $this->cleanupMemory(); }//end foreach // Store the aggregated result for statistics calculation. @@ -2047,7 +2048,7 @@ private function getAmefRegisterId(): ?int { } // Validate and normalize to positive int. - if ($rawRegisterId !== null && $rawRegisterId !== '' && is_numeric((string)$rawRegisterId) === true) { + if ($rawRegisterId !== '' && is_numeric((string)$rawRegisterId) === true) { $registerId = (int)$rawRegisterId; if ($registerId > 0) { return $registerId; @@ -2549,10 +2550,8 @@ private function findItemsInSection(array $sectionData, string $sectionName): ar // OPTIMIZATION: Removed debug logging from section processing. $items = []; - // Safety check: ensure sectionData is an array. - if (is_array($sectionData) === false) { - return []; - } + // No is_array() safety check: $sectionData is declared array, so PHP + // rejects anything else at the call boundary before this could run. // Get section structure configuration from AMEF config. $config = $this->getSectionStructureConfig(sectionName: $sectionName); @@ -4033,7 +4032,7 @@ private function processStandardVersionRelationship( $standardId = $source; } - if ($versionId !== false && $standardId === true) { + if ($standardId === true) { $stdVersionRelMap[$versionId] = $standardId; } }//end processStandaardVersieRelationship() @@ -4084,7 +4083,7 @@ private function processRelationshipImmediate( $standardId = $source; } - if ($refCompId !== false && $standardId === true) { + if ($standardId === true) { // Initialize arrays if not exists. if (isset($gemmaRelationshipMap[$refCompId]) === false) { $gemmaRelationshipMap[$refCompId] = [ @@ -4800,7 +4799,7 @@ private function transformSectionObjectsBatch( // AMEF identifier becomes slug. } else { // Fallback: extract clean UUID from AMEF identifier for slug. - if ($identifier !== false && str_starts_with($identifier, 'id-') === true) { + if (str_starts_with($identifier, 'id-') === true) { $object['@self']['slug'] = substr($identifier, 3); // Remove "id-" prefix. } else { @@ -4809,7 +4808,7 @@ private function transformSectionObjectsBatch( } } else { // No properties to flatten, use AMEF identifier logic. - if ($identifier !== false && str_starts_with($identifier, 'id-') === true) { + if (str_starts_with($identifier, 'id-') === true) { $object['@self']['slug'] = substr($identifier, 3); // Remove "id-" prefix. } else { @@ -4988,7 +4987,9 @@ private function flattenPropertiesBatch(array &$object, array $properties, array continue; } - if ($value !== null && isset($propDefMap[$defRef]) === true) { + // No isset($propDefMap[$defRef]) re-check: the loop above only + // reaches here for a $defRef the map already has. + if ($value !== null) { $propertyName = $propDefMap[$defRef]; $camelCaseName = $this->convertToCamelCase(propertyName: $propertyName); $object[$camelCaseName] = $value; @@ -5022,13 +5023,14 @@ private function flattenPropertiesBatch(array &$object, array $properties, array ); } } else { + // 'mapping_exists' is always true here — the map lookup already + // succeeded, so a null $value is the only way into this branch. $this->logger->warning( - 'Property value is null or mapping missing', + 'Property value is null', [ 'object_id' => $object['identifier'] ?? 'unknown', 'property_def_ref' => $defRef, 'value' => $value, - 'mapping_exists' => isset($propDefMap[$defRef]) === true, ] ); }//end if @@ -5720,10 +5722,8 @@ private function calculateObjectStatistics(array $normalizedData): array { $sectionKey = 'elements'; }//end if - if (isset($statistics[$sectionKey]) === false) { - continue; - // Skip unknown section types. - } + // No "skip unknown section types" guard: the branch above pins + // $sectionKey to a key $statistics always has, so it never fired. // Determine if this object was created, updated, or had errors. $objectId = $object['@self']['id'] ?? $object['identifier'] ?? null; @@ -5809,14 +5809,13 @@ private function calculateObjectStatistics(array $normalizedData): array { 'total_errors' => 0, ]; - foreach ($statistics as $section => $sectionStats) { - if ($section !== 'omschrijving') { - // Skip summary section itself. - $summary['total_objects_created'] += $sectionStats['created']; - $summary['total_objects_updated'] += $sectionStats['updated']; - $summary['total_objects_unchanged'] += $sectionStats['unchanged']; - $summary['total_errors'] += count($sectionStats['errors']); - } + // No "skip the summary section" guard: `omschrijving` is written into + // $statistics on the line AFTER this loop, so the loop can never see it. + foreach ($statistics as $sectionStats) { + $summary['total_objects_created'] += $sectionStats['created']; + $summary['total_objects_updated'] += $sectionStats['updated']; + $summary['total_objects_unchanged'] += $sectionStats['unchanged']; + $summary['total_errors'] += count($sectionStats['errors']); } $statistics['omschrijving'] = $summary; diff --git a/lib/Service/ArchiMateService.php b/lib/Service/ArchiMateService.php index 1a123163..d9376a5d 100644 --- a/lib/Service/ArchiMateService.php +++ b/lib/Service/ArchiMateService.php @@ -642,10 +642,8 @@ private function findItemsInSection(array $sectionData, string $sectionName): ar // OPTIMIZATION: Removed debug logging from section processing. $items = []; - // Safety check: ensure sectionData is an array. - if (is_array($sectionData) === false) { - return []; - } + // No is_array() safety check: $sectionData is declared array, so PHP + // rejects anything else at the call boundary before this could run. // Get section structure configuration from AMEF config. $config = $this->getSectionStructureConfig(sectionName: $sectionName); @@ -974,7 +972,7 @@ private function createSectionObject(string $section, string $identifier, array } elseif (isset($data['Object ID']) === true) { // Check if we have "Object ID" property directly. $slug = $data['Object ID']; - } elseif ($identifier !== false && str_starts_with($identifier, 'id-') === true) { + } elseif (str_starts_with($identifier, 'id-') === true) { // Fallback: extract from identifier (remove "id-" prefix if present). $slug = substr($identifier, 3); } @@ -1027,9 +1025,9 @@ private function saveObjectsToDatabase(array $objects): array { // PERFORMANCE OPTIMIZATION: Use parallel batch processing for large datasets. $batchProcessingStartTime = microtime(true); - if (self::PERFORMANCE_OPTIMIZATIONS['parallel_processing'] === true - && count($objects) > self::PERFORMANCE_OPTIMIZATIONS['batch_size'] - ) { + // PERFORMANCE_OPTIMIZATIONS['parallel_processing'] is a class constant set + // to true, so only the batch-size threshold decides this. + if (count($objects) > self::PERFORMANCE_OPTIMIZATIONS['batch_size']) { $result = $this->saveObjectsInParallelBatches( objects: $objects, objectService: $objectService, @@ -1169,9 +1167,9 @@ private function saveObjectsInParallelBatches(array $objects, ObjectServiceInter }//end try // Memory cleanup between chunks. - if (self::PERFORMANCE_OPTIMIZATIONS['memory_cleanup'] !== false) { - $this->cleanupMemory(); - } + // PERFORMANCE_OPTIMIZATIONS['memory_cleanup'] is a class constant set + // to true, so this was never conditional. + $this->cleanupMemory(); }//end foreach // Store the aggregated result for statistics calculation. @@ -1767,7 +1765,7 @@ private function getAmefRegisterId(): ?int { } // Validate and normalize to positive int. - if ($rawRegisterId !== null && $rawRegisterId !== '' && is_numeric((string)$rawRegisterId) === true) { + if ($rawRegisterId !== '' && is_numeric((string)$rawRegisterId) === true) { $registerId = (int)$rawRegisterId; if ($registerId > 0) { return $registerId; @@ -2258,10 +2256,8 @@ private function calculateObjectStatistics(array $normalizedData): array { // Default fallback. }; - if (isset($statistics[$sectionKey]) === false) { - continue; - // Skip unknown section types. - } + // No "skip unknown section types" guard: the branch above pins + // $sectionKey to a key $statistics always has, so it never fired. // Determine if this object was created, updated, or had errors. $objectId = $object['@self']['id'] ?? $object['identifier'] ?? null; @@ -2346,14 +2342,13 @@ private function calculateObjectStatistics(array $normalizedData): array { 'total_errors' => 0, ]; - foreach ($statistics as $section => $sectionStats) { - if ($section !== 'omschrijving') { - // Skip summary section itself. - $summary['total_objects_created'] += $sectionStats['created']; - $summary['total_objects_updated'] += $sectionStats['updated']; - $summary['total_objects_skipped'] += $sectionStats['skipped']; - $summary['total_errors'] += count($sectionStats['errors']); - } + // No "skip the summary section" guard: `omschrijving` is written into + // $statistics on the line AFTER this loop, so the loop can never see it. + foreach ($statistics as $sectionStats) { + $summary['total_objects_created'] += $sectionStats['created']; + $summary['total_objects_updated'] += $sectionStats['updated']; + $summary['total_objects_skipped'] += $sectionStats['skipped']; + $summary['total_errors'] += count($sectionStats['errors']); } $statistics['omschrijving'] = $summary; @@ -3061,7 +3056,7 @@ private function processRelationshipImmediate( $standardId = $source; } - if ($refCompId !== false && $standardId === true) { + if ($standardId === true) { // Initialize arrays if not exists. if (isset($gemmaRelationshipMap[$refCompId]) === false) { $gemmaRelationshipMap[$refCompId] = [ diff --git a/lib/Service/OrganisatieService.php b/lib/Service/OrganisatieService.php index 8dc86c08..f5010b4a 100644 --- a/lib/Service/OrganisatieService.php +++ b/lib/Service/OrganisatieService.php @@ -106,15 +106,15 @@ public function createOrganisationInOpenRegister(array $objectData): ?object { organizationUuid: $organizationUuid ); - if ($organisationEntity !== null) { - $this->logger->info( - 'OrganisatieService: Successfully created organization entity', - [ - 'organizationUuid' => $organizationUuid, - 'entityId' => $organisationEntity->getId(), - ] - ); - } + // createOrganisationEntityInternal() is declared non-nullable and + // throws on failure — the catch below is the real failure path. + $this->logger->info( + 'OrganisatieService: Successfully created organization entity', + [ + 'organizationUuid' => $organizationUuid, + 'entityId' => $organisationEntity->getId(), + ] + ); return $organisationEntity; } catch (\Exception $e) { diff --git a/lib/Service/OrganizationSyncService.php b/lib/Service/OrganizationSyncService.php index 9b510cfa..7f7cdc83 100644 --- a/lib/Service/OrganizationSyncService.php +++ b/lib/Service/OrganizationSyncService.php @@ -1404,6 +1404,10 @@ private function processContactPerson(object $contactPerson, array &$stats): ?st private function updateOrganisationEntityUsers(object $organisationEntity, array $usernames, array &$stats): void { try { $organisationUuid = $organisationEntity->getUuid(); + // OpenRegister is not on the analysis path, so getUsers() has no + // resolvable return type and sort() below cannot be checked without + // this. It is a list of usernames. + /** @var array $currentUsers */ $currentUsers = ($organisationEntity->getUsers() ?? []); // Add admin users to ensure they're always included. diff --git a/lib/Service/SettingsService.php b/lib/Service/SettingsService.php index b08f7d58..d354a501 100644 --- a/lib/Service/SettingsService.php +++ b/lib/Service/SettingsService.php @@ -872,7 +872,7 @@ public function getSchemaIdForObjectType(string $objectType): ?int { $amefKey = $amefKeyMap[$objectType] ?? null; - if ($amefKey !== false && isset($decodedAmefConfig[$amefKey]) === true) { + if (isset($decodedAmefConfig[$amefKey]) === true) { $schemaId = $decodedAmefConfig[$amefKey]; if (empty($schemaId) === false) { $result = (int)$schemaId; @@ -920,9 +920,7 @@ public function getSchemaIdForObjectType(string $objectType): ?int { // Only check voorzieningen config if object type exists in the key map. if ($result === null && isset($voorzieningenKeyMap[$objectType]) === true) { $voorzieningenKey = $voorzieningenKeyMap[$objectType]; - if (isset($voorzieningenConfig[$voorzieningenKey]) === true - && $voorzieningenConfig[$voorzieningenKey] !== null - ) { + if (isset($voorzieningenConfig[$voorzieningenKey]) === true) { $result = (int)$voorzieningenConfig[$voorzieningenKey]; } } @@ -5997,11 +5995,10 @@ public function getEmailConfigFocused(): array { */ public function updateEmailConfig(array $config): array { try { - if (isset($config) === true) { - $result = $this->updateEmailSettings(emailSettings: $config); - if ($result['success'] === false) { - return $result; - } + // No isset($config) guard: it is a required, non-nullable parameter. + $result = $this->updateEmailSettings(emailSettings: $config); + if ($result['success'] === false) { + return $result; } return [ @@ -6070,9 +6067,7 @@ public function updateAmefConfig(array $config): array { // Load existing config to allow merging. $existing = $this->getAmefConfig(); - if (is_array($existing) === false) { - $existing = []; - } + // No is_array() fallback: getAmefConfig() is declared to return array. // Determine target register id. if (isset($config['register']) === true) { diff --git a/lib/Service/SoftwareCatalogue/ContactPersonHandler.php b/lib/Service/SoftwareCatalogue/ContactPersonHandler.php index 095f84a1..cae6758a 100644 --- a/lib/Service/SoftwareCatalogue/ContactPersonHandler.php +++ b/lib/Service/SoftwareCatalogue/ContactPersonHandler.php @@ -1454,7 +1454,11 @@ public function setUserManager(string $username, string $managerUsername): void $user = $this->_userManager->get($username); $manager = $this->_userManager->get($managerUsername); - if ($user === null || $manager === false) { + // `$manager === null`, not `=== false`: IUserManager::get() returns + // ?IUser and signals "no such user" with null. Comparing against + // false meant a MISSING MANAGER was never detected — the guard fell + // through and the method carried on as if the manager existed. + if ($user === null || $manager === null) { $this->_logger->warning( 'Cannot set manager - user or manager not found', [ diff --git a/lib/Service/SoftwareCatalogue/OrganizationHandler.php b/lib/Service/SoftwareCatalogue/OrganizationHandler.php index 747e6a4b..25533b53 100644 --- a/lib/Service/SoftwareCatalogue/OrganizationHandler.php +++ b/lib/Service/SoftwareCatalogue/OrganizationHandler.php @@ -437,30 +437,31 @@ public function processContactpersonen(object $organizationObject): array { ); } - if ($contactgegevensObject !== null) { - $processedContacts[] = $contactgegevensObject; - - $actionLogMessage = 'Created new contactgegevens from contactpersoon'; - $actionValue = 'create'; - if ($existingContactgegevens !== null) { - $actionLogMessage = 'Updated existing contactgegevens from contactpersoon'; - $actionValue = 'update'; - } + // No null guard: saveObject() returns a non-nullable + // ObjectEntityInterface and throws on failure, which the + // catch below handles. + $processedContacts[] = $contactgegevensObject; - $this->_logger->info( - $actionLogMessage, - [ - 'organizationId' => $organizationUuid, - // UUID, not getId(): `getId()` is not on - // ObjectEntityInterface (ADR-084), and the UUID is - // the identifier every other log line here carries. - 'contactgegevensId' => $contactgegevensObject->getUuid(), - 'contactpersoonIndex' => $index, - 'email' => $contactgegevensData['email'], - 'action' => $actionValue, - ] - ); - }//end if + $actionLogMessage = 'Created new contactgegevens from contactpersoon'; + $actionValue = 'create'; + if ($existingContactgegevens !== null) { + $actionLogMessage = 'Updated existing contactgegevens from contactpersoon'; + $actionValue = 'update'; + } + + $this->_logger->info( + $actionLogMessage, + [ + 'organizationId' => $organizationUuid, + // UUID, not getId(): `getId()` is not on + // ObjectEntityInterface (ADR-084), and the UUID is + // the identifier every other log line here carries. + 'contactgegevensId' => $contactgegevensObject->getUuid(), + 'contactpersoonIndex' => $index, + 'email' => $contactgegevensData['email'], + 'action' => $actionValue, + ] + ); } catch (\Exception $e) { $this->_logger->error( 'Failed to process contactPerson: ' . $e->getMessage(), diff --git a/lib/Service/SoftwareCatalogueService.php b/lib/Service/SoftwareCatalogueService.php index 45c25c4a..307d78f2 100644 --- a/lib/Service/SoftwareCatalogueService.php +++ b/lib/Service/SoftwareCatalogueService.php @@ -1526,7 +1526,8 @@ private function createOrganisationInOpenRegisterInternal( $this->_logger->info( 'SoftwareCatalogueService: STEP 2 - Checking user context', [ - 'hasUserSession' => $userSession !== null, + // Always true: $userSession is an injected, non-nullable IUserSession. + 'hasUserSession' => true, 'currentUser' => $currentUserValue, 'isAnonymous' => $currentUser === null, ] diff --git a/phpstan.neon b/phpstan.neon index a805d1e8..ecc4397b 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -22,3 +22,19 @@ parameters: # class` errors that a bare ignore pattern cannot fix. Analysis-only; # never loaded at runtime or by PHPUnit. - tests/analysis-stubs/decidesk-events.stub.php + + ignoreErrors: + # OrganizationSyncService's `if ($contactObject !== null)` at the top of + # the contact-person loop. saveObject() returns a non-nullable + # ObjectEntityInterface, so the guard is provably true — the code a few + # lines ABOVE it already dereferences $contactObject unconditionally, + # which is the giveaway. + # + # Left in place rather than removed because the block it wraps is 243 + # lines: deleting the `if` is a pure re-indentation of a quarter of the + # method, which is a large, review-hostile diff for zero behaviour + # change. Worth doing when that method is next touched for real. + - + message: '#Strict comparison using !== between OCA\\OpenRegister\\Contract\\ObjectEntityInterface and null will always evaluate to true#' + identifier: notIdentical.alwaysTrue + path: lib/Service/OrganizationSyncService.php From 3223f41ee4c24d0c57e2b9e7c2d385d0c41e42b3 Mon Sep 17 00:00:00 2001 From: Conduction Release Bot Date: Sat, 22 Aug 2026 02:21:11 +0200 Subject: [PATCH 2/2] style(quality): satisfy phpcs on the comments this branch added MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI's phpcs step runs `--warning-severity=0`, and it failed on every one of the three PRat in this series for the same reason: comments I wrote. Two sniffs: - Squiz.Commenting.InlineComment.NotCapital — many of my new comments open with a lowercase function name ("// find() throws rather than ..."). Rephrased so the first word is a real capitalised word. - Generic.Commenting.DocComment.TagsNotGrouped — the `@param-out` tags I added were interleaved between `@param` tags, splitting the group. Moved below the last `@param`. One of those inserts had also orphaned a continuation line off the `@param` above it; that is rejoined. Where PHPStan genuinely needs a `/** @var */` inline doc-block (which Squiz.Commenting.InlineComment.DocBlock rejects), the line now carries a targeted `phpcs:ignore` naming that sniff and saying why, rather than dropping the annotation and leaving the type wrong. I should have caught this locally. I did run phpcs, but with `--report=summary | tail -3`, which prints only the timing line — so I read an empty tail as "clean" when the error count was two lines above the cut. Re-verified here with CI's exact invocation. --- lib/Service/OrganisatieService.php | 2 +- lib/Service/OrganizationSyncService.php | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/Service/OrganisatieService.php b/lib/Service/OrganisatieService.php index f5010b4a..9a1029ac 100644 --- a/lib/Service/OrganisatieService.php +++ b/lib/Service/OrganisatieService.php @@ -106,7 +106,7 @@ public function createOrganisationInOpenRegister(array $objectData): ?object { organizationUuid: $organizationUuid ); - // createOrganisationEntityInternal() is declared non-nullable and + // The createOrganisationEntityInternal() helper is declared non-nullable and // throws on failure — the catch below is the real failure path. $this->logger->info( 'OrganisatieService: Successfully created organization entity', diff --git a/lib/Service/OrganizationSyncService.php b/lib/Service/OrganizationSyncService.php index 7f7cdc83..d598cf2b 100644 --- a/lib/Service/OrganizationSyncService.php +++ b/lib/Service/OrganizationSyncService.php @@ -1404,9 +1404,10 @@ private function processContactPerson(object $contactPerson, array &$stats): ?st private function updateOrganisationEntityUsers(object $organisationEntity, array $usernames, array &$stats): void { try { $organisationUuid = $organisationEntity->getUuid(); - // OpenRegister is not on the analysis path, so getUsers() has no + // OpenRegister is not on the analysis path, so the getUsers() call has no // resolvable return type and sort() below cannot be checked without // this. It is a list of usernames. + // phpcs:ignore Squiz.Commenting.InlineComment.DocBlock -- PHPStan only reads @var from a /** */ block. /** @var array $currentUsers */ $currentUsers = ($organisationEntity->getUsers() ?? []);