Skip to content

Commit 44e068c

Browse files
cedric-annecedric-anne
authored
Add a message when CommonDBChild item creation is refused (#21524)
* Add a message when `CommonDBChild` item creation is refused * fix dashboard getFromDB case * clean invalid values instead of unsetting them
1 parent 76ab7ff commit 44e068c

File tree

6 files changed

+222
-12
lines changed

6 files changed

+222
-12
lines changed

‎src/CommonDBChild.php‎

Lines changed: 31 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -436,12 +436,36 @@ public function prepareInputForAdd($input)
436436
return false;
437437
}
438438

439-
// Check item exists
440-
if (
441-
static::$mustBeAttached
442-
&& !$this->getItemFromArray(static::$itemtype, static::$items_id, $input)
443-
) {
444-
return false;
439+
if (!$this->getItemFromArray(static::$itemtype, static::$items_id, $input)) {
440+
// The parent item is invalid.
441+
442+
if (static::$mustBeAttached) {
443+
// A valid parent item is mandatory, so creation is blocked with an error message.
444+
$linked_itemtype = preg_match('/^itemtype/', static::$itemtype)
445+
? ($input[static::$itemtype] ?? null)
446+
: static::$itemtype
447+
;
448+
$linked_items_id = $input[static::$items_id] ?? null;
449+
450+
Session::addMessageAfterRedirect(
451+
htmlescape(sprintf(
452+
__('Parent item %s #%s is invalid.'),
453+
is_a($linked_itemtype, CommonDBTM::class, true) ? $linked_itemtype::getTypeName(1) : ($linked_itemtype ?? 'null'),
454+
$linked_items_id ?? 'null'
455+
)),
456+
false,
457+
ERROR
458+
);
459+
return false;
460+
} else {
461+
// A valid parent is not mandatory, so invalid input is cleaned.
462+
if (array_key_exists(static::$itemtype, $input) && preg_match('/^itemtype/', static::$itemtype)) {
463+
$input[static::$itemtype] = ''; // `itemtype` fields are usually not nullable, a default value must be set
464+
}
465+
if (array_key_exists(static::$items_id, $input)) {
466+
$input[static::$items_id] = 0; // foreign key fields may be not nullable, a default value must be set
467+
}
468+
}
445469
}
446470

447471
return $this->addNeededInfoToInput($input);
@@ -461,6 +485,7 @@ public function prepareInputForUpdate($input)
461485
static::$items_id,
462486
])
463487
) {
488+
// A message is already added by `self::checkAttachedItemChangesAllowed()`
464489
return false;
465490
}
466491

‎src/Glpi/Dashboard/Dashboard.php‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,33 @@ public function getFromDB($ID)
142142
);
143143
}
144144

145+
if (\is_numeric($ID)) {
146+
// Search also on the `id` field.
147+
// This is mandatory to handle the `$this->getFromDB($this->getID());` reload case.
148+
$iterator = $DB->request([
149+
'FROM' => self::getTable(),
150+
'WHERE' => [
151+
'id' => $ID,
152+
],
153+
'LIMIT' => 1,
154+
]);
155+
if (count($iterator) == 1) {
156+
$this->fields = $iterator->current();
157+
$this->key = $this->fields['key'];
158+
$this->post_getFromDB();
159+
return true;
160+
} elseif (count($iterator) > 1) {
161+
throw new TooManyResultsException(
162+
sprintf(
163+
'`%1$s::getFromDB()` expects to get one result, %2$s found in query "%3$s".',
164+
static::class,
165+
count($iterator),
166+
$iterator->getSql()
167+
)
168+
);
169+
}
170+
}
171+
145172
return false;
146173
}
147174

‎src/Glpi/Dashboard/Item.php‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -43,10 +43,6 @@ class Item extends CommonDBChild
4343
public static $itemtype = Dashboard::class;
4444
public static $items_id = 'dashboards_dashboards_id';
4545

