Custom asset fields

[cconard96]

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-

Still very early WIP, but has at least basic functionality as a proof of concept and allows starting to test performance.

Supported field types (mainly based on Fields plugin types):

• [x] String (short text)
  • [x] Will be limited to 255 characters
• [x] Text (long text)
• [x] Rich text - Maybe just a flag for the text type
• [x] Number - Needs min. max, and step options
• [x] URL
• [x] Date
• [x] Date and time
• [x] Dropdown - Initial plans are for this to allow choosing a GLPI itemtype or nothing. If nothing is selected for the itemtype, the user can define their own static set of allowed values. Custom assets are part of the allowed itemtypes for more advanced support.
• [x] Yes/No
• [x] Placeholder - Only affects the form layout. Uses the nullField template. Not important now, but will be more useful when fields can be re-ordered.

Fields will be allowed to be defined as being required or readonly.

UI/UX Notes:

• The internal name for the custom fields are currently automatically generated in the web form based on the default label. If the field type is date or datetime, a date_ is prefixed to the internal name to match how fields are named in GLPI itself.
• No support currently for showing custom fields in a new tab like the Fields plugin. All custom fields show at the end of the asset form.

Technical features:

• [x] Automatic search option generation. The ID is currently 4500 + the ID for the field definition
• [x] When new fields are defined, it is intended that nothing is added to the custom_fields_json column for existing items of that asset type. The default value will be transparently used in those cases until the asset item is saved again.
• [x] Search options fill in default values when individual values are missing

[cconard96]

Just one thing, btw, the first displayed input is the system name, it's complicated to understant as the field, although set as read-only is not identifiable as non editable (a global issue in GLPI main we should address) but also is deducted from label input. I suggest to display the label field first, and make system name just for information (in a HtmlField rather Input in read only)

I'm overriding the form_fields block rather than using more_fields block, so I control where the field goes. Would it be desired to give users the ability to set this field themselves? Right now it is just auto-generated for convenience. There shouldn't be any issue with users changing the field name except that it would break compatibility of API integrations. Everything added by this PR refers to the fields by ID in the DB.

[orthagh]

Would it be desired to give users the ability to set this field themselves

Not sure it adds something useful, let's keep the information visible (displayed in second), just avoid the field input display

[cconard96]

* "Custom Field" tab should be placed 3rd (after "Capacities") and later renamed "Fields"

Done

* "Uncaught Exception Error: Call to undefined function Glpi\Asset_() in /var/www/glpi/src/Glpi/Asset/CustomField.php at line 252" when going to the "Custom fields" tab for the first time

I can't recreate the issue and the referenced line number doesn't line up to anything that makes sense.

* name field should be labelled "System name"

Done

* The full width toggle is cool, but I think it's more related to what he discussed about (all) fields reordering/removing/adding

That was the plan. I used it to make the richtext field usable while testing the creation/functionality of each field type though.

* the following could be done later in another PR, but UX for the fields properties could be maybe shared, at least partially, with form questions (ping @AdrienClairembault).

I'm not so sure about using anything directly from that.

Especially the options setup for dropdowns is better in the current form implementation.

Its just how select2 natively supports custom options.

We may also have icons for field types (also done in forms questions), date fields also have a current time toggle.

For "current time" toggle, it would require changing how default values are handled and requiring a mass update of every existing asset of the type to set the value if it isn't already. Right now, the default value is used as a fallback until the asset form is saved again or the value is specifically set in the API. Again, it wasn't really in the spec and also doesn't seem to have existed in the plugin.

[cconard96]

Unrelated to custom fields but on the topic of the Forms feature, the UI isn't working properly in any dark themes. I guess some elements are using hard-coded colors instead of variables from the framework.

[cedric-anne]

I suashed commits, rebased, and fix some issues.

I removed the rich text fields as it requires more work (the default value should be editable in a rich text editor, files and documents should be processed, ...). It could be done in a later PR, to be able to finalize the current PR ASAP.

I will continue the review this afternoon.

@cconard96 Could you check the failing E2E test? It is related to flatpicker inputs that does not seems to be targetable by their label.

[cconard96]

It is related to flatpicker inputs that does not seems to be targetable by their label.

Probably. See note in #17323. "Input labels broken when using altInput option".

[cedric-anne]

I think there is a big issue with the lack of polymorphism regarding fields types. We have condition on specific types everywhere + many match and switch depending on types.

Indeed, I think the fields types should each have an own class to be able to give the ability to the plugins to register new fields types. I add it to the backlog, we could refactor this later.

