From e71e1ec2a2993eabfcb8f33d1872dad60f497176 Mon Sep 17 00:00:00 2001 From: Rom1-B <8530352+Rom1-B@users.noreply.github.com> Date: Wed, 8 Jul 2026 16:21:30 +0200 Subject: [PATCH] Fix: search crash on duplicate field names --- CHANGELOG.md | 3 + hook.php | 89 +++++++++------------ tests/Units/HookAddWhereTest.php | 128 +++++++++++++++++++++++++++++++ 3 files changed, 166 insertions(+), 54 deletions(-) create mode 100644 tests/Units/HookAddWhereTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index f81f8269..99ef6902 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## [unreleased] +### Fixed + +- Fix search crash when two containers share a dropdown field with the same name. ## [1.24.2] - 2026-06-30 diff --git a/hook.php b/hook.php index 1ecd134b..409b9af2 100644 --- a/hook.php +++ b/hook.php @@ -359,23 +359,20 @@ function plugin_fields_addWhere($link, $nott, $itemtype, $ID, $val, $searchtype) /** @var DBmysql $DB */ global $DB; - $searchopt = Search::getOptions($itemtype); - $table = $searchopt[$ID]['table']; - $field = $searchopt[$ID]['field']; - $pfields_type = $searchopt[$ID]['pfields_type'] ?? ''; + $searchopt = Search::getOptions($itemtype); + $table = $searchopt[$ID]['table']; + $field = $searchopt[$ID]['field']; + $pfields_fields_id = $searchopt[$ID]['pfields_fields_id'] ?? 0; + // Identify the field by its id (unique), not by its name: several containers + // can define a field with the same name, which would make a name-based lookup ambiguous. $field_field = new PluginFieldsField(); + if (!$field_field->getFromDB($pfields_fields_id)) { + return null; + } - if ( - $field_field->getFromDBByCrit( - [ - 'name' => $field, - 'type' => 'number', - ], - ) - && $pfields_type == 'number' - ) { - // if 'number' field with name is found with searchtype 'equals' or 'notequals' + if ($field_field->fields['type'] === 'number') { + // if 'number' field with searchtype 'equals' or 'notequals' // update WHERE clause with `$table_$field.$field` because without `$table_$field.id` is used if ($searchtype == 'equals' || $searchtype == 'notequals') { $operator = ($searchtype == 'equals') ? '=' : '!='; @@ -385,7 +382,7 @@ function plugin_fields_addWhere($link, $nott, $itemtype, $ID, $val, $searchtype) return $link . ' CAST(' . $DB->quoteName($table . '_' . $field) . '.' . $DB->quoteName($field) . ' AS DECIMAL(10,7))' . $operator . ' ' . $DB->quoteValue($val); } else { - // if 'number' field with name is found with <= or >= or < or > search + // if 'number' field with <= or >= or < or > search // update WHERE clause with the correct operator $val = html_entity_decode((string) $val); if (preg_match('/(<=|>=|>|<)/', $val, $matches)) { @@ -396,49 +393,33 @@ function plugin_fields_addWhere($link, $nott, $itemtype, $ID, $val, $searchtype) } } - // if 'multiple' field with name is found -> 'Dropdown-XXXX' case + if (!$field_field->fields['multiple']) { + return null; + } + + // 'Dropdown-XXXX' case: the searchopt field name is the plugin field name itself // update WHERE clause with LIKE statement - if ( - $field_field->getFromDBByCrit( - [ - 'name' => $field, - 'multiple' => true, - ], - ) - ) { + if (preg_match('/^dropdown-.+$/i', (string) $field_field->fields['type'])) { $tablefield = $table . '_' . $field; - switch ($searchtype) { - case 'equals': - return PluginFieldsDropdown::multipleDropdownAddWhere($link, $tablefield, $field, $val, $nott ? 'notequals' : 'equals', $field_field); - case 'notequals': - return PluginFieldsDropdown::multipleDropdownAddWhere($link, $tablefield, $field, $val, $nott ? 'equals' : 'notequals', $field_field); - } - } else { - // if 'multiple' field with cleaned name is found -> 'dropdown' case - // update WHERE clause with LIKE statement - $cleanfield = str_replace('plugin_fields_', '', $field); - $cleanfield = str_replace('dropdowns_id', '', $cleanfield); - $tablefield = $table . '_' . $cleanfield; - if ( - $field_field->getFromDBByCrit( - [ - 'name' => $cleanfield, - 'multiple' => true, - ], - ) - ) { - switch ($searchtype) { - case 'equals': - return PluginFieldsDropdown::multipleDropdownAddWhere($link, $tablefield, $field, $val, $nott ? 'notequals' : 'equals', $field_field); - case 'notequals': - return PluginFieldsDropdown::multipleDropdownAddWhere($link, $tablefield, $field, $val, $nott ? 'equals' : 'notequals', $field_field); - } - } else { - return false; - } + return match ($searchtype) { + 'equals' => PluginFieldsDropdown::multipleDropdownAddWhere($link, $tablefield, $field, $val, $nott ? 'notequals' : 'equals', $field_field), + 'notequals' => PluginFieldsDropdown::multipleDropdownAddWhere($link, $tablefield, $field, $val, $nott ? 'equals' : 'notequals', $field_field), + default => null, + }; } - return null; + // 'dropdown' case: the searchopt field name is mangled ("plugin_fields_dropdowns_id"), + // recover the real field name to build the joined table alias + // update WHERE clause with LIKE statement + $cleanfield = str_replace('plugin_fields_', '', $field); + $cleanfield = str_replace('dropdowns_id', '', $cleanfield); + + $tablefield = $table . '_' . $cleanfield; + return match ($searchtype) { + 'equals' => PluginFieldsDropdown::multipleDropdownAddWhere($link, $tablefield, $field, $val, $nott ? 'notequals' : 'equals', $field_field), + 'notequals' => PluginFieldsDropdown::multipleDropdownAddWhere($link, $tablefield, $field, $val, $nott ? 'equals' : 'notequals', $field_field), + default => null, + }; } function plugin_item_transfer_fields(array $options): void diff --git a/tests/Units/HookAddWhereTest.php b/tests/Units/HookAddWhereTest.php new file mode 100644 index 00000000..cf5709b8 --- /dev/null +++ b/tests/Units/HookAddWhereTest.php @@ -0,0 +1,128 @@ +. + * ------------------------------------------------------------------------- + * @copyright Copyright (C) 2013-2023 by Fields plugin team. + * @license GPLv2 https://www.gnu.org/licenses/gpl-2.0.html + * @link https://github.com/pluginsGLPI/fields + * ------------------------------------------------------------------------- + */ + +namespace GlpiPlugin\Field\Tests\Units; + +use Computer; +use Glpi\Tests\DbTestCase; +use Glpi\Tests\GLPITestCase; +use GlpiPlugin\Field\Tests\FieldTestTrait; +use PluginFieldsContainer; +use PluginFieldsField; +use Search; + +require_once __DIR__ . '/../FieldTestCase.php'; + +/** + * Regression test for ticket #45192: a search on an itemtype threw + * TooManyResultsException as soon as two containers defined a 'dropdown' + * field with the same name. + * + * PluginFieldsField::prepareName() intentionally links a new 'dropdown' + * field to an existing field with the same name so both containers share + * the same underlying dropdown table. This produces two distinct rows in + * glpi_plugin_fields_fields with the same name, and plugin_fields_addWhere() + * used to look the field up by name, which is ambiguous in that case. + */ +final class HookAddWhereTest extends DbTestCase +{ + use FieldTestTrait; + + public function setUp(): void + { + GLPITestCase::setUp(); + } + + public function tearDown(): void + { + $this->tearDownFieldTest(); + GLPITestCase::tearDown(); + } + + public function testAddWhereDoesNotThrowOnDuplicateFieldName(): void + { + $this->login(); + + $container1 = $this->createFieldContainer([ + 'label' => 'Container A ' . $this->getUniqueString(), + 'type' => 'tab', + 'itemtypes' => [Computer::class], + 'is_active' => 1, + 'entities_id' => 0, + 'is_recursive' => 1, + ]); + + // Bypass createField(): default_value gets JSON re-encoded by prepareInputForAdd() + // and cannot be compared as-is, so it must be excluded from the input check. + $field1 = $this->createItem(PluginFieldsField::class, [ + 'label' => 'Accessoire', + 'type' => 'dropdown', + 'multiple' => 1, + 'default_value' => [], + PluginFieldsContainer::getForeignKeyField() => $container1->getID(), + 'ranking' => 1, + 'is_active' => 1, + 'is_readonly' => 0, + ], ['allowed_values', 'question_types', 'default_value']); + + $container2 = $this->createFieldContainer([ + 'label' => 'Container B ' . $this->getUniqueString(), + 'type' => 'tab', + 'itemtypes' => [Computer::class], + 'is_active' => 1, + 'entities_id' => 0, + 'is_recursive' => 1, + ]); + + $field2 = $this->createItem(PluginFieldsField::class, [ + 'label' => 'Accessoire', + 'type' => 'dropdown', + 'multiple' => 1, + 'default_value' => [], + PluginFieldsContainer::getForeignKeyField() => $container2->getID(), + 'ranking' => 1, + 'is_active' => 1, + 'is_readonly' => 0, + ], ['allowed_values', 'question_types', 'default_value']); + + // Both containers intentionally share the same underlying field name. + $this->assertSame($field1->fields['name'], $field2->fields['name']); + + $searchopt = Search::getOptions(Computer::class); + $so_id = PluginFieldsField::SEARCH_OPTION_STARTING_INDEX + $field1->getID(); + $this->assertArrayHasKey($so_id, $searchopt); + + // Before the fix, this threw: + // `PluginFieldsField::getFromDBByCrit()` expects to get one result, 2 found. + $result = plugin_fields_addWhere('', false, Computer::class, $so_id, '1', 'equals'); + + $this->assertNotFalse($result); + } +}