46-
// prevent bad getFromDB when bootstraping tests suite
47-
// FIXME Should be true
48-
public static $mustBeAttached = false;
49-
5046
/**
5147
* Return items for the provided dashboard
5248
*

‎tests/functional/CommonDBChildTest.php‎

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
1+
<?php
2+
3+
/**
4+
* ---------------------------------------------------------------------
5+
*
6+
* GLPI - Gestionnaire Libre de Parc Informatique
7+
*
8+
* http://glpi-project.org
9+
*
10+
* @copyright 2015-2025 Teclib' and contributors.
11+
* @licence https://www.gnu.org/licenses/gpl-3.0.html
12+
*
13+
* ---------------------------------------------------------------------
14+
*
15+
* LICENSE
16+
*
17+
* This file is part of GLPI.
18+
*
19+
* This program is free software: you can redistribute it and/or modify
20+
* it under the terms of the GNU General Public License as published by
21+
* the Free Software Foundation, either version 3 of the License, or
22+
* (at your option) any later version.
23+
*
24+
* This program is distributed in the hope that it will be useful,
25+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
26+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
27+
* GNU General Public License for more details.
28+
*
29+
* You should have received a copy of the GNU General Public License
30+
* along with this program. If not, see <https://www.gnu.org/licenses/>.
31+
*
32+
* ---------------------------------------------------------------------
33+
*/
34+
35+
namespace tests\units;
36+
37+
use CommonDBChild;
38+
use Computer;
39+
use DbTestCase;
40+
41+
class CommonDBChildTest extends DbTestCase
42+
{
43+
public function testPrepareInputForAddWithMandatoryFkeyRelation(): void
44+
{
45+
$instance = new class extends CommonDBChild {
46+
public static $itemtype = Computer::class;
47+
public static $items_id = 'computers_id';
48+
public static $mustBeAttached = true;
49+
50+
public static $disableAutoEntityForwarding = true; // prevent DB accesses
51+
};
52+
53+
$this->assertFalse($instance->prepareInputForAdd(['foo' => 'bar']));
54+
$this->hasSessionMessages(ERROR, ['Parent item Computer #null is invalid.']);
55+
56+
$this->assertFalse($instance->prepareInputForAdd(['computers_id' => 9999999999, 'foo' => 'bar']));
57+
$this->hasSessionMessages(ERROR, ['Parent item Computer #9999999999 is invalid.']);
58+
59+
$valid_id = \getItemByTypeName(Computer::class, '_test_pc01', true);
60+
$this->assertEquals(
61+
['computers_id' => $valid_id, 'foo' => 'bar'],
62+
$instance->prepareInputForAdd(['computers_id' => $valid_id, 'foo' => 'bar'])
63+
);
64+
}
65+
66+
public function testPrepareInputForAddWithOptionalFkeyRelation(): void
67+
{
68+
$instance = new class extends CommonDBChild {
69+
public static $itemtype = Computer::class;
70+
public static $items_id = 'computers_id';
71+
public static $mustBeAttached = false;
72+
73+
public static $disableAutoEntityForwarding = true; // prevent DB accesses
74+
};
75+
76+
$this->assertEquals(
77+
['foo' => 'bar'],
78+
$instance->prepareInputForAdd(['foo' => 'bar'])
79+
);
80+
81+
$this->assertEquals(
82+
['computers_id' => 0, 'foo' => 'bar'],
83+
$instance->prepareInputForAdd(['computers_id' => 9999999999, 'foo' => 'bar'])
84+
);
85+
86+
$valid_id = \getItemByTypeName(Computer::class, '_test_pc01', true);
87+
$this->assertEquals(
88+
['computers_id' => $valid_id, 'foo' => 'bar'],
89+
$instance->prepareInputForAdd(['computers_id' => $valid_id, 'foo' => 'bar'])
90+
);
91+
}
92+
public function testPrepareInputForAddWithMandatoryPolymorphicRelation(): void
93+
{
94+
$instance = new class extends CommonDBChild {
95+
public static $itemtype = 'itemtype';
96+
public static $items_id = 'items_id';
97+
public static $mustBeAttached = true;
98+
99+
public static $disableAutoEntityForwarding = true; // prevent DB accesses
100+
};
101+
102+
$this->assertFalse($instance->prepareInputForAdd(['foo' => 'bar']));
103+
$this->hasSessionMessages(ERROR, ['Parent item null #null is invalid.']);
104+
105+
$this->assertFalse($instance->prepareInputForAdd(['itemtype' => 'Computer', 'foo' => 'bar']));
106+
$this->hasSessionMessages(ERROR, ['Parent item Computer #null is invalid.']);
107+
108+
$this->assertFalse($instance->prepareInputForAdd(['itemtype' => 'NotAClass', 'foo' => 'bar']));
109+
$this->hasSessionMessages(ERROR, ['Parent item NotAClass #null is invalid.']);
110+
111+
$this->assertFalse($instance->prepareInputForAdd(['itemtype' => 'Computer', 'items_id' => 9999999999, 'foo' => 'bar']));
112+
$this->hasSessionMessages(ERROR, ['Parent item Computer #9999999999 is invalid.']);
113+
114+
$valid_id = \getItemByTypeName(Computer::class, '_test_pc01', true);
115+
$this->assertEquals(
116+
['itemtype' => 'Computer', 'items_id' => $valid_id, 'foo' => 'bar'],
117+
$instance->prepareInputForAdd(['itemtype' => 'Computer', 'items_id' => $valid_id, 'foo' => 'bar'])
118+
);
119+
}
120+
121+
public function testPrepareInputForAddWithOptionalPolymorphicRelation(): void
122+
{
123+
$instance = new class extends CommonDBChild {
124+
public static $itemtype = 'itemtype';
125+
public static $items_id = 'items_id';
126+
public static $mustBeAttached = false;
127+
128+
public static $disableAutoEntityForwarding = true; // prevent DB accesses
129+
};
130+
131+
$this->assertEquals(
132+
['foo' => 'bar'],
133+
$instance->prepareInputForAdd(['foo' => 'bar'])
134+
);
135+
136+
$this->assertEquals(
137+
['items_id' => 0, 'foo' => 'bar'],
138+
$instance->prepareInputForAdd(['items_id' => 9999999999, 'foo' => 'bar'])
139+
);
140+
141+
$this->assertEquals(
142+
['itemtype' => '', 'foo' => 'bar'],
143+
$instance->prepareInputForAdd(['itemtype' => 'NotAClass', 'foo' => 'bar'])
144+
);
145+
146+
$this->assertEquals(
147+
['itemtype' => '', 'items_id' => 0, 'foo' => 'bar'],
148+
$instance->prepareInputForAdd(['itemtype' => 'Computer', 'items_id' => 9999999999, 'foo' => 'bar'])
149+
);
150+
151+
$valid_id = \getItemByTypeName(Computer::class, '_test_pc01', true);
152+
$this->assertEquals(
153+
['itemtype' => 'Computer', 'items_id' => $valid_id, 'foo' => 'bar'],
154+
$instance->prepareInputForAdd(['itemtype' => 'Computer', 'items_id' => $valid_id, 'foo' => 'bar'])
155+
);
156+
}
157+
}

