-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Closed

Description
I am coding a project along the lines of the cookbook entry on embedded collections as stated on: http://symfony.com/doc/current/cookbook/form/form_collections.html.
I am considering the following use case:
- Starting out with entering a new entity task, tag is the embedded collection
- I click the Javascript 'add a tag'-link, which adds a bar 0 instance of the tag form
- I click the Javascript 'add a tag'-link, which adds a bar 1 instance of the tag form
- I click the Javascript 'add a tag'-link, which adds a bar 2 instance of the tag form
- I click the Javascript 'delete this tag'-link for tag(1), which leaves me with the instances 0 and 2
Now, if I would leave all these tags blank (with a notblank validation on the tag name attribute), the returned form would be expected to visualize the error statements on instances 0 and 2. However, it turns out that the feedback is only displayed for instance 0.
If I change the returned form with firebug and set the instances on the name input field of instance 2 to '1', the feedback is displayed on the correct instance. This behavior indicates a mismatch between the indexing of the embedded form instances and the validators.
Activity
franmomu commentedon Apr 8, 2013
I had the same issue, I think the problem is when you set by_reference to false the indexes get lost when PropertyAccess/PropertyPath calls addTag and the errors are attached to the previous indexes.
To solve this I used the setTags method instead addTag.
syntk commentedon Apr 18, 2013
Same issue here!
The errors are not being displayed for each field but instead are being bubbled up to the form. This only happens when by_reference = false. How did you manage to use setTags instead of addTag? How can you opt for that since as far as i see it is mandatory to have addTag method.
syntk commentedon Apr 18, 2013
Nevermind figured out how to use setTag but here's another issue. If the collection form has an element form completely blank setTags will get an array with null values and you will not be able to persist until you remove the nulls.
franmomu commentedon Apr 20, 2013
In my case, I use setTags and getTags only. It is not mandatory, PropertyAccess looks for adder and remover, if it doesn't find them, uses the setter.
The other issue you mention I think it is the same for any field if you don't have validators in the fields.
webmozart commentedon May 3, 2013
The only viable solution I can think of right now is to require
addX()
methods to implement a second argument for the index, e.g.Do you think that is an acceptable solution?
The problem is that currently the data of the collection field is modified after the
submit()
method of that field finished, so I can't think of a way to notify the field of the changed data again.$form->submit():
In step 3, the collection indices are changed through the adders, but not reassigned to the collection field, whose scope only ranges from 2.1-2.3.
syntk commentedon Nov 17, 2013
Wondering when this will be fix. This make impossible to use the collection field according to the docs. Make the add and remove methods useless since in most of the cases you will want to add and remove elements and the index will not be consecutive. The solution proposed by bschussek migh work very well.
FlorianKoerner commentedon Jul 15, 2014
Same issue here
zoundfreak commentedon Oct 1, 2014
I too have seen this problem many times. Without a little tweak Symfony's embedded collections are pretty much useless. Too bad I've seen this problem on many live sites using Symfony (lack of proper testing??)...
To "fix" this issue, I just reindex the request collection data array before validating the data. I usually add a helper method similar to this to my base controller:
Then do the reindexing before the handleRequest call, like in this service handler example:
This seems to do the job for me!
webmozart commentedon Oct 23, 2014
I just thought about this again. I'm not sure whether adding an
$index
argument toaddX()
is a good solution. Then the collection field needs to know about the indexing scheme used by the collection, which it can't. We can't assume that the collection is always indexed using ascending integers.Another problem is that of race conditions. What if the underlying collection is changed between loading and submitting the form? Then the wrong elements of the collection will be changed.
Thus, it would be better to remove the coupling between the indexes of the collection and of the collection field and index by identifying values instead:
Then we can deterministically update the right objects on the server, map violation paths and distinguish existing from new elements.
webmozart commentedon Oct 23, 2014
See also #7828 which is proposing the same feature.
wsergio commentedon Oct 24, 2014
I second this solution - I stumbled upon this issue in multiple scenarios and came to this same conclusion - field/index decoupling.
zoundfreak commentedon Oct 28, 2014
Stupid question... how do you add new items (implementation based on the symfony cookbook form collection example referenced above) when you have set 'index_by' => 'id' to your collection?
What would be the next index? Or can you add new items somehow without the index?
How would this help if you are adding new collection rows and then you remove a row and then again add new ones?
Consider the following scenario:
Now rows 1 and 3 still have their original indexes 0 and 2. Based on the cookbook example js, row 4 would get index 2 and thus it will overwrite the actual row 3 with the same index. And the validation errors won't be shown. The validator assigns errors to row with index 1 but there's still no row with index 1. There's now original row 1 with index 0 and new row 4 with index 2.
So at the moment to deal with adding items I'm reindexing the rows before validation, so there won't be any missing indexes. Now in many cases doing the reindexing is not very safe when updating... at least if your collection items have associations to other entities.
If I understand this correctly, your suggestion to using indexBy would help with updates. But how on earth can you add new items in that case? What am I missing? :)
mvrhov commentedon Oct 29, 2014
IMO the index should stay like it's now. What's purposed as an index_by should instead be key_columns... which should automatically be written as a hidden fields inside a collection
30 remaining items
Seb33300 commentedon Nov 11, 2016
@dsands Doing
$k++
you will override the last reordered existing submitted data entry (setted by the previous foreach).You should set
$k = 0;
to$k = -1;
and keep++$k
.dsands commentedon Nov 11, 2016
@Seb33300 It seemed to be working, but looking at the code again I see what you mean.
Thanks again!
Fixing a very subtle validation issue with the CollectionType. I've d…
weaverryan commentedon Feb 17, 2017
In case it helps, we ran into this issue on our KnpUniversity tutorial about form collections. The fix for our situation was actually quite simple (inspired by the solution by @Seb33300, but simpler because it just fixes our use-case): #7468 (comment)
The solution is here: knpuniversity/symfony@ab02c47#diff-458cc0b549cdd91692b368dd8a0885bc along with a detailed explanation. In our JavaScript (http://knpuniversity.com/screencast/collections/add-new-collection-prototype#the-prototype-javascript) we always increment on new items... so if you delete an item before it's submitted, you have a hole (e.g. 1, 2, 4). Re-indexing the submitted data to be 1, 2, 3 seems to fix things just fine.
I hope that helps someone out :)
xabbuh commentedon Feb 17, 2017
@weaverryan You need to be careful with such a solution as you may run into cases where this means that no new object will be created, but the object to be removed from the collection is instead updated with the data of the newly added object.
idybil commentedon Apr 6, 2017
@weaverryan Your solution is not working if you use the UniqueEntity constraint, I think this is due to the fact that the collection index is rebuild.
xabbuh commentedon Sep 23, 2018
There are many issues related to this topic. I am closing here so we can keep #7828 as the main feature request.