Skip to content

[Form] Collection indices changed by adders are not reassigned #7468

@ghost

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:

  1. Starting out with entering a new entity task, tag is the embedded collection
  2. I click the Javascript 'add a tag'-link, which adds a bar 0 instance of the tag form
  3. I click the Javascript 'add a tag'-link, which adds a bar 1 instance of the tag form
  4. I click the Javascript 'add a tag'-link, which adds a bar 2 instance of the tag form
  5. 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

franmomu commented on Apr 8, 2013

@franmomu
Contributor

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

syntk commented on Apr 18, 2013

@syntk

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

syntk commented on Apr 18, 2013

@syntk

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

franmomu commented on Apr 20, 2013

@franmomu
Contributor

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

webmozart commented on May 3, 2013

@webmozart
Contributor

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.

syntk

syntk commented on Nov 17, 2013

@syntk

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

FlorianKoerner commented on Jul 15, 2014

@FlorianKoerner

Same issue here

zoundfreak

zoundfreak commented on Oct 1, 2014

@zoundfreak

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!

webmozart

webmozart commented on Oct 23, 2014

@webmozart
Contributor

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.

webmozart

webmozart commented on Oct 23, 2014

@webmozart
Contributor

See also #7828 which is proposing the same feature.

wsergio

wsergio commented on Oct 24, 2014

@wsergio

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

zoundfreak

zoundfreak commented on Oct 28, 2014

@zoundfreak

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

mvrhov

mvrhov commented on Oct 29, 2014

@mvrhov

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

45 remaining items

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @FrancescoBorzi@weaverryan@webmozart@wsergio@ryanotella

        Issue actions

          [Form] Collection indices changed by adders are not reassigned · Issue #7468 · symfony/symfony