[Form] Errors Property Paths mismatch CollectionType children when removing an entry #39438

cristoforocervino · 2020-12-10T19:13:17Z

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.

carsonbot · 2020-12-10T19:13:19Z

Hey!

I didn't know that was capable of this emotion. I really really like reviewing this PR. Well done.

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

xabbuh · 2021-01-12T07:58:47Z

@StevioStudio Sorry, your PR somehow slipped through. Thank you for taking the time to create a failing test case. That's really appreciated. 👍 I will try to look into a possible fix soon.

xabbuh · 2021-01-17T13:37:13Z

As your test also asserts the entry form types of the collection type are not renamed. So after submitting we only have entry types named 0, 2, and 3. However, due to how removeAuthor() is implemented the submitted data is mapped to an indexed array. Thus when the form is displayed again, we now have data indexed with 0, 1, and 2 while the form still has 0, 2, 3.

With the current implementation I do not see how we can actually fix this as inside the ResizeFormListener we do not know whether or not the index is important.

For your project I see two possible workarounds:

Change the implementation in your model class to not reindex the collection when the form data is mapped back (which might not be suitable depending on how you deal with the data).
Register a custom event listener that runs after the built-in ResizeFormListener and changes the form names.

cristoforocervino · 2021-01-17T14:16:56Z

For your project I see two possible workarounds:

Change the implementation in your model class to not reindex the collection when the form data is mapped back (which might not be suitable depending on how you deal with the data).

Register a custom event listener that runs after the built-in ResizeFormListener and changes the form names.

Wait, this is not only related to the model I'm using in this unit test.
CollectionType is usually used with Doctrine Collections. Doctrine collections removeElement method also reindex the collection the same way. Shouldn't Symfony Form at least provide an option to handle this (common) situation?

xabbuh · 2021-01-17T15:25:36Z

While I agree I don't see a way to fix this without introducing a new option (like proposed in #7828). So that would go into a future minor version instead of a patch release.

By the way, is it really a common use case to use the CollectionType with Doctrine entities? What are the use case where the EntityType isn't sufficient?

cristoforocervino · 2021-01-17T20:59:56Z

Well, you know that CollectionType and EntityType are meant to handle completely different situations.
I do not have numbers or statistics to show you, but it's pretty common, for example, to have a CollectionType with the option entry_type set to EntityType.

I think the new option with a minor version release could be a good idea.
Like a sequential_keys option set to false by default.
If sequential_keys is set to true, ResizeFormListener is going to handle a situation like the one described by my unit test in the right way.

What do you think?

xabbuh · 2021-01-18T08:51:56Z

I have a feature like that on my to-do list. Though not sure when I will find the time for it. Hopefully it will make it to 5.3.

cristoforocervino · 2021-01-18T20:37:37Z

I came up with a new reindex_on_submit boolean option set to false by default.
I updated code and test. What do you think? Did you have in mind something different?

cristoforocervino · 2021-04-13T18:31:29Z

This PR initially started as a bug fix with unit test provided to explain the issue.
I changed it into a new feature by introducing a new keep_as_list option for CollectionType.
I'm still open to suggestions on how to improve this code and find the right way to solve the highlighted issue.

andimg93 · 2022-04-01T11:29:28Z

@xabbuh, @yceruto, @chalasr, @dunglas, @jderusse, @lyrixx, @sroze, @wouterj
Please look into that issue fix, there are so many issues that all trace back to this bug and have been around since 2013 - it's really important, even if it's annoying to ask for focus here. Without a solution collections are simply not decently usable, it can and must not be a solution to re-index it all via javascript (frontend). Simply that you can't find a nice solution here (frontend based), but have to search out the individual parts via regex.

The real solution must be in the backend, because it is simply a bug and not a feature.

HeahDude

I'm not against a new opt-in feature for this but we should be careful when documenting it.
Indeed, such behavior depends on how the collection type data is mapped to the parent object.
In other words, how the adder and remover are implemented in the holder of the collection impact indexes without being able to send feedback to the collection type when going back to the view with errors.
Sadly after discussing this issue with @stof, we cannot think of another way to work around that efficiently, since the PropertyAccess does not allow to set specific indexes when calling adders and removers.

HeahDude · 2023-12-09T10:48:46Z

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

+        //In fact, root form should NOT contain errors but it does
+        $this->assertCount(1, $form->getErrors(false));
+
+        //Let's do this again, but with "keep_as_list" option set to true


Can you create another test method instead of grouping two tests in the same one please?

We also need a rebase and to update the changelog.
Please tell us if you want to do it or if we should take over, thank you.

HeahDude

We need to make fabbot happy fixing the CS and update the Form component CHANGELOG please. Thanks

…moving an entry

fabpot · 2023-12-09T14:28:08Z

Thank you @cristoforocervino.

