[Form] Respect order of submitted data when dealing with sorted collection models #21595

backbone87 · 2017-02-13T06:59:11Z

Q	A
Bug report?	no
Feature request?	yes
BC Break report?	no
RFC?	n/a
Symfony version	all

Use cases (few select ones):

expanded/multiple ChoiceType (checkbox list) sortable via JS
CollectionType sortable via JS without reassigning indexes (to avoid breaking entity identity/content mismatch)

Can this be solved with custom FormTypeExtension? Yes, but its somewhat bloated and requires deep knowledge of the form component to get it working correctly and consistently, definitely far beyond "entry" level experience for such common use cases.

Was this issue brought up before? Yes, but declined for various reason. One of the major arguments was, that browsers are not required to maintain a specific order of submitted form values. This isnt true: https://www.w3.org/TR/html5/forms.html#constructing-form-data-set requires tree order.
Furthermore the data format that the form component is expecting (in compound forms) are PHP arrays (FormInterface::submit(null|string|array $submittedData, ...), null treated as empty array) and PHP arrays are ordered hash maps and therefore exhibit an explicit ordering of its key-value pairs.

ref #10575, #4492, #8315, #8987

Here are some classes i use in my projects to achieve this:

// this one has the problem that it always clears the default data and completed replaces it with default data
class RestoreInputOrderTypeExtension extends AbstractTypeExtension {

	public function buildForm(FormBuilderInterface $builder, array $options) {
		if(!$options['restore_input_order'] || !$options['compound']) {
			return;
		}

		$builder->addEventListener(FormEvents::PRE_SUBMIT, function(FormEvent $fe) {
			$data = $fe->getData();

			if($data === null) {
				return;
			}

			if(!is_array($data)) {
				throw new TransformationFailedException('expected array', 1);
			}

			$form = $fe->getForm();
			foreach(array_keys($data) as $name) {
				if($form->has($name)) {
					$child = $form->get($name);
					$form->remove($name);
					$form->add($child);
				}
			}
		}, -100);

		$builder->addEventListener(FormEvents::SUBMIT, function(FormEvent $fe) {
			$data = $fe->getData();

			if($data === null) {
				return;
			}

			if(!is_array($data)) {
				throw new TransformationFailedException('expected array', 1);
			}

			$orderedData = array();
			foreach(array_keys($fe->getForm()->all()) as $name) {
				$orderedData[$name] = $data[$name];
			}

			$fe->setData($orderedData);

		}, -100);
	}

	/**
	 * @see \Symfony\Component\Form\AbstractTypeExtension::setDefaultOptions()
	 */
	public function setDefaultOptions(OptionsResolverInterface $resolver) {
		$resolver->setDefaults(array(
			'restore_input_order' => false,
		));
	}

	/**
	 * @see \Symfony\Component\Form\FormTypeExtensionInterface::getExtendedType()
	 */
	public function getExtendedType() {
		return 'form';
	}

}

class RestoreCheckboxInputOrderListener implements EventSubscriberInterface {

	private $inputData;

	private $choiceList;

	public function __construct(ChoiceListInterface $choiceList) {
		$this->choiceList = $choiceList;
	}

	public function preSubmit(FormEvent $event) {
		$this->inputData = $event->getData();
	}

	public function submit(FormEvent $event) {
		$data = array();
		foreach($this->choiceList->getChoicesForValues($this->inputData) as $inputChoice) {
			foreach($event->getData() as $i => $choice) {
				if($choice === $inputChoice) {
					$data[$i] = $choice;
				}
			}
		}
		$event->setData($data);
	}

	public static function getSubscribedEvents() {
		return array(
			FormEvents::PRE_SUBMIT => array('preSubmit', 100),
			FormEvents::SUBMIT => 'submit'
		);
	}

}

The text was updated successfully, but these errors were encountered:

HeahDude · 2017-02-14T01:36:21Z

