From ce8096dabadee294251e6b0f8796d96ce0656060 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Anne?= Date: Thu, 23 Jan 2025 16:01:08 +0100 Subject: [PATCH] Allow numbers in custom objects system names --- .phpstan-baseline.php | 6 ----- .../Glpi/Asset/AssetDefinitionManagerTest.php | 15 +++++++++-- .../Glpi/Asset/AssetDefinitionTest.php | 14 +++++++++++ .../DropdownDefinitionManagerTest.php | 7 ++++++ .../Glpi/Dropdown/DropdownDefinitionTest.php | 14 +++++++++++ src/Glpi/Asset/AssetDefinitionManager.php | 19 ++++++++------ src/Glpi/CustomObject/AbstractDefinition.php | 14 +++++++++-- .../Dropdown/DropdownDefinitionManager.php | 5 ++-- src/Glpi/Search/Provider/SQLProvider.php | 13 +++------- .../pages/admin/customobjects/main.html.twig | 25 +++++++++++++------ 10 files changed, 95 insertions(+), 37 deletions(-) diff --git a/.phpstan-baseline.php b/.phpstan-baseline.php index 81b73504ca2..089f5d72d55 100644 --- a/.phpstan-baseline.php +++ b/.phpstan-baseline.php @@ -3073,12 +3073,6 @@ 'count' => 1, 'path' => __DIR__ . '/src/Glpi/Search/Provider/SQLProvider.php', ]; -$ignoreErrors[] = [ - 'message' => '#^Offset 2 on array\\{0\\: string, 1\\: string, 2\\: string, 3\\: numeric\\-string, 4\\?\\: string, 5\\?\\: non\\-empty\\-string\\} in isset\\(\\) always exists and is not nullable\\.$#', - 'identifier' => 'isset.offset', - 'count' => 1, - 'path' => __DIR__ . '/src/Glpi/Search/Provider/SQLProvider.php', -]; $ignoreErrors[] = [ 'message' => '#^Offset 2 on array\\{string, string, string\\} in isset\\(\\) always exists and is not nullable\\.$#', 'identifier' => 'isset.offset', diff --git a/phpunit/functional/Glpi/Asset/AssetDefinitionManagerTest.php b/phpunit/functional/Glpi/Asset/AssetDefinitionManagerTest.php index d6baa68a9b4..eb86bd01e98 100644 --- a/phpunit/functional/Glpi/Asset/AssetDefinitionManagerTest.php +++ b/phpunit/functional/Glpi/Asset/AssetDefinitionManagerTest.php @@ -37,8 +37,6 @@ use Change_Item; use DbTestCase; -use Glpi\Asset\Asset; -use Glpi\Asset\AssetDefinition; use Item_Problem; use Item_Ticket; use Profile; @@ -60,6 +58,19 @@ public function testLoadConcreteClass(): void } } + public function testAutoloader(): void + { + $this->initAssetDefinition('Test123'); // use a name with numbers, to validate it works as expected + + $this->assertTrue(\class_exists('Glpi\CustomAsset\Test123')); + $this->assertTrue(\class_exists('Glpi\CustomAsset\Test123Model')); + $this->assertTrue(\class_exists('Glpi\CustomAsset\Test123Type')); + $this->assertTrue(\class_exists('Glpi\CustomAsset\RuleDictionaryTest123ModelCollection')); + $this->assertTrue(\class_exists('Glpi\CustomAsset\RuleDictionaryTest123Model')); + $this->assertTrue(\class_exists('Glpi\CustomAsset\RuleDictionaryTest123TypeCollection')); + $this->assertTrue(\class_exists('Glpi\CustomAsset\RuleDictionaryTest123Type')); + } + /** * Ensure all asset types are registered in the ticket types configuration. * diff --git a/phpunit/functional/Glpi/Asset/AssetDefinitionTest.php b/phpunit/functional/Glpi/Asset/AssetDefinitionTest.php index de47b69e0d6..6194a0fac6e 100644 --- a/phpunit/functional/Glpi/Asset/AssetDefinitionTest.php +++ b/phpunit/functional/Glpi/Asset/AssetDefinitionTest.php @@ -260,6 +260,7 @@ public static function addInputProvider(): iterable if ( ($char >= "A" && $char <= "Z") // A -> Z || ($char >= "a" && $char <= "z") // a -> z + || ($char >= "0" && $char <= "9") // 0 -> 9 ) { yield [ 'input' => [ @@ -335,6 +336,19 @@ public static function addInputProvider(): iterable ]; } + // System name must not start with a number + yield [ + 'input' => [ + 'system_name' => '3DPrinter', + ], + 'output' => false, + 'messages' => [ + ERROR => [ + 'The following field has an incorrect value: "System name".', + ], + ], + ]; + // System name must not end with `Model` suffix yield [ 'input' => [ diff --git a/phpunit/functional/Glpi/Dropdown/DropdownDefinitionManagerTest.php b/phpunit/functional/Glpi/Dropdown/DropdownDefinitionManagerTest.php index 0f260acdbac..11d9da3cde8 100644 --- a/phpunit/functional/Glpi/Dropdown/DropdownDefinitionManagerTest.php +++ b/phpunit/functional/Glpi/Dropdown/DropdownDefinitionManagerTest.php @@ -55,6 +55,13 @@ public function testLoadConcreteClass(): void } } + public function testAutoloader(): void + { + $this->initDropdownDefinition('Iso3CountryCode'); // use a name with numbers, to validate it works as expected + + $this->assertTrue(\class_exists('Glpi\CustomDropdown\Iso3CountryCode')); + } + /** * Ensure all asset types are registered in the ticket types configuration. * diff --git a/phpunit/functional/Glpi/Dropdown/DropdownDefinitionTest.php b/phpunit/functional/Glpi/Dropdown/DropdownDefinitionTest.php index 49b5339ea32..64619d1d9de 100644 --- a/phpunit/functional/Glpi/Dropdown/DropdownDefinitionTest.php +++ b/phpunit/functional/Glpi/Dropdown/DropdownDefinitionTest.php @@ -211,6 +211,7 @@ public static function addInputProvider(): iterable if ( ($char >= "A" && $char <= "Z") // A -> Z || ($char >= "a" && $char <= "z") // a -> z + || ($char >= "0" && $char <= "9") // 0 -> 9 ) { yield [ 'input' => [ @@ -280,6 +281,19 @@ public static function addInputProvider(): iterable ]; } + // System name must not start with a number + yield [ + 'input' => [ + 'system_name' => '7Test', + ], + 'output' => false, + 'messages' => [ + ERROR => [ + 'The following field has an incorrect value: "System name".', + ], + ], + ]; + // System name must not end with `Model` suffix yield [ 'input' => [ diff --git a/src/Glpi/Asset/AssetDefinitionManager.php b/src/Glpi/Asset/AssetDefinitionManager.php index cb4ccf63c11..4e237dbd6cc 100644 --- a/src/Glpi/Asset/AssetDefinitionManager.php +++ b/src/Glpi/Asset/AssetDefinitionManager.php @@ -247,20 +247,23 @@ public function bootstrapDefinition(AbstractDefinition $definition): void */ public function autoloadClass(string $classname): void { - $ns = self::getDefinitionClass()::getCustomObjectNamespace() . '\\'; + $definition_class = self::getDefinitionClass(); + $ns = $definition_class::getCustomObjectNamespace() . '\\'; if (!\str_starts_with($classname, $ns)) { return; } + $system_name_pattern = $definition_class::SYSTEM_NAME_PATTERN; + $patterns = [ - '/^' . preg_quote($ns, '/') . 'RuleDictionary([A-Za-z]+)ModelCollection$/' => 'loadConcreteModelDictionaryCollectionClass', - '/^' . preg_quote($ns, '/') . 'RuleDictionary([A-Za-z]+)TypeCollection$/' => 'loadConcreteTypeDictionaryCollectionClass', - '/^' . preg_quote($ns, '/') . 'RuleDictionary([A-Za-z]+)Model$/' => 'loadConcreteModelDictionaryClass', - '/^' . preg_quote($ns, '/') . 'RuleDictionary([A-Za-z]+)Type$/' => 'loadConcreteTypeDictionaryClass', - '/^' . preg_quote($ns, '/') . '([A-Za-z]+)Model$/' => 'loadConcreteModelClass', - '/^' . preg_quote($ns, '/') . '([A-Za-z]+)Type$/' => 'loadConcreteTypeClass', - '/^' . preg_quote($ns, '/') . '([A-Za-z]+)$/' => 'loadConcreteClass', + '/^' . preg_quote($ns, '/') . 'RuleDictionary(' . $system_name_pattern . ')ModelCollection$/' => 'loadConcreteModelDictionaryCollectionClass', + '/^' . preg_quote($ns, '/') . 'RuleDictionary(' . $system_name_pattern . ')TypeCollection$/' => 'loadConcreteTypeDictionaryCollectionClass', + '/^' . preg_quote($ns, '/') . 'RuleDictionary(' . $system_name_pattern . ')Model$/' => 'loadConcreteModelDictionaryClass', + '/^' . preg_quote($ns, '/') . 'RuleDictionary(' . $system_name_pattern . ')Type$/' => 'loadConcreteTypeDictionaryClass', + '/^' . preg_quote($ns, '/') . '(' . $system_name_pattern . ')Model$/' => 'loadConcreteModelClass', + '/^' . preg_quote($ns, '/') . '(' . $system_name_pattern . ')Type$/' => 'loadConcreteTypeClass', + '/^' . preg_quote($ns, '/') . '(' . $system_name_pattern . ')$/' => 'loadConcreteClass', ]; foreach ($patterns as $pattern => $load_function) { diff --git a/src/Glpi/CustomObject/AbstractDefinition.php b/src/Glpi/CustomObject/AbstractDefinition.php index 459aaa4a0f2..35402a6b65f 100644 --- a/src/Glpi/CustomObject/AbstractDefinition.php +++ b/src/Glpi/CustomObject/AbstractDefinition.php @@ -40,7 +40,6 @@ use Gettext\Languages\CldrData as Language_CldrData; use Gettext\Languages\Language; use Glpi\Application\View\TemplateRenderer; -use Glpi\Asset\AssetDefinition; use Glpi\Asset\CustomFieldDefinition; use Profile; use ProfileRight; @@ -52,6 +51,14 @@ */ abstract class AbstractDefinition extends CommonDBTM { + /** + * System name regex pattern. + * + * 1. Must start with a letter. + * 2. Must contain only letters or numbers. + */ + public const SYSTEM_NAME_PATTERN = '[A-Za-z][A-Za-z0-9]*'; + public static $rightname = 'config'; /** @@ -434,7 +441,10 @@ protected function prepareInput(array $input): array|bool $has_errors = false; if (array_key_exists('system_name', $input)) { - if (!is_string($input['system_name']) || preg_match('/^[a-z]+$/i', $input['system_name']) !== 1) { + if ( + !is_string($input['system_name']) + || preg_match('/^' . self::SYSTEM_NAME_PATTERN . '$/', $input['system_name']) !== 1 + ) { Session::addMessageAfterRedirect( htmlescape(sprintf( __('The following field has an incorrect value: "%s".'), diff --git a/src/Glpi/Dropdown/DropdownDefinitionManager.php b/src/Glpi/Dropdown/DropdownDefinitionManager.php index afc38da4fd4..9bb3fd4a801 100644 --- a/src/Glpi/Dropdown/DropdownDefinitionManager.php +++ b/src/Glpi/Dropdown/DropdownDefinitionManager.php @@ -95,13 +95,14 @@ public function getReservedSystemNames(): array public function autoloadClass(string $classname): void { - $ns = static::getDefinitionClass()::getCustomObjectNamespace() . '\\'; + $definition_class = self::getDefinitionClass(); + $ns = $definition_class::getCustomObjectNamespace() . '\\'; if (!\str_starts_with($classname, $ns)) { return; } - $pattern = '/^' . preg_quote($ns, '/') . '([A-Za-z]+)$/'; + $pattern = '/^' . preg_quote($ns, '/') . '(' . $definition_class::SYSTEM_NAME_PATTERN . ')$/'; if (preg_match($pattern, $classname) === 1) { $system_name = preg_replace($pattern, '$1', $classname); diff --git a/src/Glpi/Search/Provider/SQLProvider.php b/src/Glpi/Search/Provider/SQLProvider.php index 606c808190b..4730306ca39 100644 --- a/src/Glpi/Search/Provider/SQLProvider.php +++ b/src/Glpi/Search/Provider/SQLProvider.php @@ -4789,15 +4789,10 @@ public static function constructData(array &$data, $onlycount = false) // Parse data foreach ($newrow['raw'] as $key => $val) { - if (preg_match('/ITEM(_(\w[^\d]+))?_(\d+)(_(.+))?/', $key, $matches)) { - $j = $matches[3]; - if (isset($matches[2]) && !empty($matches[2])) { - $j = $matches[2] . '_' . $matches[3]; - } - $fieldname = 'name'; - if (isset($matches[5])) { - $fieldname = $matches[5]; - } + $matches = []; + if (preg_match('/^ITEM(_(?[a-z][\w\\\]*))?_(?\d+)(_(?.+))?$/i', $key, $matches)) { + $j = (!empty($matches['itemtype']) ? $matches['itemtype'] . '_' : '') . $matches['num']; + $fieldname = $matches['fieldname'] ?? 'name'; // No Group_concat case if ($fieldname == 'content' || !is_string($val) || strpos($val, \Search::LONGSEP) === false) { diff --git a/templates/pages/admin/customobjects/main.html.twig b/templates/pages/admin/customobjects/main.html.twig index 459662ff7a2..0da8f507d99 100644 --- a/templates/pages/admin/customobjects/main.html.twig +++ b/templates/pages/admin/customobjects/main.html.twig @@ -128,30 +128,39 @@ {% endif %}