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

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

As your test also asserts the entry form types of the collection type are not renamed. So after submitting we only have entry types named0,2, and3. However, due to howremoveAuthor() is implemented the submitted data is mapped to an indexed array. Thus when the form is displayed again, we now have data indexed with0,1, and2 while the form still has0,2,3.

With the current implementation I do not see how we can actually fix this as inside theResizeFormListener 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-inResizeFormListener and changes the form names.

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-inResizeFormListener 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 collectionsremoveElement method also reindex the collection the same way. Shouldn't Symfony Form at least provide an option to handle this (common) situation?

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 theCollectionType with Doctrine entities? What are the use case where theEntityType isn't sufficient?

Well, you know thatCollectionType andEntityType 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 aCollectionType with the optionentry_type set toEntityType.

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

What do you think?

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.

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

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 newkeep_as_list option forCollectionType.
I'm still open to suggestions on how to improve this code and find the right way to solve the highlighted issue.

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

Thank you@cristoforocervino.