Skip to content

Commit fba29be

Browse files
LainowRom1-B
andauthored
Fix render error from duplicated form (#1301)
* Fix rendering error 'unknown named parameters' for duplicated form * Review * Rector * Add editor rendering test for string-keyed default value * Update inc/questiontype.class.php Co-authored-by: Romain B. <8530352+Rom1-B@users.noreply.github.com> --------- Co-authored-by: Romain B. <8530352+Rom1-B@users.noreply.github.com>
1 parent 4f2ec3a commit fba29be

6 files changed

Lines changed: 288 additions & 7 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
1111

1212
- Fix item creation failing when a Yes/No field has a non boolean default value
1313
- Fix mandatory fields of a block hidden by its display conditions blocking the save of an item
14+
- Fix rendering error `unknown named parameters` for duplicated form
1415
- Fix fields values updates not applying the expected checks in some cases
1516
- Fix `Yes/No` field values returned as strings instead of integers in search results
1617
- Fix ticket observers being able to edit and save additional fields they are not allowed to modify

‎hook.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ function plugin_fields_install()
7171
PluginFieldsField::class,
7272
PluginFieldsLabelTranslation::class,
7373
PluginFieldsProfile::class,
74+
PluginFieldsQuestionType::class,
7475
PluginFieldsStatusOverride::class,
7576
];
7677

‎inc/field.class.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1402,7 +1402,7 @@ public static function prepareHtmlFields(
14021402
$value = array_values(array_filter(
14031403
array_merge(...array_map(
14041404
static fn($v) => is_array($v) ? array_values($v) : [$v],
1405-
$value,
1405+
array_values($value),
14061406
)),
14071407
is_scalar(...),
14081408
));

‎inc/questiontype.class.php‎

Lines changed: 90 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
use Glpi\Form\Question;
3838
use Glpi\Form\QuestionType\AbstractQuestionType;
3939
use Glpi\Form\QuestionType\QuestionTypeCategoryInterface;
40+
use Safe\Exceptions\JsonException;
4041

4142
use function Safe\json_decode;
4243
use function Safe\json_encode;
@@ -85,6 +86,83 @@ public function formatDefaultValueForDB(mixed $value): string
8586
return json_encode($value);
8687
}
8788

89+
/**
90+
* Decode the stored default value of a cloned question, as it is passed
91+
* again to `formatDefaultValueForDB()` when the clone is added.
92+
*/
93+
public static function preQuestionAdd(Question $question): void
94+
{
95+
if (
96+
!($question->input['clone'] ?? false)
97+
|| ($question->input['type'] ?? null) !== self::class
98+
|| !is_string($question->input['default_value'] ?? null)
99+
) {
100+
return;
101+
}
102+
103+
try {
104+
$question->input['default_value'] = json_decode($question->input['default_value'], true);
105+
} catch (JsonException) {
106+
// Not a JSON value, keep it as is
107+
}
108+
}
109+
110+
/**
111+
* Repair default values that were JSON encoded several times by previous form clones.
112+
*/
113+
public static function installBaseData(Migration $migration, $version): void
114+
{
115+
/** @var DBmysql $DB */
116+
global $DB;
117+
118+
$question_table = Question::getTable();
119+
if (version_compare($version, '1.24.6', '>') || !$DB->tableExists($question_table)) {
120+
return;
121+
}
122+
123+
$migration->displayMessage(__('Repair default values of cloned form questions', 'fields'));
124+
125+
$questions = $DB->request([
126+
'SELECT' => ['id', 'default_value', 'extra_data'],
127+
'FROM' => $question_table,
128+
'WHERE' => ['type' => self::class],
129+
]);
130+
foreach ($questions as $question) {
131+
if (!is_string($question['default_value']) || !is_string($question['extra_data'])) {
132+
continue;
133+
}
134+
135+
$extra_data = \json_decode($question['extra_data'], true);
136+
$field = new PluginFieldsField();
137+
if (!is_array($extra_data) || !$field->getFromDB((int) ($extra_data['field_id'] ?? 0))) {
138+
continue;
139+
}
140+
141+
// Dropdowns and GLPI items store an array, other fields store a scalar value
142+
$is_array_value = $field->fields['type'] === 'glpi_item'
143+
|| str_starts_with((string) $field->fields['type'], 'dropdown');
144+
145+
$default_value = $question['default_value'];
146+
while (is_string($decoded = \json_decode($default_value, true))) {
147+
// Unwrap while the value is still a JSON encoded string (or array for array values)
148+
$inner = \json_decode($decoded, true);
149+
if (!is_string($inner) && (!$is_array_value || !is_array($inner))) {
150+
break;
151+
}
152+
153+
$default_value = $decoded;
154+
}
155+
156+
if ($default_value !== $question['default_value']) {
157+
$DB->update(
158+
$question_table,
159+
['default_value' => $default_value],
160+
['id' => $question['id']],
161+
);
162+
}
163+
}
164+
}
165+
88166
#[Override]
89167
public function validateExtraDataInput(array $input): bool
90168
{
@@ -130,8 +208,8 @@ public function renderAdministrationTemplate(?Question $question): string
130208

131209
// Compute default value for the field
132210
$default_value = null;
133-
if ($question instanceof Question && !empty($question->fields['default_value'])) {
134-
$default_value = json_decode($question->fields['default_value'], true);
211+
if ($question instanceof Question) {
212+
$default_value = $this->decodeDefaultValue($question);
135213
}
136214

137215
$twig = TemplateRenderer::getInstance();
@@ -145,6 +223,15 @@ public function renderAdministrationTemplate(?Question $question): string
145223
]);
146224
}
147225