[cedric-anne]

I think there is a big issue with the lack of polymorphism regarding fields types. We have condition on specific types everywhere + many match and switch depending on types.

Indeed, I think the fields types should each have an own class to be able to give the ability to the plugins to register new fields types. I add it to the backlog, we could refactor this later.

I realize that it is indeed very complicated to review the code, because logics specific to specific types are mixed with global logics. I will try to define what would be needed as an interface and see if it is possible to modify this point without spending too much time on it.

[cconard96]

I think there is a big issue with the lack of polymorphism regarding fields types. We have condition on specific types everywhere + many match and switch depending on types.

Indeed, I think the fields types should each have an own class to be able to give the ability to the plugins to register new fields types. I add it to the backlog, we could refactor this later.

There are 3 match statements added. That's it. 2 for data validation/normalization and 1 for matching search option data types when they don't match the field type name. Also, 1 set of if/else statements to render the correct Twig macro for the field type. The only real odd case are placeholder fields since they have no data and are just a way to add a blank/null field to the form to help align the other fields. It isn't a maintainability issue. Having another 10+ classes that do basically the same thing would be overkill.

I don't see any reason why plugins would need to add new field types at this point. They didn't have that ability in the fields plugin and if there is somehow a special type of field we don't already support, it should probably be added in the core anyways.

We have very different views on what is complex logic here and this is basically the same argument that killed the Advanced Config UI PR.

[cedric-anne]

I don't see any reason why plugins would need to add new field types at this point. They didn't have that ability in the fields plugin and if there is somehow a special type of field we don't already support, it should probably be added in the core anyways.

The fields plugin does not allow this, but it does not mean that there are no plugin that implemented its own logic to propose additionnal fields to some forms. There are many use cases for this:

• a choice between hardcoded values;
• a choice between values that are populated from a remote service (e.g. a list of documents retrieved from a EDMS, a list of wiki pages, a list of invoices fetched from an ERP, ...);
• a text field that where the user would paste the URL of an external media and that would be rendered with alink to a media player;
• ... All of these use cases have nothing to do in the GLPI core code.

For now, plugins have to do this kind of stuffs by displaying the fileld with a hook, validate and store the data with another hook, use their own table in the database, and maybe also have their own configuration page to list the itemtypes that should handle these fields... We could simplify their work by giving them the ability to just a register a class that would implement a given interface, with no more need to have their own table nor their own configuration page.

Everytime we simplify the plugins developpers job, we increase the probability to see a new feature published in a plugin, and therefore the likelihood that GLPI will meet the needs of the community, and attract new users.

We have very different views on what is complex logic here

We did not say that logic is complex. But having to jump between multiple files to be able to understand how a specific field type is displayed, have its default value handled, is validated, ... is harder to read than having all the specific code to this specific type handled in a single class. The logic would not be different, but it would be placed in other files and normalized by a dedicated interface.

This is the Single-responsibility principle, and it often make the code easier to read, and easier to test.

[cedric-anne]

Here is an example of what could be an interface to move the code of each type/option to a dedicated class.

namespace Glpi\Asset\CustomFieldType;

interface CustomFieldTypeInterface
{
    /**
     * Get the field type name.
     */
    public function getName(): string;

    /**
     * Get the HTML code to use to display the custom field input in the asset form.
     *
     * @param mixed $name The field name.
     * @param mixed $value The current value (or the default value).
     */
    public function getFormInput(string $name, mixed $value): string;

    /**
     * Normalize the value submitted using the input generated by `self::getFormInput()`.
     *
     * @param mixed $value
     * @throws \InvalidArgumentException Thrown if the submitted value does not correspond to a valid value.
     */
    public function normalizeValue(mixed $value): mixed;

    /**
     * Get the field options.
     *
     * @return CustomFieldTypeOptionInterface[]
     */
    public function getOptions(): array;

    /**
     * Defines configured default value.
     */
    public function setDefaultValue(mixed $value): void;

    /**
     * Get the configured default value.
     */
    public function getDefaultValue(): mixed;

    /**
     * Get the HTML code to use to display the default value input in the configuration form.
     *
     * @param mixed $default_value The current default value.
     */
    public function getDefaultValueFormInput(mixed $default_value): string;

    /**
     * Normalize the default value submitted using the input generated by `self::getDefaultValueFormInput()`.
     *
     * @param mixed $default_value The submitted value.
     */
    public function normalizeDefaultValue(mixed $default_value): mixed;