Actually, the data is submitted in the order which the submitted data is given, not considering how the form is built, ~~so your listener for the choices is doing the opposite of what the title of this issue is suggesting~~.

The problem with collections is coming from the ResizeFormListener, my todo list has a dedicated item for it, I'm not yet sure of the better way to solve this.

HeahDude · 2017-02-14T01:43:37Z

Sorry I've edited my post as I've miss read the listener example. However changing the order of submission depending on the form tree build on initialization rather than using the submitted data order is a big BC break...

backbone87 · 2017-02-14T07:35:09Z

However changing the order of submission depending on the form tree build on initialization rather than using the submitted data order is a big BC break...

I would definitely make this an opt-in feature, because order should only matter, if you bind your data to collection-like structures (array or objects with ArrayAccess and Traversable).

Consider a form providing edit capabilities for personal data email, firstname, lastname. While the data passed to the FormInterface (the data binder), exhibits sorted map behavior (nature of PHP arrays). The model, the data will be bound to, does not and therefore it would be very bad code smell, if you rely on the order in which the setters are called/properties are set.

Its about intention of your model. If your model is intented to expose an ordered collection of things, you want your data binder to be able to bind things in order they were submitted.

HeahDude · 2017-02-14T08:08:37Z

I'm not worried about underlying data, but about form dynamic mechanisms happening on submission.

Some may (can) rely on this order although they shouldn't (ref #11241 (comment)).

You said this solves:

expanded/multiple ChoiceType (checkbox list) sortable via JS
CollectionType sortable via JS without reassigning indexes (to avoid breaking entity identity/content mismatch)

but one the contrary, your examples seem to make impossible to reorder the model data based on the submitted order. Data is already sortable in the view (i.e via JS). Preventing it with listeners is a lot of overhead and impacts performances.

For this reason I think the title of this issue is very confusing.

The only use case I see, is when you handle an array of data in the model and you don't want the order is the view to be messed up with. So why not as an opt-in? But IMHO, those listeners can live in user land.

backbone87 · 2017-02-14T08:54:41Z

Some may (can) rely on this order although they shouldn't (ref #11241 (comment)).

The order in which the FormInterface submits its children is clearly defined (FIFO). But i dont see how this relates here.

So why not as an opt-in?

I said i want it as an opt-in feature

But IMHO, those listeners can live in user land.

Here i dont agree. The 2 use cases i presented are very common and the feature is very hard to get right and consistent without having deep knowledge of the form component.

but one the contrary, your examples seem to make impossible to reorder the model data based on the submitted order. Data is already sortable in the view (i.e via JS). Preventing it with listeners is a lot of overhead and impacts performances.

I dont understand what you mean with that.

Also note that the classes i posted are not meant to be anything near PR ready and just some quick drop-ins i use in my projects.

I think we are basically on the same domain about the feature itself and how it should/could be done, just not about who should provide it (userland or framework).

xabbuh · 2018-09-23T16:44:14Z

I am closing here as this should be possible if #7828 is implemented

spackmat · 2024-02-22T15:24:12Z

Hey, sorry to dig out that old issue @xabbuh, but I'm facing the problem again, but with custom (string) Keys on the model data, so #7828 still does not make resorting of model data possible, hence this issue should not have been closed.

I dug a little deeper into the code and tried to understand the problem. Here is what I found:

