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.

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.

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.

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.

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.

public function addTag($tag, $index = null)
{
    if (null !== $index) {
        $this->tags[$index] = $tag;
    } else {
        $this->tags[] = $tag;
    }
}

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():

1. SUBMIT
2. $collection->submit()
   1. PRE_SUBMIT
   2. SUBMIT
   3. POST_SUBMIT
3. $collection->getData() is merged into $form->getData()
4. SUBMIT
5. POST_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.

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.

Same issue here

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:

/**
 * @param \Symfony\Component\HttpFoundation\Request $request
 */
protected function reindexDynamicFields(Request $request)
{
    $requestData = $request->request->get('my_form');
    $requestData['myCollection'] = array_values($requestData['myCollection']);
    $request->request->set('my_form', $requestData);
}

Then do the reindexing before the handleRequest call, like in this service handler example:

$service = new Service();
$form = $this->createCreateForm($service);

$this->reindexDynamicFields($request);
$form->handleRequest($request);

if ($form->isValid()) {
    ....
}

This seems to do the job for me!

I just thought about this again. I'm not sure whether adding an $index argument to addX() 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:

$builder->add('tags', 'collection', array(
    'index_by' => 'id',
));

Then we can deterministically update the right objects on the server, map violation paths and distinguish existing from new elements.

See also #7828 which is proposing the same feature.

"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"

I second this solution - I stumbled upon this issue in multiple scenarios and came to this same conclusion - field/index decoupling.

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:

1. Start to create a new item with a collection
2. Enter collection rows 1, 2, 3 (will get indexes 0, 1 and 2) / assume row 3 is not valid
3. Remove row 2
4. Submit -> validation fails
5. Add row 4

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? :)

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

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.

Nobody ever said that the cookbook example JS was a perfect JS code for handling collection (IIRC, authors of this snippet even recognized it had some weak parts when submitting it to the doc)

Stof, you're right... nobody made that claim and we can see that it's not perfect. However the original question still remains open. How do people handle these kinds of forms easily and reliably? Does anyone have a good solution that could be added to the cookbook example?

I bypass the issue on the front-end side. In the example, in function addTagFormDeleteLink, instead of

$tagFormLi.remove();

I use

$tagFormLi.hide();
$tagFormLi.find("input, textarea").prop({"disabled": true});
//               ^^^^^^^^^^^^^^^
// the example collection in my case consist of 2 inputs and a textarea

This way the indexes remain in the DOM, but the values are disabled and effectively detached from the main object on form submit.
I'm not sure if it's a good way, but works for me, and I'm feeling much more comfortable with it rather than with tweaking the back-end. Maybe it would work as an example for other not very experienced users like me.

Yay! Just waited 2+ hours on this.

We often see issues around this. We resolved the index collision problem in dynamic forms (which can occur if you follow the cookbook recipe), by using randomly generated indexes in JS. So to prevent data corruption for occasional cases we end up breaking collection validation in every case. However, vague error messages seemed like a better compromise to us than broken data.

We've also had race condition issues mentioned by @webmozart. In the end we resolved that by using indexBy on the Doctrine collection, and aggressively persisting new items before the form is rendered.

We'd love to see a resolution to this, but I realise it's very difficult.

+1 for this issue! Spend 3 hours.

I have come round this issue on the client side by modifying the Javascript/Jquery code given in the Symfony Documentation.

Instead of numbering the new elements by counting the sub-elements, I am looking at the last element's id and extracting its index with a regular expression.

When adding an element, I am incrementing the last index by 1. That way, I never use the same index.

Here is my code :

        // Initializing default index at 0
        var index = 0;

        // Looking for collection fields in the form
        var $findinput = $container.find(':input');

        // If fields found then looking for last existing index
        if ( $findinput.length > 0 ) {

            // Reading id of last field
            var myString = $findinput.last().attr('id')

            // Setting regular expression to extract number from id containing letters, hyphens and underscores
            var myRegex = /^[-_A-Za-z]+([0-9]+)[-_A-Za-z]*$/

            // Executing regular expression on last collection field id
            var test = myRegex.exec(myString);

            // Extracting last index and incrementing by 1
            if (test.length > 0) index = parseInt(test[1]) + 1;
        }

@Zoopme you could use twig filters "keys -> last".
Your solution won't fix the errors from #7468 (comment)

I wasn't aware of the keys filter until now, but I ended up using something like the code below instead:

{% if customer.vehicles is not empty %}
    {% set nextIndex = max(customer.vehicles.getKeys) + 1 %}
{% else %}
    {% set nextIndex = 0 %}
{% endif %}

<div class="dynamicForm" data-next-index="{{ nextIndex }}" data-prototype="{% filter escape %}{% include 'ApplicationBundle:UserManagement:customer_vehicle_prototype.html.twig' with { 'item': form.vehicles.vars.prototype } %}{% endfilter %}">
    {% for item in form.vehicles %}
        {% include 'ApplicationBundle:UserManagement:customer_vehicle_prototype.html.twig' with { 'item': item } %}
    {% endfor %}

    {{ form_widget(form.vehicles) }}
</div>

I have a javascript that binds dynamic form handling to each dynamicForm element. In the javascript I use and modify the next-index data parameter. The next-index is increased on every addition and never decreased. I've managed to get this working automatically for any number of nested forms.

So, to summarize this issue: if you have a collection on a form, and if you want to use allow_add and allow_delete you should not use by_reference = false, because this will cause issues with mapping the error message back to the form. Correct?

@Tjeerd Sadly I don't think we have the choice for now since the mapping is handled by the ResizeFormListener :/

Solved this by extending my forms with this:

use Symfony\Component\Form\AbstractType;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\Form\FormEvent;
use Symfony\Component\Form\FormEvents;
use Symfony\Component\Form\FormInterface;

/**
 * Class AbstractEntityType
 * @package BvStandard\ServiceBundle\Form
 */
abstract class AbstractEntityType extends AbstractType
{
    /**
     * @param FormBuilderInterface $builder
     * @param array                $options
     */
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder->addEventListener(FormEvents::PRE_SUBMIT, array($this, 'onPreSubmit'));
    }

    /**
     * @param FormEvent $event
     */
    public function onPreSubmit(FormEvent $event)
    {
        $form = $event->getForm();
        $data = $event->getData();
        $object = $event->getForm()->getData();

        // Loop on form fields
        foreach ($event->getForm()->all() as $field) {
            /** @var FormInterface $field */
            $fieldName = $field->getName();
            if ($field->getConfig()->getType()->getName() == 'collection') {
                if (isset($data[$fieldName]) && $object) {
                    $collection = $data[$fieldName];
                    $data[$fieldName] = array();
                    $k = 0;
                    // Reorder submitted data
                    foreach ($object->{'get'.ucfirst($fieldName)}() as $k => $item) {
                        // Search
                        foreach ($collection as $i => $c) {
                            $cId = is_array($c) && isset($c['id']) ? $c['id'] : $c;
                            if ($item->getId() == $cId) {
                                $data[$fieldName][$k] = $c;
                                unset($collection[$i]);
                                break;
                            }
                        }
                    }
                    // Added items
                    foreach ($collection as $c) {
                        $data[$fieldName][++$k] = $c;
                    }
                }
            }
        }

        $event->setData($data);
    }
}

Of course, you have to add parent::buildForm($builder, $options); in your FormType extending this.

Thanks!

@Seb33300 Thank you for the solution. Does this approach also work for nested collections?

If by nested collections, you mean something like:

    ->add('fieldName', CollectionType::class, array(
        'entry_type'   => OtherFormType::class,
        'allow_add'    => true,
        'allow_delete' => true,
        'by_reference' => false,
    ))

With OtherFormType::class is a form type containing a collection field.
Yes, it will work if OtherFormType extends the above class too.

Yes, that's what I meant. Thanks!

@Seb33300 @placid2000 Though you should rather think about moving the code into a form type extension instead of having to extend some base class.

I have the exact same issue with Symfony version 2.3.13-DEV.

@Tjeerd what is involved if I do not use by_reference = false - do I then only have to map my newly created sub-entities through my collection manually to my parent entity in my action that handles the form submission? Or something else as well?

@Seb33300 thanks for your solution. But this does actually remove an item and create a new one when updating, so its changing the entity ID. Something to be aware of.

@kimwue If you want to update your entity without removing it (without changing the id), your form have to submit the id of each entities you are updating. If no id is submitted, a new entity is created.

Confirmed @artorozenga

I can't use the UniqueEntity constraint on my CollectionTypes (where allow_add is true).
However I don't see these forms working without by_reference => false.
I didn't understand the whole thread here but my issue seems linked.
The uniqueness seems to be checked against the database items only.

@Glideh as far as I know you need to write your own validator (or logic on Controller) to check if there are non-unique items. UniqueEntity checks only against DB.

I see, I'll do that, so it is the expected behavior.

@Seb33300 Thanks for this bit of code.

I made one very minor modification to handle zero based indexing:

I changed

$data[$fieldName][++$k] = $c;

to

$data[$fieldName][$k++] = $c;

I then followed the doc http://symfony.com/doc/current/form/create_form_type_extension.html

I extended the form type FormType, so all forms now automatically use this extension.

Nice solution!

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

@Seb33300 It seemed to be working, but looking at the code again I see what you mean.

Thanks again!

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 :)

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

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

There are many issues related to this topic. I am closing here so we can keep #7828 as the main feature request.