    /**
     * Get the field search option specs to be added to the result of `\Glpi\Asset\Asset::rawSearchOptions()`.
     */
    public function getSearchOption(): array;

    /**
     * @see \CommonDBTM::getSpecificValueToSelect()
     */
    public function getSpecificValueToSelect(string $field, string $name, array $values, array $options): string;

    /**
     * @see \CommonDBTM::getSpecificValueToDisplay()
     */
    public function getSpecificValueToDisplay(string $field, array $values, array $options): string;
}


interface CustomFieldTypeOptionInterface
{
    /**
     * Get the option name.
     */
    public function getName(): string;

    /**
     * Get the option key.
     */
    public function getKey(): string;

    /**
     * Defines configured value.
     */
    public function setValue(mixed $value): void;

    /**
     * Get the configured value.
     */
    public function getValue(): mixed;

    /**
     * Get the HTML code to use to display the option input in the custom field form.
     * @param mixed $name The field name.
     * @param mixed $value The current value.
     */
    public function getFormInput(string $name, mixed $value): string;

    /**
     * Normalize the value submitted using the input generated by `self::getFormInput()`.
     *
     * @param mixed $value
     * @throws \InvalidArgumentException Thrown if the submitted value does not correspond to a valid value.
     */
    public function normalizeValue(mixed $value): mixed;

    /**
     * Get the configured value.
     */
    public function getValue(): string;

    /**
     * Get the name corresponding to the given value.
     */
    public function getValueName(mixed $value): string;
}

and a class would be something like that:

class IntegerFieldType extends AbstractCustomFieldType implements CustomFieldTypeInterface
{
    public function getName(): string
    {
        return __('Integer number');
    }

    public function getFormInput(sring $name, mixed $value): string
    {
        $template = <<<TWIG
            {% import 'components/form/fields_macros.html.twig' as fields %}

            {{ fields.numberField(
                name,
                $value,
                "",
                {
                    ...
                }
            ) }}
TWIG;

        $twig = TemplateRenderer::getInstance();
        return $twig->renderFromStringTemplate($template, [
            'name' => $name,
            'value' => $value,
        ]);
    }

    public function normalizeValue(mixed $value): mixed
    {
        if (!is_number($value)) {
            throw new \InvalidArgumentException();
        }

        // TODO validate min/max

        return (int) $value;
    }

    public function getOptions(): array
    {
        return [
            new IntegerOption(key: 'min', label: __('Minimum value')),
            new IntegerOption(key: 'max', label: __('Maximum value')),
        ];
    }

    // ...
}

The idea is simple. Once the code correctly separated between the global logic and the specific logic, adding a new field type should not require much than adding just a new class (and new classes for new options types if any). It could sometimes require to adapt the interface and the existing implementations, but this is generally happening only when the first implementations are made, beacause we may have misse something, or we may want to do small refactorings to enhance the code.

[trasher]

Just my two cents...

Indeed, I think the fields types should each have an own class to be able to give the ability to the plugins to register new fields types. I add it to the backlog, we could refactor this later.

I'd agree with that; that seems to me a better way to go.

You talked about advanced configuration where Cédric made a similar remark. It was event more true on that one; using a bunch of switch|if/else|match is what has been done for years in GLPI source code, and that often is not a good solution.

With proposed solution (see latest Cédric comment), it seems to me more clear and precise even if it does requires a bit more of work that - for current PR - would not strictly be required.

[cconard96]

There at least needs to be better planning for something like this and not a custom fields specific design.

We would have a dozen or so classes to define different field and option types for custom fields, a dozen or so classes to define again different types of config options for the advanced config UI feature, more duplicates for defining question types in forms, and if we ever define regular form fields in a more structured way there would be yet another duplicate set of classes.

[cedric-anne]

There at least needs to be better planning for something like this and not a custom fields specific design.

We would have a dozen or so classes to define different field and option types for custom fields, a dozen or so classes to define again different types of config options for the advanced config UI feature, more duplicates for defining question types in forms, and if we ever define regular form fields in a more structured way there would be yet another duplicate set of classes.

Custom fields types, form fields types and config fields types are probably similar in some points, but may still have huge differences. Trying to figure out how we can mutualize the corresponding code right now is a bit complicated, as each feature may still evolve. Also, the duplicated code will probably be related to only small simple code parts. Maybe we could later mutalize them in a parent interface/abstract class that provides the common parts for each feature.

[cconard96]

