Skip to content

Commit 37634c6

Browse files
fabpotfabpot
committed
feature #39438 [Form] Errors Property Paths mismatch CollectionType children when removing an entry (cristoforocervino)
This PR was squashed before being merged into the 7.1 branch. Discussion ---------- [Form] Errors Property Paths mismatch CollectionType children when removing an entry | Q | A | ------------- | --- | Branch? | 7.1 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - **Description** When you remove an entry from `CollectionType` and you have errors on children, errors property paths generated by `ValidatorExtension` are matching final object collection indexes but do not match From children entries index/names. **How to reproduce** _Unit test provided._ **Solution** Introduce new feature with `keep_as_list` (`boolean` default to `false`) option to reindex children on submit. Commits ------- 39587cb [Form] Errors Property Paths mismatch CollectionType children when removing an entry
2 parents c715b55 + 39587cb commit 37634c6

File tree

5 files changed

+238
-2
lines changed

5 files changed

+238
-2
lines changed

‎src/Symfony/Component/Form/CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ CHANGELOG
55
---
66

77
* Deprecate not configuring the `default_protocol` option of the `UrlType`, it will default to `null` in 8.0
8+
* Add a `keep_as_list` option to `CollectionType`
89

910
7.0
1011
---

‎src/Symfony/Component/Form/Extension/Core/EventListener/ResizeFormListener.php

+17-1
Original file line numberDiff line numberDiff line change
@@ -31,15 +31,17 @@ class ResizeFormListener implements EventSubscriberInterface
3131
protected bool $allowDelete;
3232

3333
private \Closure|bool $deleteEmpty;
34+
private bool $keepAsList;
3435

35-
public function __construct(string $type, array $options = [], bool $allowAdd = false, bool $allowDelete = false, bool|callable $deleteEmpty = false, array $prototypeOptions = null)
36+
public function __construct(string $type, array $options = [], bool $allowAdd = false, bool $allowDelete = false, bool|callable $deleteEmpty = false, array $prototypeOptions = null, bool $keepAsList = false)
3637
{
3738
$this->type = $type;
3839
$this->allowAdd = $allowAdd;
3940
$this->allowDelete = $allowDelete;
4041
$this->options = $options;
4142
$this->deleteEmpty = \is_bool($deleteEmpty) ? $deleteEmpty : $deleteEmpty(...);
4243
$this->prototypeOptions = $prototypeOptions ?? $options;
44+
$this->keepAsList = $keepAsList;
4345
}
4446

4547
public static function getSubscribedEvents(): array
@@ -153,6 +155,20 @@ public function onSubmit(FormEvent $event): void
153155
}
154156
}
155157

158+
if ($this->keepAsList) {
159+
$formReindex = [];
160+
foreach ($form as $name => $child) {
161+
$formReindex[] = $child;
162+
$form->remove($name);
163+
}
164+
foreach ($formReindex as $index => $child) {
165+
$form->add($index, $this->type, array_replace([
166+
'property_path' => '['.$index.']',
167+
], $this->options));
168+
}
169+
$data = array_values($data);
170+
}
171+
156172
$event->setData($data);
157173
}
158174
}

‎src/Symfony/Component/Form/Extension/Core/Type/CollectionType.php

+4-1
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,8 @@ public function buildForm(FormBuilderInterface $builder, array $options): void
4545
$options['allow_add'],
4646
$options['allow_delete'],
4747
$options['delete_empty'],
48-
$resizePrototypeOptions
48+
$resizePrototypeOptions,
49+
$options['keep_as_list']
4950
);
5051

5152
$builder->addEventSubscriber($resizeListener);
@@ -114,12 +115,14 @@ public function configureOptions(OptionsResolver $resolver): void
114115
'prototype_options' => [],
115116
'delete_empty' => false,
116117
'invalid_message' => 'The collection is invalid.',
118+
'keep_as_list' => false,
117119
]);
118120

119121
$resolver->setNormalizer('entry_options', $entryOptionsNormalizer);
120122

121123
$resolver->setAllowedTypes('delete_empty', ['bool', 'callable']);
122124
$resolver->setAllowedTypes('prototype_options', 'array');
125+
$resolver->setAllowedTypes('keep_as_list', ['bool']);
123126
}
124127

125128
public function getBlockPrefix(): string

‎src/Symfony/Component/Form/Tests/Extension/Validator/Type/FormTypeValidatorExtensionTest.php