‎tests/functional/Glpi/Dashboard/DashboardTest.php‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,11 +66,15 @@ public function testLoad()
6666

6767
public function testGetFromDB()
6868
{
69-
// we need to test we get the dashboard by it's key and not it's id
70-
$this->assertFalse($this->dashboard->getFromDB(1));
69+
// get the dashboard by its key
7170
$this->assertTrue($this->dashboard->getFromDB('test_dashboard'));
7271
$this->assertEquals('test_dashboard', $this->getPrivateProperty('key'));
7372
$this->assertNotEmpty($this->getPrivateProperty('fields'));
73+
74+
// get the dashboard by its id
75+
$dashboard = $this->createItem(Dashboard::class, ['key' => 'test_by_id', 'name' => __FUNCTION__]);
76+
$this->assertTrue($this->dashboard->getFromDB($dashboard->getID()));
77+
$this->assertEquals('test_by_id', $this->getPrivateProperty('key'));
7478
}
7579

7680

‎tests/functional/RuleCriteriaTest.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,7 @@ public function testPrepareInputForAdd()
204204
// Expects prepareInputForAdd to return false if `rules_id` is not a valid ID
205205
$input = ['rules_id' => $rules_id + 1000, 'criteria' => 'name'];
206206
$this->assertFalse($criteria->prepareInputForAdd($input));
207+
$this->hasSessionMessages(ERROR, ['Parent item Rule #' . ($rules_id + 1000) . ' is invalid.']);
207208
}
208209

209210
public function testGetSearchOptionsNew()

0 commit comments

Comments
 (0)