There at least needs to be better planning for something like this and not a custom fields specific design. We would have a dozen or so classes to define different field and option types for custom fields, a dozen or so classes to define again different types of config options for the advanced config UI feature, more duplicates for defining question types in forms, and if we ever define regular form fields in a more structured way there would be yet another duplicate set of classes.

Custom fields types, form fields types and config fields types are probably similar in some points, but may still have huge differences. Trying to figure out how we can mutualize the corresponding code right now is a bit complicated, as each feature may still evolve. Also, the duplicated code will probably be related to only small simple code parts. Maybe we could later mutalize them in a parent interface/abstract class that provides the common parts for each feature.

I guess at a later point (AKA not GLPI 11), we could look at adding Symfony Forms to clean up how forms and fields are handled throughout the project.

[cedric-anne]

I guess at a later point (AKA not GLPI 11), we could look at adding Symfony Forms to clean up how forms and fields are handled throughout the project.

Yes, it could be an option, but using Symfony Forms in GLPI is probably something we will not be able to do even in the following major version.

[cedric-anne]

I added 2 fixes and I rebased the branch to be sure that PHPStan level 4 checks are done.

[cedric-anne]

1. In the Add a new field form, the Default value field still does not work as expected. First, it is readonly, so no value can be set. Second, whatever the selected Type is, the field remains a text input. User would expect this field to be a date picker for date fields, a rich text editor for text fields, ...

2. I created a Number field, with a max of 150, a min of -50 and a step of 5. These options are correctly applied in the asset creation form (<input type="number" ... value="0" min="-50" max="150" step="5.00">), but when I open the field definition update form, all these options have a 0 value.

Also, on an asset that exists previously, I get an exception when opening its form:

The value must be a number
In /var/www/glpi/src/Glpi/Asset/CustomFieldType/NumberType.php::80
#0 /var/www/glpi/src/Glpi/Asset/CustomFieldType/AbstractType.php(56): Glpi\Asset\CustomFieldType\NumberType->normalizeValue('2024-09-05')
#1 /var/www/glpi/src/Glpi/Asset/Asset.php(500): Glpi\Asset\CustomFieldType\AbstractType->formatValueFromDB('2024-09-05')
#2 /var/www/glpi/src/CommonDBTM.php(303): Glpi\Asset\Asset->post_getFromDB()
#3 /var/www/glpi/src/Glpi/Asset/Asset.php(150): CommonDBTM->getFromDB(1)
#4 /var/www/glpi/front/asset/asset.form.php(46): Glpi\Asset\Asset::getById(1)
#5 /var/www/glpi/src/Glpi/Controller/LegacyFileLoadController.php(58): require('/var/www/glpi/f...')
#6 /var/www/glpi/vendor/symfony/http-kernel/HttpKernel.php(101): Glpi\Controller\LegacyFileLoadController->Glpi\Controller\{closure}()
#7 /var/www/glpi/vendor/symfony/http-foundation/StreamedResponse.php(106): Symfony\Component\HttpKernel\HttpKernel::Symfony\Component\HttpKernel\{closure}()
#8 /var/www/glpi/vendor/symfony/http-foundation/Response.php(423): Symfony\Component\HttpFoundation\StreamedResponse->sendContent()
#9 /var/www/glpi/public/index.php(58): Symfony\Component\HttpFoundation\Response->send()
#10 {main}

3. It seems to be possible to change the type of a field. IMHO, we should not permit that, at least for now, as it may requires many additional code/test. For instance, if I try to switch from a Number to a Dropdown, an error occurs:

Twig Error (Twig\Error\RuntimeError): "An exception has been thrown during the rendering of a template ("array_diff(): Argument #2 must be of type array, string given")." in template "" at line 2

Also, if I switch to a Date type, the system name is changed. This field should be read-only and a check should on PHP side to prevent this (as it is done for the system_name of an asset definition).

4. When I try to update a custom field definition option (I have only one at this moment), a The system name must be unique among fields for this asset definition error message is shown and no update is done.

5. On a Date field with default_value=NULL in DB, the update form displays the current date in the Default value form. The field should remains empty if no default value is set.

[cconard96]

1. The options/default values seems to be loaded correctly on the field definition update form, but they are not loaded anyomre on the field definition creation form.

Should be fixed.

2. When a field is defined as mandatory and/or readonly, these options are also applied to the default value field. They should not.

I added a param to options to define if they apply to the default value field (true by default) and removed the special handling done previously just for the "full_width" option.

