Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
accognet wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions application/cmdbabstract.class.inc.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4263,9 +4263,30 @@ protected function PrepareValueFromPostedForm($sFormPrefix, $sAttCode, $sClass =
case 'Set':
case 'TagSet':
$sTagSetJson = utils::ReadPostedParam("attr_{$sFormPrefix}{$sAttCode}", null, 'raw_data');
if ($sTagSetJson !== null) { // bulk modify, direct linked set not handled
$value = json_decode($sTagSetJson, true);
}
if ($sTagSetJson !== null) { // bulk modify, direct linked set not handled
$value = json_decode($sTagSetJson, true);
if ($this->IsNew()) {
if ($value['orig_value'] != '' ) {
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

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'])) {

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@Molkobain Molkobain Jan 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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

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;
}
}
Comment on lines +4278 to +4282
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
foreach ($acurrentValues as $val) {
if ( !in_array($val, $value['orig_value']) && !in_array($val, $value['removed']) && !in_array($val, $value['added']) ) {
Comment on lines +4279 to +4284
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
Comment on lines +4279 to +4284
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
$value['removed'][] = $val;
Comment on lines +4268 to +4285
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
}
}
Comment on lines +4268 to +4287
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The code accesses $value['removed'] and $value['added'] arrays without first checking if these keys exist. According to the JSON structure from the widget and AttributeSet.php (lines 114-115), these keys should always be present, but defensive coding would be better. If the JSON is malformed or comes from an unexpected source, this could cause PHP warnings or errors. Consider using isset() checks or the null coalescing operator before accessing these array elements.

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;
}
}
if (is_array($value)) {
// Ensure expected keys exist and are arrays
$origValues = [];
if (isset($value['orig_value']) && is_array($value['orig_value'])) {
$origValues = $value['orig_value'];
}
$removedValues = [];
if (isset($value['removed']) && is_array($value['removed'])) {
$removedValues = $value['removed'];
}
$addedValues = [];
if (isset($value['added']) && is_array($value['added'])) {
$addedValues = $value['added'];
}
if ($this->IsNew()) {
if (!empty($origValues)) {
foreach ($origValues as $val) {
if (!in_array($val, $removedValues)) {
$addedValues[] = $val;
}
}
}
} else {
$acurrentValues = $this->Get($sAttCode)->GetValues();
foreach ($origValues as $val) {
if (!in_array($val, $acurrentValues)
&& !in_array($val, $removedValues)
&& !in_array($val, $addedValues)
) {
$addedValues[] = $val;
}
}
foreach ($acurrentValues as $val) {
if (!in_array($val, $origValues)
&& !in_array($val, $removedValues)
&& !in_array($val, $addedValues)
) {
$removedValues[] = $val;
}
}
}
// Write normalized values back to $value for further processing
$value['orig_value'] = $origValues;
$value['removed'] = $removedValues;
$value['added'] = $addedValues;

Copilot uses AI. Check for mistakes.
Comment on lines +4272 to +4287
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
Comment on lines +4277 to +4287
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

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'.

Copilot uses AI. Check for mistakes.
}
}
break;

default:
Expand Down