N°6071 - Prefill Tagset in transition displayed but not saved

[accognet]

internal

[Copilot]

Pull request overview

This pull request fixes issue N°6071 where prefilled TagSet values in transitions were being displayed but not saved when the form was submitted. The fix modifies the PrepareValueFromPostedForm method to properly handle the reconciliation between form-provided values (orig_value) and the actual state that should be saved.

Changes:

• Modified TagSet/Set handling in PrepareValueFromPostedForm to reconcile prefilled values with database state
• For new objects: moves non-removed items from orig_value into the added array
• For existing objects: reconciles form's orig_value with current database values, updating added and removed arrays accordingly

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The code directly appends to $value['added'] without initializing it if it doesn't exist. While the JSON structure from the widget should include this key, if $value['added'] is undefined, this will create a PHP notice. Use $value['added'] = $value['added'] ?? []; before the loop or initialize it at the start of each branch to ensure the array exists.

[Copilot]

There is inconsistent spacing in this condition. There's an extra space after the opening parenthesis (before the first !in_array) and there are double spaces before the closing parenthesis. For consistency with the rest of the codebase, format as: if (!in_array($val, $acurrentValues) && !in_array($val, $value['removed']) && !in_array($val, $value['added'])). The same issue appears on line 4284.

Suggested change

	if ( !in_array($val, $acurrentValues) && !in_array($val, $value['removed']) && !in_array($val, $value['added']) ) {
	$value['added'][] = $val;
	}
	}
	foreach ($acurrentValues as $val) {
	if ( !in_array($val, $value['orig_value']) && !in_array($val, $value['removed']) && !in_array($val, $value['added']) ) {
	if (!in_array($val, $acurrentValues) && !in_array($val, $value['removed']) && !in_array($val, $value['added'])) {
	$value['added'][] = $val;
	}
	}
	foreach ($acurrentValues as $val) {
	if (!in_array($val, $value['orig_value']) && !in_array($val, $value['removed']) && !in_array($val, $value['added'])) {

[Copilot]

The code performs multiple in_array() calls in nested loops which could be inefficient if the arrays are large. For better performance, especially with larger tag sets, consider converting $value['removed'] and $value['added'] to associative arrays using array_flip() before the loops, and then use isset() instead of in_array(). This would reduce complexity from O(n*m) to O(n) for each loop.

Suggested change

	if ($this->IsNew()) {
	if ($value['orig_value'] != '' ) {
	foreach ($value['orig_value'] as $val) {
	if (!in_array($val, $value['removed'])) {
	$value['added'][] = $val;
	}
	}
	}
	} else {
	$acurrentValues = $this->Get($sAttCode)->GetValues();
	foreach ($value['orig_value'] as $val) {
	if ( !in_array($val, $acurrentValues) && !in_array($val, $value['removed']) && !in_array($val, $value['added']) ) {
	$value['added'][] = $val;
	}
	}
	foreach ($acurrentValues as $val) {
	if ( !in_array($val, $value['orig_value']) && !in_array($val, $value['removed']) && !in_array($val, $value['added']) ) {
	$value['removed'][] = $val;
	// Ensure expected keys exist and are arrays
	if (!isset($value['orig_value']) || $value['orig_value'] === '') {
	$value['orig_value'] = [];
	}
	if (!isset($value['added']) || $value['added'] === '') {
	$value['added'] = [];
	}
	if (!isset($value['removed']) || $value['removed'] === '') {
	$value['removed'] = [];
	}
	// Precompute lookup maps for faster membership tests
	$removedMap = array_flip($value['removed']);
	$addedMap = array_flip($value['added']);
	if ($this->IsNew()) {
	if (!empty($value['orig_value'])) {
	foreach ($value['orig_value'] as $val) {
	if (!isset($removedMap[$val])) {
	$value['added'][] = $val;
	$addedMap[$val] = true;
	}
	}
	}
	} else {
	$acurrentValues = $this->Get($sAttCode)->GetValues();
	$currentMap = array_flip($acurrentValues);
	$origMap = array_flip($value['orig_value']);
	foreach ($value['orig_value'] as $val) {
	if (!isset($currentMap[$val]) && !isset($removedMap[$val]) && !isset($addedMap[$val])) {
	$value['added'][] = $val;
	$addedMap[$val] = true;
	}
	}
	foreach ($acurrentValues as $val) {
	if (!isset($origMap[$val]) && !isset($removedMap[$val]) && !isset($addedMap[$val])) {
	$value['removed'][] = $val;
	$removedMap[$val] = true;

[Copilot]

The check $value['orig_value'] != '' is insufficient and could cause errors. If orig_value is not set in the JSON (due to malformed data), this will generate a PHP notice. Additionally, comparing an array to an empty string is semantically incorrect. Use !empty($value['orig_value']) instead, which properly handles missing keys, null values, and empty arrays, and is more semantically appropriate for array validation.

Suggested change

	if ($value['orig_value'] != '' ) {
	if (!empty($value['orig_value'])) {

[Molkobain]

In this case, we rather use the \utils::StrLen($value['orig_value']) > 0 pattern.

[Copilot]

There is inconsistent spacing in this condition. There's an extra space after the opening parenthesis (before the first !in_array) and there are double spaces before the closing parenthesis. For consistency with the rest of the codebase, format as: if (!in_array($val, $value['orig_value']) && !in_array($val, $value['removed']) && !in_array($val, $value['added'])). The same issue appears on line 4279.

Suggested change

	if ( !in_array($val, $acurrentValues) && !in_array($val, $value['removed']) && !in_array($val, $value['added']) ) {
	$value['added'][] = $val;
	}
	}
	foreach ($acurrentValues as $val) {
	if ( !in_array($val, $value['orig_value']) && !in_array($val, $value['removed']) && !in_array($val, $value['added']) ) {
	if (!in_array($val, $acurrentValues) && !in_array($val, $value['removed']) && !in_array($val, $value['added'])) {
	$value['added'][] = $val;
	}
	}
	foreach ($acurrentValues as $val) {
	if (!in_array($val, $value['orig_value']) && !in_array($val, $value['removed']) && !in_array($val, $value['added'])) {

[Copilot]

The code iterates over $value['orig_value'] without first checking if it exists or is an array. This could cause a PHP error if 'orig_value' is not present in the JSON data. Add a check to ensure isset($value['orig_value']) && is_array($value['orig_value']) before the foreach loop.

[Copilot]

Variable naming is inconsistent: $acurrentValues uses a lowercase 'c' making it look like Hungarian notation but doesn't follow standard PHP conventions. For consistency with the rest of the codebase and to improve readability, use $aCurrentValues (with capital C) to follow the pattern seen elsewhere where array variables are prefixed with 'a'.

[v-dumas]

@greptileai can review this PR

[greptile-apps]

Greptile Summary

This PR fixes a bug where TagSet (and Set) attribute values pre-filled by a workflow transition were visible in the form but silently discarded on save. The root cause was that the transition-injected values ended up only in the orig_value field of the posted JSON delta, while added remained empty, so ormTagSet::ApplyDelta() had nothing to apply.

Key changes:

• Adds reconciliation logic inside PrepareValueFromPostedForm for the Set/TagSet cases.
• For new objects (IsNew()): every item in orig_value that is not in removed is appended to added.
• For existing objects: orig_value is diffed against live database values (GetValues()) to compute missing added/removed entries.

Minor issues noted (P2 only):

• The IsNew() branch is missing a !in_array($val, $value['added']) guard (present in the else branch), producing potential duplicates in added — harmless due to idempotent Add().
• The else branch iterates foreach ($value['orig_value'] as $val) without guarding against orig_value being an empty string \"\", unlike the IsNew() path.

Confidence Score: 5/5

Safe to merge; the bug fix is correct and both remaining findings are P2 style inconsistencies with no runtime impact.

The core reconciliation logic is sound for both new and existing objects. ormTagSet::Add and ormSet::Add are idempotent, making the missing duplicate guard harmless, and the missing foreach guard would at worst produce a PHP warning with no data loss. All remaining findings are P2.

application/cmdbabstract.class.inc.php — minor inconsistency between IsNew() branch and else branch guards.

Important Files Changed

Filename	Overview
application/cmdbabstract.class.inc.php	Adds reconciliation logic so prefilled TagSet/Set values from a transition are correctly saved; two minor guard inconsistencies noted in the new IsNew() branch and existing-object branch.

Sequence Diagram

sequenceDiagram
    participant Form as Transition Form (Browser)
    participant PHP as cmdbabstract::PrepareValueFromPostedForm
    participant ORM as ormTagSet / ormSet
    participant DB as Database

    Form->>PHP: POST attr_<code> = {orig_value, added, removed}
    Note over PHP: json_decode posted JSON
    alt IsNew()
        PHP->>PHP: foreach orig_value: if not in removed → push to added
    else Existing object
        PHP->>ORM: Get($sAttCode)->GetValues()
        ORM-->>PHP: currentValues[]
        PHP->>PHP: foreach orig_value: if not in currentValues && not in removed && not in added → push to added
        PHP->>PHP: foreach currentValues: if not in orig_value && not in removed && not in added → push to removed
    end
    PHP->>ORM: ApplyDelta({added, removed})
    ORM->>ORM: Add / Remove each item
    ORM->>DB: Persist final tag set

Loading

_{Reviews (1): Last reviewed commit: "N°6071 - Prefill Tagset in transition di..." | Re-trigger Greptile}