From 6c99c040b238f7795690868db14293e9ecbb2749 Mon Sep 17 00:00:00 2001 From: fs Date: Fri, 24 Apr 2026 19:11:53 +0200 Subject: [PATCH] refactor(support): centralize ID-array normalization in toIntIds() Consolidates the scattered `array_values(array_unique(array_map('intval', $x)))` + manual positive-filter pattern behind a single lenient helper `toIntIds(mixed $value): array` in core/Support/helpers/array.php. - `RepositoryArrayHelper::sanitizePositiveIds()` now delegates (keeps strict array-input contract + existing tests intact). - Drops two private duplicates: `UserProfileViewService::normalizeIds()` and `AddressBookService::normalizeIds()`. - Replaces 12 inline occurrences across admin action pages with the helper, cutting boilerplate by 3-5 lines per site. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Support/RepositoryArrayHelper.php | 7 +-- core/Service/User/UserProfileViewService.php | 12 +---- core/Support/helpers.php | 1 + core/Support/helpers/array.php | 28 ++++++++++ .../Service/AddressBookService.php | 8 +-- pages/admin/departments/create().php | 2 +- pages/admin/departments/data().php | 2 +- pages/admin/departments/index().php | 2 +- pages/admin/permissions/edit($id).php | 12 +---- pages/admin/roles/create().php | 12 +---- pages/admin/roles/edit($id).php | 18 ++----- pages/admin/tenants/data().php | 2 +- pages/admin/users/_form.phtml | 10 +--- tests/Support/Helpers/ToIntIdsTest.php | 51 +++++++++++++++++++ 14 files changed, 99 insertions(+), 68 deletions(-) create mode 100644 core/Support/helpers/array.php create mode 100644 tests/Support/Helpers/ToIntIdsTest.php diff --git a/core/Repository/Support/RepositoryArrayHelper.php b/core/Repository/Support/RepositoryArrayHelper.php index ef608c3..cfe45c6 100644 --- a/core/Repository/Support/RepositoryArrayHelper.php +++ b/core/Repository/Support/RepositoryArrayHelper.php @@ -63,12 +63,13 @@ final class RepositoryArrayHelper /** * Sanitize an array of IDs to unique positive integers. + * + * Thin repository-layer adapter over the global `toIntIds()` helper, + * kept for the stricter `array` input contract expected by repositories. */ public static function sanitizePositiveIds(array $ids): array { - $ids = array_values(array_unique(array_map('intval', $ids))); - - return array_values(array_filter($ids, static fn (int $id): bool => $id > 0)); + return toIntIds($ids); } /** diff --git a/core/Service/User/UserProfileViewService.php b/core/Service/User/UserProfileViewService.php index 8d4bad8..f5e157c 100644 --- a/core/Service/User/UserProfileViewService.php +++ b/core/Service/User/UserProfileViewService.php @@ -50,7 +50,7 @@ class UserProfileViewService return ['status' => 'forbidden']; } - $viewerTenantIds = $this->normalizeIds($this->scopeGateway->getUserTenantIds($currentUserId)); + $viewerTenantIds = toIntIds($this->scopeGateway->getUserTenantIds($currentUserId)); $assignments = $this->userAssignmentService->buildAssignmentsForUser($targetUserId); $departmentMap = []; @@ -154,14 +154,4 @@ class UserProfileViewService return $profile; } - - /** - * @param array $values - * @return list - */ - private function normalizeIds(array $values): array - { - $ids = array_values(array_unique(array_map('intval', $values))); - return array_values(array_filter($ids, static fn (int $id): bool => $id > 0)); - } } diff --git a/core/Support/helpers.php b/core/Support/helpers.php index fb7faa6..84f36d5 100644 --- a/core/Support/helpers.php +++ b/core/Support/helpers.php @@ -2,6 +2,7 @@ // Load all global helper groups used by templates and page actions. require __DIR__ . '/helpers/app.php'; +require __DIR__ . '/helpers/array.php'; require __DIR__ . '/helpers/branding.php'; require __DIR__ . '/helpers/export.php'; require __DIR__ . '/helpers/grid.php'; diff --git a/core/Support/helpers/array.php b/core/Support/helpers/array.php new file mode 100644 index 0000000..4cb8d9a --- /dev/null +++ b/core/Support/helpers/array.php @@ -0,0 +1,28 @@ +body('role_ids', [])` + * before handing them to a service or repository. + * + * @return list + */ +function toIntIds(mixed $value): array +{ + if ($value === null || $value === '') { + return []; + } + if (!is_array($value)) { + $value = [$value]; + } + $ids = array_map('intval', $value); + $ids = array_filter($ids, static fn (int $id): bool => $id > 0); + + return array_values(array_unique($ids)); +} diff --git a/modules/addressbook/lib/Module/AddressBook/Service/AddressBookService.php b/modules/addressbook/lib/Module/AddressBook/Service/AddressBookService.php index ff26ba2..a006007 100644 --- a/modules/addressbook/lib/Module/AddressBook/Service/AddressBookService.php +++ b/modules/addressbook/lib/Module/AddressBook/Service/AddressBookService.php @@ -33,7 +33,7 @@ class AddressBookService $activeRoles = $this->splitCommaValues($query['roles'] ?? ''); $activeDepartments = $this->splitCommaValues($query['departments'] ?? ''); - $tenantIds = $this->normalizeIds($this->scopeGateway->getUserTenantIds($currentUserId)); + $tenantIds = toIntIds($this->scopeGateway->getUserTenantIds($currentUserId)); $tenants = $this->tenantService->list(); if ($tenantIds) { $allowedTenantMap = array_fill_keys($tenantIds, true); @@ -318,12 +318,6 @@ class AddressBookService )); } - private function normalizeIds(array $values): array - { - $ids = array_values(array_unique(array_map('intval', $values))); - return array_values(array_filter($ids, static fn (int $id): bool => $id > 0)); - } - // Accepts either a '||'-delimited DB aggregate string (from GROUP_CONCAT) or a plain array. private function normalizeLabelList($value): array { diff --git a/pages/admin/departments/create().php b/pages/admin/departments/create().php index c3ad762..5d18f8b 100644 --- a/pages/admin/departments/create().php +++ b/pages/admin/departments/create().php @@ -26,7 +26,7 @@ if (!$decision->isAllowed()) { Guard::deny(); } $capabilities = $decision->attribute('capabilities', []); -$allowedTenantIds = is_array($capabilities) ? array_values(array_unique(array_map('intval', (array) ($capabilities['allowed_tenant_ids'] ?? [])))) : []; +$allowedTenantIds = is_array($capabilities) ? toIntIds($capabilities['allowed_tenant_ids'] ?? []) : []; $isStrictScope = is_array($capabilities) ? (bool) ($capabilities['is_strict_scope'] ?? false) : false; $errorBag = formErrors(); diff --git a/pages/admin/departments/data().php b/pages/admin/departments/data().php index 6d57805..0713ac3 100644 --- a/pages/admin/departments/data().php +++ b/pages/admin/departments/data().php @@ -11,7 +11,7 @@ gridRequireGetRequest(); $currentUserId = (int) ($session['user']['id'] ?? 0); $decision = Guard::requireAbilityDecisionOrForbidden(DepartmentAuthorizationPolicy::ABILITY_ADMIN_DEPARTMENTS_VIEW); $capabilities = $decision->attribute('capabilities', []); -$tenantIds = is_array($capabilities) ? array_values(array_unique(array_map('intval', (array) ($capabilities['allowed_tenant_ids'] ?? [])))) : []; +$tenantIds = is_array($capabilities) ? toIntIds($capabilities['allowed_tenant_ids'] ?? []) : []; $filters = gridParseFiltersFromSchemaFile(__DIR__ . '/filter-schema.php'); diff --git a/pages/admin/departments/index().php b/pages/admin/departments/index().php index b05f8f7..608afee 100644 --- a/pages/admin/departments/index().php +++ b/pages/admin/departments/index().php @@ -18,7 +18,7 @@ if (!$decision->isAllowed()) { Guard::deny(); } $capabilities = $decision->attribute('capabilities', []); -$allowedTenantIds = is_array($capabilities) ? array_values(array_unique(array_map('intval', (array) ($capabilities['allowed_tenant_ids'] ?? [])))) : []; +$allowedTenantIds = is_array($capabilities) ? toIntIds($capabilities['allowed_tenant_ids'] ?? []) : []; $isStrictScope = is_array($capabilities) ? (bool) ($capabilities['is_strict_scope'] ?? false) : false; $canCreateDepartments = is_array($capabilities) ? (bool) ($capabilities['can_create_department'] ?? false) : false; $filterSchema = require __DIR__ . '/filter-schema.php'; diff --git a/pages/admin/permissions/edit($id).php b/pages/admin/permissions/edit($id).php index 1f01e0a..0af8cb7 100644 --- a/pages/admin/permissions/edit($id).php +++ b/pages/admin/permissions/edit($id).php @@ -63,18 +63,10 @@ if ($request->isMethod('POST')) { $result = app(\MintyPHP\Service\Access\PermissionService::class)->updateFromAdmin($id, $request->bodyAll()); $form = $result['form'] ?? $form; $errorBag->merge($result['errors'] ?? []); - $selectedRoleIds = $request->body('role_ids', $selectedRoleIds); - if (!is_array($selectedRoleIds)) { - $selectedRoleIds = [$selectedRoleIds]; - } - $selectedRoleIds = array_values(array_unique(array_map('intval', $selectedRoleIds))); + $selectedRoleIds = toIntIds($request->body('role_ids', $selectedRoleIds)); if (($result['ok'] ?? false) && !$errorBag->hasAny()) { - $roleIds = $request->body('role_ids', []); - if (!is_array($roleIds)) { - $roleIds = [$roleIds]; - } - $roleIds = array_values(array_unique(array_map('intval', $roleIds))); + $roleIds = toIntIds($request->body('role_ids', [])); $syncSucceeded = false; $dbSession = app(\MintyPHP\Repository\Support\DatabaseSessionRepository::class); $dbSession->beginTransaction(); diff --git a/pages/admin/roles/create().php b/pages/admin/roles/create().php index 5171944..dc08dc4 100644 --- a/pages/admin/roles/create().php +++ b/pages/admin/roles/create().php @@ -52,18 +52,10 @@ if ($request->isMethod('POST')) { $form = $result['form'] ?? $form; $warnings = $result['warnings'] ?? []; $errorBag->merge($result['errors'] ?? []); - $selectedPermissionIds = $request->body('permission_ids', $selectedPermissionIds); - if (!is_array($selectedPermissionIds)) { - $selectedPermissionIds = [$selectedPermissionIds]; - } - $selectedPermissionIds = array_values(array_unique(array_map('intval', $selectedPermissionIds))); + $selectedPermissionIds = toIntIds($request->body('permission_ids', $selectedPermissionIds)); if (($result['ok'] ?? false) && !$errorBag->hasAny()) { - $permissionIds = $request->body('permission_ids', []); - if (!is_array($permissionIds)) { - $permissionIds = [$permissionIds]; - } - $permissionIds = array_values(array_unique(array_map('intval', $permissionIds))); + $permissionIds = toIntIds($request->body('permission_ids', [])); $createdId = (int) ($result['id'] ?? 0); if ($createdId <= 0) { throw new \RuntimeException('role_create_missing_id'); diff --git a/pages/admin/roles/edit($id).php b/pages/admin/roles/edit($id).php index 1af7ff7..086054b 100644 --- a/pages/admin/roles/edit($id).php +++ b/pages/admin/roles/edit($id).php @@ -68,23 +68,11 @@ if ($request->isMethod('POST')) { $form = $result['form'] ?? $form; $warnings = $result['warnings'] ?? []; $errorBag->merge($result['errors'] ?? []); - $selectedPermissionIds = $request->body('permission_ids', $selectedPermissionIds); - if (!is_array($selectedPermissionIds)) { - $selectedPermissionIds = [$selectedPermissionIds]; - } - $selectedPermissionIds = array_values(array_unique(array_map('intval', $selectedPermissionIds))); - $selectedAssignableRoleIds = $request->body('assignable_role_ids', $selectedAssignableRoleIds); - if (!is_array($selectedAssignableRoleIds)) { - $selectedAssignableRoleIds = [$selectedAssignableRoleIds]; - } - $selectedAssignableRoleIds = array_values(array_unique(array_map('intval', $selectedAssignableRoleIds))); + $selectedPermissionIds = toIntIds($request->body('permission_ids', $selectedPermissionIds)); + $selectedAssignableRoleIds = toIntIds($request->body('assignable_role_ids', $selectedAssignableRoleIds)); if (($result['ok'] ?? false) && !$errorBag->hasAny()) { - $permissionIds = $request->body('permission_ids', []); - if (!is_array($permissionIds)) { - $permissionIds = [$permissionIds]; - } - $permissionIds = array_values(array_unique(array_map('intval', $permissionIds))); + $permissionIds = toIntIds($request->body('permission_ids', [])); $syncSucceeded = false; $dbSession = app(\MintyPHP\Repository\Support\DatabaseSessionRepository::class); $dbSession->beginTransaction(); diff --git a/pages/admin/tenants/data().php b/pages/admin/tenants/data().php index bf2f8cf..cc4de28 100644 --- a/pages/admin/tenants/data().php +++ b/pages/admin/tenants/data().php @@ -9,7 +9,7 @@ gridRequireGetRequest(); $decision = Guard::requireAbilityDecisionOrForbidden(TenantAuthorizationPolicy::ABILITY_ADMIN_TENANTS_VIEW); $capabilities = $decision->attribute('capabilities', []); -$allowedTenantIds = is_array($capabilities) ? array_values(array_unique(array_map('intval', (array) ($capabilities['allowed_tenant_ids'] ?? [])))) : []; +$allowedTenantIds = is_array($capabilities) ? toIntIds($capabilities['allowed_tenant_ids'] ?? []) : []; $isStrictScope = is_array($capabilities) ? (bool) ($capabilities['is_strict_scope'] ?? false) : false; $hasGlobalAccess = is_array($capabilities) ? (bool) ($capabilities['has_global_access'] ?? false) : false; diff --git a/pages/admin/users/_form.phtml b/pages/admin/users/_form.phtml index ee4293d..f336add 100644 --- a/pages/admin/users/_form.phtml +++ b/pages/admin/users/_form.phtml @@ -798,14 +798,8 @@ foreach ($tenants as $tenant) { ? $customFieldScalarPosted[$scalarPostedKey] : null; $multiPostedRaw = $customFieldMultiPosted[$scalarPostedKey] ?? null; - $multiPosted = is_array($multiPostedRaw) - ? array_values(array_unique(array_map('intval', $multiPostedRaw))) - : []; - $multiPosted = array_values(array_filter($multiPosted, static fn (int $id): bool => $id > 0)); - $currentOptionIds = is_array($currentValue['option_ids'] ?? null) - ? array_values(array_unique(array_map('intval', $currentValue['option_ids']))) - : []; - $currentOptionIds = array_values(array_filter($currentOptionIds, static fn (int $id): bool => $id > 0)); + $multiPosted = is_array($multiPostedRaw) ? toIntIds($multiPostedRaw) : []; + $currentOptionIds = toIntIds($currentValue['option_ids'] ?? null); $valueText = array_key_exists('value_text', $currentValue) ? (string) ($currentValue['value_text'] ?? '') : ''; $valueBool = array_key_exists('value_bool', $currentValue) && $currentValue['value_bool'] !== null ? (string) ((int) $currentValue['value_bool']) diff --git a/tests/Support/Helpers/ToIntIdsTest.php b/tests/Support/Helpers/ToIntIdsTest.php new file mode 100644 index 0000000..240351c --- /dev/null +++ b/tests/Support/Helpers/ToIntIdsTest.php @@ -0,0 +1,51 @@ +assertSame([1, 3, 5], toIntIds([1, 3, 5, 1, 3])); + } + + public function testDropsNonPositiveValues(): void + { + $this->assertSame([2, 4], toIntIds([0, -1, 2, -3, 4])); + } + + public function testCoercesNumericStrings(): void + { + $this->assertSame([1, 2, 3], toIntIds(['1', '2', '3'])); + } + + public function testIgnoresNonNumericStrings(): void + { + $this->assertSame([7], toIntIds(['abc', '7', ''])); + } + + public function testEmptyArrayReturnsEmpty(): void + { + $this->assertSame([], toIntIds([])); + } + + public function testNullAndEmptyStringReturnEmpty(): void + { + $this->assertSame([], toIntIds(null)); + $this->assertSame([], toIntIds('')); + } + + public function testScalarIsWrappedAsSingleElement(): void + { + $this->assertSame([42], toIntIds(42)); + $this->assertSame([42], toIntIds('42')); + } + + public function testZeroScalarReturnsEmpty(): void + { + $this->assertSame([], toIntIds(0)); + $this->assertSame([], toIntIds('0')); + } +}