Allow numbers in custom objects system names

glpi-project · Jan 30, 2025 · ce8096d · ce8096d
1 parent 219cb7c
commit ce8096d
Show file tree

Hide file tree

Showing 10 changed files with 95 additions and 37 deletions.
diff --git 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
@@ -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
@@ -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: &quot;System name&quot;.',
+                ],
+            ],
+        ];
+
         // 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
@@ -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
@@ -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: &quot;System name&quot;.',
+                ],
+            ],
+        ];
+
         // System name must not end with `Model` suffix
         yield [
             'input'    => [

diff --git 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
@@ -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
@@ -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
@@ -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(_(?<itemtype>[a-z][\w\\\]*))?_(?<num>\d+)(_(?<fieldname>.+))?$/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
@@ -128,30 +128,39 @@
         </script>
     {% endif %}
     <script type="module">
-        $('#mainformtable input[name="system_name"]').on('input', () => {
+        $('#mainformtable input[name="system_name"]').on('change', () => {
             if ($('input[name="system_name"]').val() === '') {
                 $('input[name="system_name"]').data('manually_changed', false);
             } else {
                 $('input[name="system_name"]').data('manually_changed', true);
             }
         });
-        function autoUpdateNameField() {
-            if ({{ not item.isNewItem() ? 'true' : 'false' }} || $('#mainformtable input[name="system_name"]').data('manually_changed')) {
-                return;
-            }
+        $('#mainformtable input[name="system_name"]').on('input', () => {
             const reserved_names = {{ reserved_system_names|json_encode()|raw }}.map((n) => n.toLowerCase());
             const existing_names = {{ existing_system_names|json_encode()|raw }}.map((n) => n.toLowerCase());
-            $('#mainformtable input[name="system_name"]').val(
-                $('#mainformtable input[name="label"]').val().normalize('NFD').replace(/[^a-z]/gi, '')
-            );
             const system_name = $('#mainformtable input[name="system_name"]').val().toLowerCase();
+            const system_name_pattern = /^{{ constant('Glpi\\CustomObject\\AbstractDefinition::SYSTEM_NAME_PATTERN')|raw }}$/;
+
             if (reserved_names.includes(system_name) || system_name.endsWith('type') || system_name.endsWith('model')) {
                 $('#mainformtable input[name="system_name"]').get(0).setCustomValidity(__('The system name is a reserved name. Please enter a different label or manually change the system name.'));
             } else if (existing_names.includes(system_name)) {
                 $('#mainformtable input[name="system_name"]').get(0).setCustomValidity(__('The system name is already in use. Please enter a different label or manually change the system name.'));
+            } else if (system_name_pattern.test(system_name) === false) {
+                {# See pattern used in \Glpi\CustomObject\AbstractDefinition::prepareInput() #}
+                $('#mainformtable input[name="system_name"]').get(0).setCustomValidity(__('The system name is invalid. It must start with a letter and contain only alphanumeric chars.'));
             } else {
                 $('#mainformtable input[name="system_name"]').get(0).setCustomValidity('');
             }
+        });
+        function autoUpdateNameField() {
+            if ({{ not item.isNewItem() ? 'true' : 'false' }} || $('#mainformtable input[name="system_name"]').data('manually_changed')) {
+                return;
+            }
+
+            $('#mainformtable input[name="system_name"]').val(
+                $('#mainformtable input[name="label"]').val().normalize('NFD').replace(/[^a-z0-9]/gi, '')
+            );
+            $('#mainformtable input[name="system_name"]').trigger('input');
         };
         $('#mainformtable input[name="label"]').on('input', () => {
             autoUpdateNameField();