Skip to content

Fix - Massive action update on CustomAssets - #1247

Open
RomainLvr wants to merge 2 commits into
mainfrom
fix/massive-action-custom-asset-dropdown-fields
Open

Fix - Massive action update on CustomAssets#1247
RomainLvr wants to merge 2 commits into
mainfrom
fix/massive-action-custom-asset-dropdown-fields

Conversation

@RomainLvr

@RomainLvr RomainLvr commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Checklist before requesting a review

Please delete options that are not relevant.

  • I have performed a self-review of my code.
  • I have added tests (when available) that prove my fix is effective or that my feature works.
  • I have updated the CHANGELOG with a short functional description of the fix or new feature.
  • This change requires a documentation update.

Description

  • It fixes !45865
  • Here is a brief description of what this PR does

Massive action "Update" was broken for Fields-plugin fields attached to Custom Assets:

  1. The value input never displayed. PluginFieldsField::showSingle() looks up the field's container with a LIKE query against containers.itemtypes, but didn't escape backslashes the same way PluginFieldsContainer::getAddSearchOptions() already does. Since CustomAsset itemtypes are namespaced classes (e.g. Glpi\CustomAsset\Transmission), the query never matched, so the field never rendered, only the "Send" button showed, and submitting produced an Undefined array key warning.
  2. hook.php masked the failure. plugin_fields_MassiveActionsFieldsDisplay() ignored showSingle()'s return value and always returned true, preventing GLPI core's fallback renderer from ever kicking in.
  3. Multi-select "GLPI item" dropdown fields got wiped. A field of type dropdown-<Itemtype> (e.g. a multi-select of Locations) with multiple enabled was silently emptied whenever a different field in the same container was mass-updated afterwards. The plain dropdown type already had a guard against this (fixed in fix: multiple dropdown fields emptied when solution added #795 / fix(MassiveAction): fix incompatibility with multiple dropdowns #974), but it was missing on this sibling code path.

Fix

  • Apply the same backslash-escaping logic in showSingle() as in getAddSearchOptions().
  • Make hook.php return showSingle()'s actual result.
  • Apply the same "don't blank an untouched field" guard to the dropdown-<Itemtype> multi-value branch in PluginFieldsContainer::populateData().

Test plan

  • The test only passes if the patch Fix - Avoid array-to-string warnings on multi-value massive action fields glpi-project/glpi#25210 is applied otherwise, the error ‘Converting an array to a string’ causes the tests to fail.

  • Added regression tests :

    • MassiveActionCustomAssetTest => field display for CustomAssets.
    • MassiveActionMultipleDropdownTest / MassiveActionRealFlowTest => plain dropdown multiple field survives a sibling field's massive update.
    • MassiveActionGlpiItemDropdownTest => dropdown-<Itemtype> multiple field survives a sibling field's massive update (fails without the fix, passes with it).

PS : i've added psalm.xml because it was missing from make test process

@RomainLvr
RomainLvr requested review from Rom1-B and stonebuzz August 20, 2026 12:57
@RomainLvr RomainLvr self-assigned this Aug 20, 2026
@RomainLvr RomainLvr added the bug label Aug 20, 2026
Rom1-B

This comment was marked as resolved.

Comment thread inc/container.class.php
Comment on lines +2111 to +2115
} elseif (isset($_REQUEST['massiveaction'])) { // called from massiveaction
if (isset($_POST[$field['name']])) {
$data[$field['name']] = $_POST[$field['name']];
$has_fields = true;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} elseif (isset($_REQUEST['massiveaction'])) { // called from massiveaction
if (isset($_POST[$field['name']])) {
$data[$field['name']] = $_POST[$field['name']];
$has_fields = true;
}
} elseif (
isset($_REQUEST['massiveaction'])
&& (isset($_POST[$field['name']])
) {
$data[$field['name']] = $_POST[$field['name']];
$has_fields = true;

Comment thread psalm.xml
Comment on lines +6 to +8
<directory name="src"/>
<file name="hook.php"/>
<file name="setup.php"/>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to create a dedicated PR for PSALM to make it complete:

Suggested change
<directory name="src"/>
<file name="hook.php"/>
<file name="setup.php"/>
<directory name="ajax" />
<directory name="front" />
<directory name="inc" />
<directory name="src" />
<directory name="tests" />
<file name="hook.php" />
<file name="setup.php" />

Comment thread inc/field.class.php
Comment on lines +1367 to +1368
// But do not escape backslashes for CustomAsset, as they are alrady escaped
if (!str_contains($search_string, 'CustomAsset')) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// But do not escape backslashes for CustomAsset, as they are alrady escaped
if (!str_contains($search_string, 'CustomAsset')) {
// But do not escape backslashes for namespaced itemtypes, as they are already escaped
if (!str_contains((string) $itemtype, '\\')) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants