From a13042ca408e97485e90d2eb75723dda6864bb1f Mon Sep 17 00:00:00 2001 From: mbressy Date: Thu, 8 Oct 2026 09:21:35 +0000 Subject: [PATCH 1/2] mandatory fields blocking ticket creation from a GLPI form --- CHANGELOG.md | 6 +++ inc/container.class.php | 25 ++++++++++- inc/destinationfield.class.php | 20 +++++++++ templates/fields.html.twig | 4 +- tests/Units/FieldDestinationFieldTest.php | 52 +++++++++++++++++++++++ 5 files changed, 105 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b582f6f..a18fcdae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/) and this project adheres to [Semantic Versioning](http://semver.org/). +## [Unreleased] + +### Fixed + +- Fix mandatory fields blocking ticket creation from a GLPI form. + ## [1.24.6] - 2026-10-06 ### Added diff --git a/inc/container.class.php b/inc/container.class.php index 0bed98d9..1e6c12ee 100644 --- a/inc/container.class.php +++ b/inc/container.class.php @@ -1684,13 +1684,24 @@ public static function constructHistory( } } + /** @var array, array> Items created by a form destination in this request */ + private static array $form_destination_items = []; + + // Also true for the updates the destination does right after the creation (e.g. link to the form) + private static function isCreatedByFormDestination(CommonDBTM $item): bool + { + return PluginFieldsDestinationField::hasValidMarker($item->input) + || isset(self::$form_destination_items[$item::class][$item->getID()]); + } + private static function isMandatoryCheckBypassed(array $data): bool { return isCommandLine() || Session::isCron() || isAPI() || !empty($data['_auto_import']) - || !empty($data['is_dynamic']); + || !empty($data['is_dynamic']) + || PluginFieldsDestinationField::hasValidMarker($data); } /** @@ -2044,6 +2055,10 @@ public static function findAllContainers($itemtype) */ public static function postItemAdd(CommonDBTM $item) { + if (PluginFieldsDestinationField::hasValidMarker($item->input)) { + self::$form_destination_items[$item::class][$item->getID()] = true; + } + if (array_key_exists('_plugin_fields_data', $item->input)) { $data = $item->input['_plugin_fields_data']; $data['itemtype'] = $item::class; @@ -2222,6 +2237,10 @@ private static function checkContainerMandatory(CommonDBTM $item, PluginFieldsCo $data['is_dynamic'] = true; } + if (self::isCreatedByFormDestination($item)) { + $data[PluginFieldsDestinationField::INPUT_MARKER] = PluginFieldsDestinationField::getMarkerToken(); + } + if (($status_value = self::getStatusValue($item)) !== null) { $data[$status_field_name] = $status_value; } @@ -2296,6 +2315,10 @@ private static function populateData($c_id, CommonDBTM $item) $data['is_dynamic'] = true; } + if (self::isCreatedByFormDestination($item)) { + $data[PluginFieldsDestinationField::INPUT_MARKER] = PluginFieldsDestinationField::getMarkerToken(); + } + // Add status so it can be used with status overrides $data[PluginFieldsStatusOverride::getStatusFieldName($item->getType())] = self::getStatusValue($item); diff --git a/inc/destinationfield.class.php b/inc/destinationfield.class.php index 00214e10..4d639e06 100644 --- a/inc/destinationfield.class.php +++ b/inc/destinationfield.class.php @@ -42,6 +42,24 @@ class PluginFieldsDestinationField extends AbstractConfigField { + // Input key flagging an item as created by a form destination + public const INPUT_MARKER = '_plugin_fields_form_destination'; + + private static ?string $marker_token = null; + + // Random per-request value: the marker cannot be forged from a submitted form + public static function getMarkerToken(): string + { + return self::$marker_token ??= bin2hex(random_bytes(16)); + } + + public static function hasValidMarker(array $input): bool + { + $marker = $input[self::INPUT_MARKER] ?? null; + + return is_string($marker) && hash_equals(self::getMarkerToken(), $marker); + } + public function __construct(private readonly AbstractCommonITILFormDestination $itil_destination) {} #[Override] @@ -86,6 +104,8 @@ public function applyConfiguratedValueToInputUsingAnswers( throw new InvalidArgumentException("Unexpected config class"); } + $input[self::INPUT_MARKER] = self::getMarkerToken(); + if ((bool) $config->getValue()) { $answers = $answers_set->getAnswersByTypes([ PluginFieldsQuestionType::class, diff --git a/templates/fields.html.twig b/templates/fields.html.twig index c53da422..08bc29bf 100644 --- a/templates/fields.html.twig +++ b/templates/fields.html.twig @@ -96,6 +96,8 @@ {% set value = item.input[name]|default(field['value']) %} {% set readonly = field['is_readonly'] %} {% set rand = random() %} + {# On an item form, a single dropdown without value must still render its empty option #} + {% set dropdown_value = field['multiple'] or massiveaction or in_form_builder ? value : value|default(0) %} {% set field_options = field_options|merge({ 'readonly': readonly or not canedit, @@ -161,7 +163,7 @@ {% if input_name == name %} {% set name_fk = call("getForeignKeyFieldForItemType", [dropdown_itemtype]) %} {% endif %} - {{ macros.dropdownField(dropdown_itemtype, name_fk ?? input_name, value, label, field_options|merge(dropdown_options|default({}))) }} + {{ macros.dropdownField(dropdown_itemtype, name_fk ?? input_name, dropdown_value, label, field_options|merge(dropdown_options|default({}))) }} {% elseif type matches '/^dropdown-.+/i' %} {% set dropdown_options = {'entity': item.getEntityID()} %} diff --git a/tests/Units/FieldDestinationFieldTest.php b/tests/Units/FieldDestinationFieldTest.php index 604d0d52..f3bff6aa 100644 --- a/tests/Units/FieldDestinationFieldTest.php +++ b/tests/Units/FieldDestinationFieldTest.php @@ -235,6 +235,58 @@ public function testDestinationWithAdditionalFieldsButDisabledInConfig(): void ); } + public function testDestinationIsNotBlockedByMandatoryFieldNotProvidedByForm(): void + { + $this->login(); + $GLOBALS['GLPI_IS_COMMAND_LINE'] = false; + + $mandatory_field = $this->createField([ + 'label' => 'Mandatory text', + 'type' => 'text', + PluginFieldsContainer::getForeignKeyField() => $this->blocks[Ticket::class]->getID(), + 'ranking' => 4, + 'is_active' => 1, + 'is_readonly' => 0, + 'mandatory' => 1, + ]); + + $builder = (new FormBuilder())->addQuestion("Short text", QuestionTypeShortText::class); + $form = $this->createForm($builder); + + try { + foreach ([new SimpleValueConfig(1), new SimpleValueConfig(false)] as $config) { + $this->sendFormAndAssertITILObjectAdditionalFields( + form: $form, + config: $config, + answers: [ + "Short text" => "Test value", + ], + expected_field_values: [Ticket::class => []], + ); + } + + // The field is still mandatory outside of a form destination, even with a forged marker + $ticket = new Ticket(); + $this->assertFalse($ticket->add([ + 'name' => 'Ticket created from the ticket form', + 'content' => 'Test creation', + 'entities_id' => $this->getTestRootEntity(true), + PluginFieldsDestinationField::INPUT_MARKER => '1', + ])); + $this->hasSessionMessageThatContains( + __('Some mandatory fields are empty', 'fields'), + ERROR, + ); + $this->hasSessionMessageThatContains( + __('The form or source creating this item does not provide the mandatory fields above: map them to it, or make them optional.', 'fields'), + ERROR, + ); + } finally { + unset($GLOBALS['GLPI_IS_COMMAND_LINE']); + $mandatory_field->delete($mandatory_field->fields, true); + } + } + public function testDestinationWithLocationAdditonalFields(): void { $this->login(); From f40f3be513b127fd7b137171bcd411fb828fea49 Mon Sep 17 00:00:00 2001 From: mbressy Date: Fri, 9 Oct 2026 12:14:10 +0000 Subject: [PATCH 2/2] enforce mandatory fields provided by the form, drop input marker --- inc/container.class.php | 31 ++------ inc/destinationfield.class.php | 49 +++++++++--- templates/fields.html.twig | 2 +- tests/Units/ContainerItemRightTest.php | 38 ++++++++++ tests/Units/FieldDestinationFieldTest.php | 91 +++++++++++++++++++++-- 5 files changed, 169 insertions(+), 42 deletions(-) diff --git a/inc/container.class.php b/inc/container.class.php index 1e6c12ee..991cf82d 100644 --- a/inc/container.class.php +++ b/inc/container.class.php @@ -1684,24 +1684,13 @@ public static function constructHistory( } } - /** @var array, array> Items created by a form destination in this request */ - private static array $form_destination_items = []; - - // Also true for the updates the destination does right after the creation (e.g. link to the form) - private static function isCreatedByFormDestination(CommonDBTM $item): bool - { - return PluginFieldsDestinationField::hasValidMarker($item->input) - || isset(self::$form_destination_items[$item::class][$item->getID()]); - } - private static function isMandatoryCheckBypassed(array $data): bool { return isCommandLine() || Session::isCron() || isAPI() || !empty($data['_auto_import']) - || !empty($data['is_dynamic']) - || PluginFieldsDestinationField::hasValidMarker($data); + || !empty($data['is_dynamic']); } /** @@ -1758,6 +1747,9 @@ public static function validateValues($data, $itemtype, $massiveaction, $is_crea $stored_values = self::getStoredValues($container, $itemtype, (int) ($data['items_id'] ?? 0)); + // Set while a form destination creates the item: only the fields the form provides are then mandatory + $form_fields = PluginFieldsDestinationField::getFieldsProvidedByForm($itemtype, (int) ($data['items_id'] ?? 0)); + foreach ($fields as $field) { if (!$field['is_active']) { continue; @@ -1802,6 +1794,7 @@ public static function validateValues($data, $itemtype, $massiveaction, $is_crea if ( !self::isMandatoryCheckBypassed($data) + && ($form_fields === null || in_array((int) $field['id'], $form_fields, true)) && $field['mandatory'] == 1 && ( empty($value) @@ -2055,9 +2048,7 @@ public static function findAllContainers($itemtype) */ public static function postItemAdd(CommonDBTM $item) { - if (PluginFieldsDestinationField::hasValidMarker($item->input)) { - self::$form_destination_items[$item::class][$item->getID()] = true; - } + PluginFieldsDestinationField::registerCreatedItem($item); if (array_key_exists('_plugin_fields_data', $item->input)) { $data = $item->input['_plugin_fields_data']; @@ -2237,15 +2228,13 @@ private static function checkContainerMandatory(CommonDBTM $item, PluginFieldsCo $data['is_dynamic'] = true; } - if (self::isCreatedByFormDestination($item)) { - $data[PluginFieldsDestinationField::INPUT_MARKER] = PluginFieldsDestinationField::getMarkerToken(); - } - if (($status_value = self::getStatusValue($item)) !== null) { $data[$status_field_name] = $status_value; } if (!$item->isNewItem()) { + $data['items_id'] = $item->getID(); + // merge already persisted values to avoid false positives $classname = self::getClassname($item::getType(), $loc_c->fields['name']); $dbu = new DbUtils(); @@ -2315,10 +2304,6 @@ private static function populateData($c_id, CommonDBTM $item) $data['is_dynamic'] = true; } - if (self::isCreatedByFormDestination($item)) { - $data[PluginFieldsDestinationField::INPUT_MARKER] = PluginFieldsDestinationField::getMarkerToken(); - } - // Add status so it can be used with status overrides $data[PluginFieldsStatusOverride::getStatusFieldName($item->getType())] = self::getStatusValue($item); diff --git a/inc/destinationfield.class.php b/inc/destinationfield.class.php index 4d639e06..58173bf8 100644 --- a/inc/destinationfield.class.php +++ b/inc/destinationfield.class.php @@ -42,22 +42,30 @@ class PluginFieldsDestinationField extends AbstractConfigField { - // Input key flagging an item as created by a form destination - public const INPUT_MARKER = '_plugin_fields_form_destination'; + /** @var array{itemtype: class-string, fields: list}|null Item a destination is about to create */ + private static ?array $pending_item = null; - private static ?string $marker_token = null; + /** @var array, array>> Fields provided by the form, per item being created */ + private static array $created_items = []; - // Random per-request value: the marker cannot be forged from a submitted form - public static function getMarkerToken(): string + /** @return list|null Fields provided by the form, null when no destination is creating the item */ + public static function getFieldsProvidedByForm(string $itemtype, int $items_id): ?array { - return self::$marker_token ??= bin2hex(random_bytes(16)); + if ($items_id > 0) { + return self::$created_items[$itemtype][$items_id] ?? null; + } + + return (self::$pending_item['itemtype'] ?? null) === $itemtype ? self::$pending_item['fields'] : null; } - public static function hasValidMarker(array $input): bool + public static function registerCreatedItem(CommonDBTM $item): void { - $marker = $input[self::INPUT_MARKER] ?? null; + if ((self::$pending_item['itemtype'] ?? null) !== $item::class) { + return; + } - return is_string($marker) && hash_equals(self::getMarkerToken(), $marker); + self::$created_items[$item::class][$item->getID()] = self::$pending_item['fields']; + self::$pending_item = null; } public function __construct(private readonly AbstractCommonITILFormDestination $itil_destination) {} @@ -104,7 +112,7 @@ public function applyConfiguratedValueToInputUsingAnswers( throw new InvalidArgumentException("Unexpected config class"); } - $input[self::INPUT_MARKER] = self::getMarkerToken(); + $provided_fields = []; if ((bool) $config->getValue()) { $answers = $answers_set->getAnswersByTypes([ @@ -146,6 +154,7 @@ public function applyConfiguratedValueToInputUsingAnswers( } $input['c_id'] = $block_id; + $provided_fields[] = $field->getID(); if ($field->fields['type'] == 'dropdown') { $field_name = 'plugin_fields_' . $field->fields['name'] . 'dropdowns_id'; } else { @@ -171,9 +180,29 @@ public function applyConfiguratedValueToInputUsingAnswers( } } + // Mandatory fields the form does not provide must not block the creation + self::$pending_item = [ + 'itemtype' => $this->itil_destination->getTarget()::class, + 'fields' => $provided_fields, + ]; + return $input; } + #[Override] + public function applyConfiguratedValueAfterDestinationCreation( + FormDestination $destination, + JsonFieldInterface $config, + AnswersSet $answers_set, + array $created_objects, + ): void { + // The destination is done: its items are validated like any other from now on + self::$pending_item = null; + foreach ($created_objects[$destination->getID()] ?? [] as $item) { + unset(self::$created_items[$item::class][$item->getID()]); + } + } + #[Override] public function getDefaultConfig(Form $form): SimpleValueConfig { diff --git a/templates/fields.html.twig b/templates/fields.html.twig index 08bc29bf..6ed1fe82 100644 --- a/templates/fields.html.twig +++ b/templates/fields.html.twig @@ -178,7 +178,7 @@ {% if field['multiple'] %} {% set dropdown_options = dropdown_options|merge({'multiple': true}) %} {% endif %} - {{ macros.dropdownField(field['dropdown_class'], input_name, value, label, field_options|merge(dropdown_options|default({}))) }} + {{ macros.dropdownField(field['dropdown_class'], input_name, dropdown_value, label, field_options|merge(dropdown_options|default({}))) }} {% elseif type == 'glpi_item' %} {% if not massiveaction %} diff --git a/tests/Units/ContainerItemRightTest.php b/tests/Units/ContainerItemRightTest.php index ae31b33c..3b588d1b 100644 --- a/tests/Units/ContainerItemRightTest.php +++ b/tests/Units/ContainerItemRightTest.php @@ -37,6 +37,7 @@ use Glpi\Tests\DbTestCase; use Glpi\Tests\GLPITestCase; use GlpiPlugin\Field\Tests\FieldTestTrait; +use Location; use PluginFieldsContainer; use PluginFieldsField; use PluginFieldsProfile; @@ -158,6 +159,43 @@ public function testShowDomContainerRendersReadOnlyFieldsWithoutUpdateRight(): v ); } + public function testShowDomContainerRendersEmptyOptionForMandatoryDropdowns(): void + { + $entity_id = getItemByTypeName(Entity::class, '_test_root_entity', true); + $this->setEntity($entity_id, true); + + $container = $this->createFieldContainer([ + 'label' => 'Dom container ' . $this->getUniqueString(), + 'type' => 'dom', + 'itemtypes' => [Computer::class], + 'is_active' => 1, + 'entities_id' => $entity_id, + 'is_recursive' => 1, + ]); + foreach (['dropdown', 'dropdown-' . Location::class] as $ranking => $type) { + $this->createField([ + 'label' => 'Mandatory ' . $type, + 'type' => $type, + PluginFieldsContainer::getForeignKeyField() => $container->getID(), + 'ranking' => $ranking + 1, + 'is_active' => 1, + 'is_readonly' => 0, + 'mandatory' => 1, + ]); + } + + $computer = $this->createItem(Computer::class, [ + 'name' => 'Computer ' . $this->getUniqueString(), + 'entities_id' => $entity_id, + ]); + + // Without its empty option, a mandatory dropdown without value makes the save a silent no-op + $this->assertSame( + 2, + substr_count($this->renderDomContainer($container->getID(), $computer), '