Say we have a CollectionType, where the model data is a string-indexed Array (or ArrayCollection, doesn't matter) like ['a' => 'value a', 'b' => 'value b', 'c' => 'value c'] . On submitting that form with a new order of that collection (submitted value is now ['a' => 'value a', 'c' => 'value c', 'b' => 'value b'], this is what happens:

The ResizeFormListener removes all children of the collection field and rebuilds it in "correct order" on FormEvents::PRE_SET_DATA. That "correct order" (a comment calls it so) is the order of the old model state. So far, that is the correct order for now as we do not know the submitted order yet.
The $data within the FormEvents::PRE_SUBMIT handler of ResizeFormListener is the submitted data ['a' => 'value a', 'c' => 'value c', 'b' => 'value b']. Cool, that's the order we want to be the data in.
Now we are inside the Form class in the submit() method after we called the FormEvents::PRE_SUBMIT listeners. After some dozen of lines we get to the call of the data mapper, where the mapFormsToData() method is called to set the submitted data on the form's viewData, that is the data from the model at that moment. That data mapper is the default Symfony\Component\Form\Extension\Core\DataMapper\DataMapper class in that case and if we look into its mapFormsToData() method, we see that the child forms are iterated and their values are written to their index of the passed viewData. That is the place, where the order of the submitted data gets ignored.

To check, if that was the problem, I tried to reorder the collection's form children in ResizeFormListener::preSubmit() based on the submitted data, that is available here, but that is too late, because the Form class had set its viewData before (inside setData()) and does not sync the order of the form children and the data later on. So that alone does not help, as we also can see in the RestoreInputOrderTypeExtension posted by @backbone87 in the first post. He also overwrites the whole data with a very early priority in FormEvents::SUBMIT with the submitted data, throwing the whole model data out. That can be a solution in some cases but is very hacky and I assume several side effects.

So from my understanding, the problem can either be solved inside the data mapper (or with a new one for an opt-in sortable version of CollectionType) or in FormEvents::SUBMIT by reordering the normalized data by the keys from the submitted data.

Any comments on that? Am I on the right track?

spackmat · 2024-02-23T12:52:24Z

I solved my problem with an improved solution derived from the RestoreInputOrderTypeExtension posted by @backbone87 in the first post. My CollectionTypeSortableExtension basically does the same, but takes care, that the modified form children order contains all present children, also those that are not contained within the submitted data. Also I throw a TransformationFailedException, when I detect an integer key within the submitted data of the sortable Collection, because sorted arrays in PHP with integer keys are a painful thing and we don't need any of that pain here.

<?php
declare(strict_types=1);
namespace App\Form\Extension;

use Doctrine\Common\Collections\ArrayCollection;
use Symfony\Component\Form\AbstractTypeExtension;
use Symfony\Component\Form\Event\PreSubmitEvent;
use Symfony\Component\Form\Event\SubmitEvent;
use Symfony\Component\Form\Exception\TransformationFailedException;
use Symfony\Component\Form\Extension\Core\Type\CollectionType;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\Form\FormEvents;
use Symfony\Component\Form\FormInterface;
use Symfony\Component\Form\FormView;
use Symfony\Component\OptionsResolver\OptionsResolver;

final class CollectionTypeSortableExtension extends AbstractTypeExtension
{
    /** @inheritdoc */
    public function buildForm(FormBuilderInterface $builder, array $options): void
    {
        if (true !== $options['sortable']) {
            return;
        }

        // first, reorder the CollectionType's children by the order of the request
        $builder->addEventListener(
            eventName: FormEvents::PRE_SUBMIT,
            listener: function (PreSubmitEvent $event): void {
                $form = $event->getForm();
                $data = $event->getData();

                if (null === $data || count($form->all()) < 1) {
                    return;
                }

                if (!is_array($data)) {
                    throw new TransformationFailedException('Expected array data');
                }

                // add the CollectionType's children, that have data within the request in the request order and maintain a list of not yet added children
                $unmappedIndexes = new ArrayCollection(array_keys($form->all()));
                foreach (array_keys($data) as $childName) {
                    if (is_int($childName)) {
                        throw new TransformationFailedException('Integer-indexed data is not allowed, as that could lead to really hard to debug side-effects.');
                    }
                    if ($form->has($childName)) {
                        $currentChild = $form->get($childName);
                        $form->remove($childName);
                        $form->add($currentChild);
                        $unmappedIndexes->removeElement($childName);
                    }
                }
                unset($currentChild);
                unset($childName);

                // also add the remaining children, so that they are not lost
                foreach ($unmappedIndexes as $unmappedIndex) {
                    $currentChild = $form->get($unmappedIndex);
                    $form->remove($unmappedIndex);
                    $form->add($currentChild);
                }
                unset($currentChild);
                unset($unmappedIndex);
            },
            priority: -100
        );

        // and later, reorder the mapped data (ordered by model state) by the order of the previously reordered CollectionType's children
        $builder->addEventListener(
            eventName: FormEvents::SUBMIT,
            listener: function (SubmitEvent $event): void {
                $form = $event->getForm();
                $data = $event->getData();

                if (null === $data) {
                    return;
                }

                // we support ArrayCollection and array for the data
                if ($data instanceof ArrayCollection) {
                    $dataType = 'ArrayCollection';
                }
                elseif (is_array($data)) {
                    $dataType = 'array';
                    $data = new ArrayCollection($data);
                }
                else {
                    throw new TransformationFailedException('Expected ArrayCollection or array data');
                }

                // add the datasets, that have a corresponding form CollectionType's child in the CollectionType's children order
                // and maintain a list of not yet added datasets
                $unmappedIndexes = new ArrayCollection($data->getKeys());
                $orderedData = new ArrayCollection();
                foreach ($form->all() as $childName => $child) {
                    $orderedData->set($childName, $data->get($childName) ?? $child->getData());
                    $unmappedIndexes->removeElement($childName);
                }
                unset($child);

                // also add the remaining datasets, so that they are not lost, just in case as there should not be any at all
                foreach ($unmappedIndexes as $unmappedIndex) {
                    $orderedData->set($unmappedIndex, $data->get($unmappedIndex));
                }
                unset($unmappedIndexes);
                unset($unmappedIndex);

                $event->setData('array' === $dataType ? $orderedData->toArray() : $orderedData);
            },
            priority: -100
        );
    }

    /** @inheritdoc */
    public function buildView(FormView $view, FormInterface $form, array $options): void
    {
        $view->vars['sortable'] = $options['sortable'];
    }

    /** @inheritdoc */
    public function configureOptions(OptionsResolver $resolver): void
    {
        $resolver
            ->setDefaults(['sortable' => false])
            ->setAllowedTypes('sortable', 'bool')
        ;
    }

    /** @inheritdoc */
    public static function getExtendedTypes(): iterable
    {
        return [CollectionType::class];
    }
}

Have in mind, that this only works, when the model data already comes with string indexed data. Adding those indexes within a data transformer comes too late. So when for example the model data is not a DTO, but maybe a Doctrine Entity and the indexes cannot be added within the collection's getter, they must be added earlier, maybe within a PRE_SET_DATA listener. Most of my forms use DTOs, so the indexes can be added inside their constructors, but in one legacy case I ran into that model transformer pitfall.

For that reason I would love to see the core DataMapper keeping the submitted order and not assigning every single entry to its key of the model-state ordered old data, at least as an opt-in.

xabbuh added Feature Form labels Feb 13, 2017

backbone87 changed the title ~~[Form] Respect order of submitted data~~ [Form] Respect order of submitted data when dealing with sorted collection models Feb 14, 2017

jakzal mentioned this issue Oct 4, 2017

Symfony 3.2 Form field collection errors mapping #24268

Closed

maryo mentioned this issue Oct 14, 2017

Added preserve_names option, fixed position_selector duplication button ninsuo/symfony-collection#89

Merged

xabbuh closed this as completed Sep 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Form] Respect order of submitted data when dealing with sorted collection models #21595

[Form] Respect order of submitted data when dealing with sorted collection models #21595

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[Form] Respect order of submitted data when dealing with sorted collection models #21595

[Form] Respect order of submitted data when dealing with sorted collection models #21595

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!