3. The default value field for a `Number` type applies the current min/max/step values. When these are updated, it may produce an unexepected behaviour:

This has been fixed.

4. The `itemtype` of a `Dropdown` field is not configurable (there is no field displayed for that).

Fixed. Check was using the old "dropdown" key instead of the class name.

5. The `Text` field seems to be similar to the `String` type, I guess it is an error. In the `fields` plugin, there was 3 fields types for a text: `Text (single line)` (a basic input), `Text (multiple lines)` (a textarea) and `Rich Text` (a TinyMCE editor). We may implement the `Rich Text` type later, as it may not be simple as the ther fields, but I think that we should implement the 2 others right now.

String - Plain-text only and limited to 255 characters Text - Multiline/Long text. Rich text was, and I guess will be when it gets re-added, an option on this field type.

6. Deleting a field definition does not seems to work. I get a blank page.

Fixed

7. When I submit the asset form containaing a field of each type, I get the following SQL error:

I don't get this SQL error

8. Other fields types are not logging anything in the `Historical` tab when they are updated.

Not yet addressed. Also not sure why it says a dropdown was updated when the values haven't changed.

Also, this is not a bug, but there is currently no way to disable the min/max/step on a Number field. I do not know if it is a problem, but I guess some people will complain about that. These options were not available, and therefore not mandatory in the fields plugin. People could probably use max=999999999 min=-999999999999 step=0.0001 as a workaround.

Workaround added by supporting a default value for options.

[cedric-anne]

7. When I submit the asset form containaing a field of each type, I get the following SQL error:

I don't get this SQL error

Do you have the Historical capacity enabled on your custom asset definition ?

[cconard96]

The custom field update form does not load anymore

Test again on latest commit with a new asset definition/field definition to ensure the right ID is used in the AJAX request and the field definition is using the new "type" values.

[cconard96]

7. When I submit the asset form containaing a field of each type, I get the following SQL error:

I don't get this SQL error

Do you have the Historical capacity enabled on your custom asset definition ?

Yes

[cedric-anne]

The custom field update form does not load anymore

Test again on latest commit with a new asset definition/field definition to ensure the right ID is used in the AJAX request and the field definition is using the new "type" values.

I still have this issue:

Payload is:

[orthagh]

A quick functional review:

• when adding a new field, the "add a new field" button disappears (it's ok), but you can't cancel the addition without reloading the page

  To avoid that, I suggest moving the form into a modal (same for the edit action), it will fix every behavior related to canceling the action

• yes/no could be rendered as a slider or at least an option for a slider could be appreciated
• The display order of the fields is strange, but I guess we will fix that later in the dedicated task of reordering all fields (including default ones)
• full-width option -> We need to do something about that, this is a global issue for this option in GLPI. It makes fields not aligned and it's ugly.

I didn't encounter anything blocking regarding usage, no bugs, and no errors in logs. Apart from the above-mentioned UX issues, everything seems robust

[cconard96]

* when adding a new field, the "add a new field" button disappears (it's ok), but you can't cancel the addition without reloading the page
  > To avoid that, I suggest moving the form into a modal (same for the edit action), it will fix every behavior related to canceling the action

Yeah, this is more a global issue with the inline forms/sublist combo tabs. If the add/edit is inline when the form isn't too complex, it is good UX as opposed to using a modal. With the UI work I did on Rules, the Add button is always visible. I've removed the code in the new template I started in this PR to standardize "viewsubitem" form display that had hidden the "Add" button after clicking it.

* yes/no could be rendered as a slider or at least an option for a slider could be appreciated

Done

* full-width option -> We need to do something about that, this is a global issue for this option in GLPI. It makes fields not aligned and it's ugly.

Yeah, I've noticed and there are a ton of 'hacks' in templates that have full width fields to fix label/input sizes. Probably good to do in a separate PR.

[AdrienClairembault]

I can add multiple values for a dropdown where "multiple" is not enabled:

[AdrienClairembault]

Toggling the "mandatory" or "read only" option on a dropdown with a default value triggers a fatal error:

[AdrienClairembault]

On a dropfield field, I can add an invalid empty value:

If I then switch the type, I have a fatal error:

PS: to reproduce you need to have 0 monitor on your GLPI

[AdrienClairembault]

Dropdown with readonly + multiple values rendering seems broken:

[AdrienClairembault]

The mandatory "red dot" is only added for dropdown when previewing fields. Same for readonly, the input is only made readonly for dropdowns, anything else can be edited.