cristoforocervino marked this pull request as ready for review December 10, 2020 19:15

cristoforocervino requested review from xabbuh and yceruto as code owners December 10, 2020 19:15

carsonbot added Status: Needs Review Form Bug labels Dec 10, 2020

carsonbot added this to the 4.4 milestone Dec 10, 2020

cristoforocervino force-pushed the form-collection-type-errors-property-paths-mismatch branch from a435596 to dc5e0b2 Compare December 10, 2020 19:16

cristoforocervino force-pushed the form-collection-type-errors-property-paths-mismatch branch from 88d307e to a4462c8 Compare January 18, 2021 20:30

cristoforocervino force-pushed the form-collection-type-errors-property-paths-mismatch branch from a4462c8 to 2cad8ee Compare April 13, 2021 18:16

cristoforocervino requested review from chalasr, dunglas, jderusse, lyrixx, sroze and wouterj as code owners April 13, 2021 18:16

cristoforocervino changed the base branch from 4.4 to 5.x April 13, 2021 18:18

fabpot modified the milestones: 4.4, 5.4 Aug 26, 2021

fabpot modified the milestones: 5.4, 6.1 Oct 30, 2021

fabpot modified the milestones: 6.1, 6.2 May 20, 2022

nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022

nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023

nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023

HeahDude reviewed Dec 9, 2023

View reviewed changes

cristoforocervino force-pushed the form-collection-type-errors-property-paths-mismatch branch from 94bae4d to 70e1b0e Compare December 9, 2023 11:53

OskarStark modified the milestones: 6.4, 7.1 Dec 9, 2023

OskarStark added Feature and removed Bug labels Dec 9, 2023

cristoforocervino force-pushed the form-collection-type-errors-property-paths-mismatch branch from 70e1b0e to fff25e3 Compare December 9, 2023 12:34

HeahDude reviewed Dec 9, 2023

View reviewed changes

cristoforocervino force-pushed the form-collection-type-errors-property-paths-mismatch branch from fff25e3 to f31de2b Compare December 9, 2023 13:18

HeahDude approved these changes Dec 9, 2023

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 9, 2023

fabpot approved these changes Dec 9, 2023

View reviewed changes

[Form] Errors Property Paths mismatch CollectionType children when re…

39587cb

…moving an entry

fabpot force-pushed the form-collection-type-errors-property-paths-mismatch branch from f31de2b to 39587cb Compare December 9, 2023 14:28

fabpot mentioned this pull request Dec 9, 2023

[Form] Errors Property Paths mismatch CollectionType children when remo… symfony/symfony-docs#19248

Closed

fabpot merged commit 37634c6 into symfony:7.1 Dec 9, 2023

fabpot mentioned this pull request May 2, 2024

Release v7.1.0-BETA1 #54820

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Form] Errors Property Paths mismatch CollectionType children when removing an entry #39438

[Form] Errors Property Paths mismatch CollectionType children when removing an entry #39438

cristoforocervino commented Dec 10, 2020 •

edited by OskarStark

Loading

carsonbot commented Dec 10, 2020

xabbuh commented Jan 12, 2021

xabbuh commented Jan 17, 2021

cristoforocervino commented Jan 17, 2021

xabbuh commented Jan 17, 2021

cristoforocervino commented Jan 17, 2021 •

edited

Loading

xabbuh commented Jan 18, 2021

cristoforocervino commented Jan 18, 2021

cristoforocervino commented Apr 13, 2021

andimg93 commented Apr 1, 2022

HeahDude left a comment

HeahDude Dec 9, 2023

HeahDude Dec 9, 2023

HeahDude left a comment

fabpot commented Dec 9, 2023

[Form] Errors Property Paths mismatch CollectionType children when removing an entry #39438

[Form] Errors Property Paths mismatch CollectionType children when removing an entry #39438

Conversation

cristoforocervino commented Dec 10, 2020 • edited by OskarStark Loading

carsonbot commented Dec 10, 2020

xabbuh commented Jan 12, 2021

xabbuh commented Jan 17, 2021

cristoforocervino commented Jan 17, 2021

xabbuh commented Jan 17, 2021

cristoforocervino commented Jan 17, 2021 • edited Loading

xabbuh commented Jan 18, 2021

cristoforocervino commented Jan 18, 2021

cristoforocervino commented Apr 13, 2021

andimg93 commented Apr 1, 2022

HeahDude left a comment

Choose a reason for hiding this comment

HeahDude Dec 9, 2023

Choose a reason for hiding this comment

HeahDude Dec 9, 2023

Choose a reason for hiding this comment

HeahDude left a comment

Choose a reason for hiding this comment

fabpot commented Dec 9, 2023

cristoforocervino commented Dec 10, 2020 •

edited by OskarStark

Loading

cristoforocervino commented Jan 17, 2021 •

edited

Loading