226+
private function decodeDefaultValue(Question $question): mixed
227+
{
228+
if (empty($question->fields['default_value'])) {
229+
return null;
230+
}
231+
232+
return json_decode($question->fields['default_value'], true);
233+
}
234+
148235
#[Override]
149236
public function renderEndUserTemplate(Question $question): string
150237
{
@@ -165,10 +252,7 @@ public function renderEndUserTemplate(Question $question): string
165252
$current_field = PluginFieldsField::getById($current_field_id);
166253

167254
// Compute default value for the field
168-
$default_value = null;
169-
if (!empty($question->fields['default_value'])) {
170-
$default_value = json_decode($question->fields['default_value'], true);
171-
}
255+
$default_value = $this->decodeDefaultValue($question);
172256

173257
$itemtype = null;
174258
if (str_starts_with((string) $current_field->fields['type'], 'dropdown')) {

‎setup.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@
7575
use Glpi\Form\Destination\FormDestinationProblem;
7676
use Glpi\Form\Destination\FormDestinationTicket;
7777
use Glpi\Form\Migration\TypesConversionMapper;
78+
use Glpi\Form\Question;
7879
use Glpi\Form\QuestionType\QuestionTypesManager;
7980
use Glpi\Plugin\Hooks;
8081
use Symfony\Component\Yaml\Yaml;
@@ -134,6 +135,7 @@ function plugin_init_fields()
134135

135136
// Register fields question type
136137
plugin_fields_register_plugin_types();
138+
$PLUGIN_HOOKS['pre_item_add']['fields'][Question::class] = PluginFieldsQuestionType::preQuestionAdd(...);
137139

138140
if (Session::getLoginUserID() || isCommandLine()) {
139141
// Init hook about itemtype(s) for plugin fields

‎tests/Units/FieldQuestionTypeTest.php‎

Lines changed: 193 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030

3131
namespace GlpiPlugin\Field\Tests\Units;
3232

33+
use DBmysql;
3334
use Glpi\Form\Condition\ConditionHandler\ItemAsTextConditionHandler;
3435
use Glpi\Form\Condition\ConditionHandler\ItemConditionHandler;
3536
use Glpi\Form\Condition\Engine;
@@ -38,20 +39,24 @@
3839
use Glpi\Form\Condition\Type;
3940
use Glpi\Form\Condition\ValueOperator;
4041
use Glpi\Form\Condition\VisibilityStrategy;
42+
use Glpi\Form\Form;
4143
use Glpi\Form\Question;
4244
use Glpi\Form\QuestionType\QuestionTypeShortText;
4345
use Glpi\Form\QuestionType\QuestionTypesManager;
4446
use Glpi\Tests\FormBuilder;
4547
use GlpiPlugin\Field\Tests\QuestionTypeTestCase;
4648
use Location;
4749
use LogicException;
50+
use Migration;
51+
use PHPUnit\Framework\Attributes\DataProvider;
4852
use PluginFieldsContainer;
4953
use PluginFieldsDropdown;
5054
use PluginFieldsField;
5155
use PluginFieldsQuestionType;
5256
use PluginFieldsQuestionTypeCategory;
5357
use PluginFieldsQuestionTypeExtraDataConfig;
5458

59+
use function Safe\json_decode;
5560
use function Safe\json_encode;
5661

5762
final class FieldQuestionTypeTest extends QuestionTypeTestCase
@@ -242,6 +247,194 @@ public function testFormatRawAnswerForMultipleGlpiItemDropdown(): void
242247
}
243248
}
244249

250+
public function testFieldsQuestionEditorRenderingForMultipleDropdownWithStringKeyedDefaultValue(): void
251+
{
252+
$this->login();
253+
254+
// Arrange: create a multiple "dropdown-Location" field
255+
$this->fields['locations'] = $this->createField([
256+
'label' => 'Locations',
257+
'type' => 'dropdown-' . Location::class,
258+
'multiple' => 1,
259+
'default_value' => [],
260+
PluginFieldsContainer::getForeignKeyField() => $this->block->getID(),
261+
'ranking' => 2,
262+
'is_active' => 1,
263+
], ['default_value']);
264+
265+
$location1 = $this->createItem(Location::class, [
266+
'name' => __FUNCTION__ . ' 1',
267+
'entities_id' => $this->getTestRootEntity(true),
268+
]);
269+
$location2 = $this->createItem(Location::class, [
270+
'name' => __FUNCTION__ . ' 2',
271+
'entities_id' => $this->getTestRootEntity(true),
272+
]);
273+
274+
// Default value with string keys, as they would be passed as named parameters if not reindexed
275+
$builder = new FormBuilder("My form");
276+
$builder->addQuestion(
277+
"Locations question",
278+
PluginFieldsQuestionType::class,
279+
default_value: [
280+
'itemtype' => Location::class,
281+
'items_id' => [
282+
'first' => $location1->getID(),
283+
'second' => $location2->getID(),
284+
],
285+
],
286+
extra_data: json_encode($this->getFieldExtraDataConfig('locations')),
287+
);
288+
$form = $this->createForm($builder);
289+
290+
try {
291+
// Act: render form editor
292+
$crawler = $this->renderFormEditor($form);
293+
294+
// Assert: both locations are rendered as the default value
295+
$selected_values = $crawler
296+
->filter('.form-editor-container [data-glpi-form-editor-question] input[name^="default_value[items_id]"]')
297+
->each(fn($input) => (int) $input->attr('value'));
298+
$this->assertEqualsCanonicalizing(
299+
[$location1->getID(), $location2->getID()],
300+
$selected_values,
301+
);
302+
} finally {
303+
// delete locations for another run
304+
$location1->delete($location1->fields, true);
305+
$location2->delete($location2->fields, true);
306+
}
307+
}
308+
309+
public static function cloneDefaultValueProvider(): iterable
310+
{
311+
yield 'text' => ['text', 'hello'];
312+
yield 'textarea' => ['textarea', "first line\nsecond line"];
313+
yield 'number' => ['number', '42'];
314+
yield 'url' => ['url', 'https://glpi-project.org'];
315+
yield 'date' => ['date', '2026-10-15'];
316+
yield 'json-like text' => ['text', '[1,2]'];
317+
}
318+
319+
#[DataProvider('cloneDefaultValueProvider')]
320+
public function testCloneKeepsScalarDefaultValue(string $type, string $default_value): void
321+
{
322+
$this->login();
323+
324+
// Arrange: create a form with a question using a scalar field
325+
$this->fields['scalar'] = $this->createField([
326+
'label' => 'Scalar field',
327+
'type' => $type,
328+
PluginFieldsContainer::getForeignKeyField() => $this->block->getID(),
329+
'ranking' => 2,
330+
'is_active' => 1,
331+
]);
332+
$builder = new FormBuilder("My form");
333+
$builder->addQuestion(
334+
"Scalar question",
335+
PluginFieldsQuestionType::class,
336+
default_value: $default_value,
337+
extra_data: json_encode($this->getFieldExtraDataConfig('scalar')),
338+
);
339+
$form = $this->createForm($builder);
340+
341+
// Act: clone the form, then clone the clone
342+
$clone = Form::getById($form->clone());
343+
$clone_of_clone = Form::getById($clone->clone());
344+
345+
// Assert: the default value is encoded only once in each form
346+
foreach ([$form, $clone, $clone_of_clone] as $current_form) {
347+
$question = Question::getById($this->getQuestionId($current_form, "Scalar question"));
348+
$this->assertSame(json_encode($default_value), $question->fields['default_value']);
349+
}
350+
}
351+
352+
public function testCloneKeepsDropdownDefaultValue(): void
353+
{
354+
$this->login();
355+
356+
// Arrange: create a form with a dropdown question having a default value
357+
$itemtype = PluginFieldsDropdown::getClassname($this->fields['dropdown']->fields['name']);
358+
$option = $this->createItem($itemtype, ['name' => 'Default option']);
359+
$default_value = ['itemtype' => $itemtype, 'items_id' => $option->getID()];
360+
361+
$builder = new FormBuilder("My form");
362+
$builder->addQuestion(
363+
"Dropdown question",
364+
PluginFieldsQuestionType::class,
365+
default_value: $default_value,
366+
extra_data: json_encode($this->getFieldExtraDataConfig('dropdown')),
367+
);
368+
$form = $this->createForm($builder);
369+
370+
// Act: clone the form, then clone the clone
371+
$clone = Form::getById($form->clone());
372+
$clone_of_clone = Form::getById($clone->clone());
373+
374+
// Assert: the default value is kept as an array in each form
375+
foreach ([$form, $clone, $clone_of_clone] as $current_form) {
376+
$question = Question::getById($this->getQuestionId($current_form, "Dropdown question"));
377+
$this->assertSame($default_value, json_decode($question->fields['default_value'], true));
378+
}
379+
}
380+
381+
public function testInstallRepairsDefaultValuesEncodedByClones(): void
382+
{
383+
/** @var DBmysql $DB */
384+
global $DB;
385+
386+
$this->login();
387+
388+
// Arrange: create questions whose default values were encoded by successive clones
389+
$this->fields['text'] = $this->createField([
390+
'label' => 'Text field',
391+
'type' => 'text',
392+
PluginFieldsContainer::getForeignKeyField() => $this->block->getID(),
393+
'ranking' => 2,
394+
'is_active' => 1,
395+
]);
396+
$itemtype = PluginFieldsDropdown::getClassname($this->fields['dropdown']->fields['name']);
397+
$dropdown_value = ['itemtype' => $itemtype, 'items_id' => 1];
398+
399+
$cases = [
400+
// question name => [field, stored default value, expected default value]
401+
'Text cloned twice' => ['text', json_encode(json_encode(json_encode('hello'))), json_encode('hello')],
402+
'Text not cloned' => ['text', json_encode('hello'), json_encode('hello')],
403+
'JSON-like text' => ['text', json_encode('[1,2]'), json_encode('[1,2]')],
404+
'Dropdown cloned twice' => ['dropdown', json_encode(json_encode(json_encode($dropdown_value))), json_encode($dropdown_value)],
405+
'Dropdown not cloned' => ['dropdown', json_encode($dropdown_value), json_encode($dropdown_value)],
406+
];
407+
408+
$builder = new FormBuilder("My form");
409+
foreach ($cases as $name => [$field_name]) {
410+
$builder->addQuestion(
411+
$name,
412+
PluginFieldsQuestionType::class,
413+
extra_data: json_encode($this->getFieldExtraDataConfig($field_name)),
414+
);
415+
}
416+
417+
$form = $this->createForm($builder);
418+
foreach ($cases as $name => [, $stored_value]) {
419+
$DB->update(
420+
Question::getTable(),
421+
['default_value' => $stored_value],
422+
['id' => $this->getQuestionId($form, $name)],
423+
);
424+
}
425+
426+
// Act: run the plugin migration
427+
ob_start();
428+
PluginFieldsQuestionType::installBaseData(new Migration(PLUGIN_FIELDS_VERSION), PLUGIN_FIELDS_VERSION);
429+
ob_end_clean();
430+
431+
// Assert: default values are encoded only once
432+
foreach ($cases as $name => [, , $expected_value]) {
433+
$question = Question::getById($this->getQuestionId($form, $name));
434+
$this->assertSame($expected_value, $question->fields['default_value'], $name);
435+
}
436+
}
437+
245438
public function testFieldDeletionWhenUsedInForm(): void
246439
{
247440
$this->login();

0 commit comments

Comments
 (0)