+184
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,12 @@
1515
use Symfony\Component\Form\Form;
1616
use Symfony\Component\Form\Forms;
1717
use Symfony\Component\Form\Test\Traits\ValidatorExtensionTrait;
18+
use Symfony\Component\Form\Tests\Extension\Core\Type\CollectionTypeTest;
1819
use Symfony\Component\Form\Tests\Extension\Core\Type\FormTypeTest;
1920
use Symfony\Component\Form\Tests\Extension\Core\Type\TextTypeTest;
2021
use Symfony\Component\Form\Tests\Fixtures\Author;
22+
use Symfony\Component\Form\Tests\Fixtures\AuthorType;
23+
use Symfony\Component\Form\Tests\Fixtures\Organization;
2124
use Symfony\Component\OptionsResolver\Exception\InvalidOptionsException;
2225
use Symfony\Component\Validator\Constraints\GroupSequence;
2326
use Symfony\Component\Validator\Constraints\Length;
@@ -158,4 +161,185 @@ protected function createForm(array $options = [])
158161
{
159162
return $this->factory->create(FormTypeTest::TESTED_TYPE, null, $options);
160163
}
164+
165+
public function testCollectionTypeKeepAsListOptionFalse()
166+
{
167+
$formMetadata = new ClassMetadata(Form::class);
168+
$authorMetadata = (new ClassMetadata(Author::class))
169+
->addPropertyConstraint('firstName', new NotBlank());
170+
$organizationMetadata = (new ClassMetadata(Organization::class))
171+
->addPropertyConstraint('authors', new Valid());
172+
$metadataFactory = $this->createMock(MetadataFactoryInterface::class);
173+
$metadataFactory->expects($this->any())
174+
->method('getMetadataFor')
175+
->willReturnCallback(static function ($classOrObject) use ($formMetadata, $authorMetadata, $organizationMetadata) {
176+
if (Author::class === $classOrObject || $classOrObject instanceof Author) {
177+
return $authorMetadata;
178+
}
179+
180+
if (Organization::class === $classOrObject || $classOrObject instanceof Organization) {
181+
return $organizationMetadata;
182+
}
183+
184+
if (Form::class === $classOrObject || $classOrObject instanceof Form) {
185+
return $formMetadata;
186+
}
187+
188+
return new ClassMetadata(\is_string($classOrObject) ? $classOrObject : $classOrObject::class);
189+
});
190+
191+
$validator = Validation::createValidatorBuilder()
192+
->setMetadataFactory($metadataFactory)
193+
->getValidator();
194+
195+
$form = Forms::createFormFactoryBuilder()
196+
->addExtension(new ValidatorExtension($validator))
197+
->getFormFactory()
198+
->create(FormTypeTest::TESTED_TYPE, new Organization([]), [
199+
'data_class' => Organization::class,
200+
'by_reference' => false,
201+
])
202+
->add('authors', CollectionTypeTest::TESTED_TYPE, [
203+
'entry_type' => AuthorType::class,
204+
'allow_add' => true,
205+
'allow_delete' => true,
206+
'keep_as_list' => false,
207+
])
208+
;
209+
210+
$form->submit([
211+
'authors' => [
212+
0 => [
213+
'firstName' => '', // Fires a Not Blank Error
214+
'lastName' => 'lastName1',
215+
],
216+
// key "1" could be missing if we add 4 blank form entries and then remove it.
217+
2 => [
218+
'firstName' => '', // Fires a Not Blank Error
219+
'lastName' => 'lastName3',
220+
],
221+
3 => [
222+
'firstName' => '', // Fires a Not Blank Error
223+
'lastName' => 'lastName3',
224+
],
225+
],
226+
]);
227+
228+
// Form does have 3 not blank errors
229+
$errors = $form->getErrors(true);
230+
$this->assertCount(3, $errors);
231+
232+
// Form behaves as expected. It has index 0, 2 and 3 (1 has been removed)
233+
// But errors property paths mismatch happening with "keep_as_list" option set to false
234+
$errorPaths = [
235+
$errors[0]->getCause()->getPropertyPath(),
236+
$errors[1]->getCause()->getPropertyPath(),
237+
$errors[2]->getCause()->getPropertyPath(),
238+
];
239+
240+
$this->assertTrue($form->get('authors')->has('0'));
241+
$this->assertContains('data.authors[0].firstName', $errorPaths);
242+
243+
$this->assertFalse($form->get('authors')->has('1'));
244+
$this->assertContains('data.authors[1].firstName', $errorPaths);
245+
246+
$this->assertTrue($form->get('authors')->has('2'));
247+
$this->assertContains('data.authors[2].firstName', $errorPaths);
248+
249+
$this->assertTrue($form->get('authors')->has('3'));
250+
$this->assertNotContains('data.authors[3].firstName', $errorPaths);
251+
252+
// As result, root form contain errors
253+
$this->assertCount(1, $form->getErrors(false));
254+
}
255+
256+
public function testCollectionTypeKeepAsListOptionTrue()
257+
{
258+
$formMetadata = new ClassMetadata(Form::class);
259+
$authorMetadata = (new ClassMetadata(Author::class))
260+
->addPropertyConstraint('firstName', new NotBlank());
261+
$organizationMetadata = (new ClassMetadata(Organization::class))
262+
->addPropertyConstraint('authors', new Valid());
263+
$metadataFactory = $this->createMock(MetadataFactoryInterface::class);
264+
$metadataFactory->expects($this->any())
265+
->method('getMetadataFor')
266+
->willReturnCallback(static function ($classOrObject) use ($formMetadata, $authorMetadata, $organizationMetadata) {
267+
if (Author::class === $classOrObject || $classOrObject instanceof Author) {
268+
return $authorMetadata;
269+
}
270+
271+
if (Organization::class === $classOrObject || $classOrObject instanceof Organization) {
272+
return $organizationMetadata;
273+
}
274+
275+
if (Form::class === $classOrObject || $classOrObject instanceof Form) {
276+
return $formMetadata;
277+
}
278+
279+
return new ClassMetadata(\is_string($classOrObject) ? $classOrObject : $classOrObject::class);
280+
});
281+
282+
$validator = Validation::createValidatorBuilder()
283+
->setMetadataFactory($metadataFactory)
284+
->getValidator();
285+
286+
$form = Forms::createFormFactoryBuilder()
287+
->addExtension(new ValidatorExtension($validator))
288+
->getFormFactory()
289+
->create(FormTypeTest::TESTED_TYPE, new Organization([]), [
290+
'data_class' => Organization::class,
291+
'by_reference' => false,
292+
])
293+
->add('authors', CollectionTypeTest::TESTED_TYPE, [
294+
'entry_type' => AuthorType::class,
295+
'allow_add' => true,
296+
'allow_delete' => true,
297+
'keep_as_list' => true,
298+
])
299+
;
300+
301+
$form->submit([
302+
'authors' => [
303+
0 => [
304+
'firstName' => '', // Fires a Not Blank Error
305+
'lastName' => 'lastName1',
306+
],
307+
// key "1" could be missing if we add 4 blank form entries and then remove it.
308+
2 => [
309+
'firstName' => '', // Fires a Not Blank Error
310+
'lastName' => 'lastName3',
311+
],
312+
3 => [
313+
'firstName' => '', // Fires a Not Blank Error
314+
'lastName' => 'lastName3',
315+
],
316+
],
317+
]);
318+
319+
// Form does have 3 not blank errors
320+
$errors = $form->getErrors(true);
321+
$this->assertCount(3, $errors);
322+
323+
// No property paths mismatch happening with "keep_as_list" option set to true
324+
$errorPaths = [
325+
$errors[0]->getCause()->getPropertyPath(),
326+
$errors[1]->getCause()->getPropertyPath(),
327+
$errors[2]->getCause()->getPropertyPath(),
328+
];
329+
330+
$this->assertTrue($form->get('authors')->has('0'));
331+
$this->assertContains('data.authors[0].firstName', $errorPaths);
332+
333+
$this->assertTrue($form->get('authors')->has('1'));
334+
$this->assertContains('data.authors[1].firstName', $errorPaths);
335+
336+
$this->assertTrue($form->get('authors')->has('2'));
337+
$this->assertContains('data.authors[2].firstName', $errorPaths);
338+
339+
$this->assertFalse($form->get('authors')->has('3'));
340+
$this->assertNotContains('data.authors[3].firstName', $errorPaths);
341+
342+
// Root form does NOT contain errors
343+
$this->assertCount(0, $form->getErrors(false));
344+
}
161345
}

‎src/Symfony/Component/Form/Tests/Fixtures/Organization.php

+32
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
<?php
2+
3+
namespace Symfony\Component\Form\Tests\Fixtures;
4+
5+
class Organization
6+
{
7+
public $authors = [];
8+
9+
public function __construct(array $authors = [])
10+
{
11+
$this->authors = $authors;
12+
}
13+
14+
public function getAuthors(): array
15+
{
16+
return $this->authors;
17+
}
18+
19+
public function addAuthor(Author $author): self
20+
{
21+
$this->authors[] = $author;
22+
return $this;
23+
}
24+
25+
public function removeAuthor(Author $author): self
26+
{
27+
if (false !== $key = array_search($author, $this->authors, true)) {
28+
array_splice($this->authors, $key, 1);
29+
}
30+
return $this;
31+
}
32+
}

0 commit comments

Comments
 (0)