Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
jwmkrezmien opened this issue Mar 24, 2013 · 44 comments
Closed

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

jwmkrezmien opened this issue Mar 24, 2013 · 44 comments

Comments

@jwmkrezmien
Copy link

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.

@franmomu
Copy link
Contributor

franmomu commented 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
Copy link

syntk commented 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
Copy link

syntk commented 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
Copy link
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
Copy link
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
Copy link

syntk commented 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
Copy link

Same issue here

@zoundfreak
Copy link

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
Copy link
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
Copy link
Contributor

See also #7828 which is proposing the same feature.

@wsergio
Copy link

wsergio commented Oct 24, 2014

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

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
Copy link

mvrhov commented 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

@stof
Copy link
Member

stof commented Nov 21, 2014

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)

@zoundfreak
Copy link

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?

@tazGPL
Copy link

tazGPL commented Nov 21, 2014

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.

@jrmyio
Copy link
Contributor

jrmyio commented Dec 7, 2014

Yay! Just waited 2+ hours on this.

@ryancastle
Copy link

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.

@angelk
Copy link
Contributor

angelk commented Aug 19, 2015

+1 for this issue! Spend 3 hours.

@Zoopme
Copy link

Zoopme commented Mar 3, 2016

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;
        }

@angelk
Copy link
Contributor

angelk commented Mar 3, 2016

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

@zoundfreak
Copy link

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.

@Tjeerd
Copy link

Tjeerd commented Mar 31, 2016

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?

@HeahDude
Copy link
Contributor

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

@Seb33300
Copy link
Contributor

Seb33300 commented Apr 22, 2016

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.

@vcraescu
Copy link

Thanks!

@placid2000
Copy link

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

@Seb33300
Copy link
Contributor

Seb33300 commented May 3, 2016

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.

@placid2000
Copy link

Yes, that's what I meant. Thanks!

@xabbuh
Copy link
Member

xabbuh commented May 4, 2016

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

@wuestkamp
Copy link

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?

@wuestkamp
Copy link

wuestkamp commented May 26, 2016

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

@Seb33300
Copy link
Contributor

Seb33300 commented May 26, 2016

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

@FrancescoBorzi
Copy link
Contributor

Confirmed @artorozenga

@Glideh
Copy link

Glideh commented Jul 11, 2016

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.

@wuestkamp
Copy link

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

@Glideh
Copy link

Glideh commented Jul 12, 2016

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

@dsands
Copy link

dsands commented Nov 10, 2016

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

@Seb33300
Copy link
Contributor

Seb33300 commented 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
Copy link

dsands commented Nov 11, 2016

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

Thanks again!

weaverryan added a commit to knpuniversity/symfony that referenced this issue Feb 17, 2017
…oc'ed it quite a bit - it fixes (in our project) symfony/symfony#7468. Thanks to Jürgen Modic for the tip on this!
@weaverryan
Copy link
Member

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
Copy link
Member

xabbuh commented 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
Copy link

idybil commented 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
Copy link
Member

xabbuh commented 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests