[Form] collection type is broken #3539

Closed
mvrhov opened this Issue Mar 9, 2012 · 13 comments

Projects

None yet

4 participants

@mvrhov

This one is a bit hard to explain, but I'll try my best.

PR#3239 brought proper handling of collections, but it doesn't work as expected. I've traced this only for blank collection, but this is probably the same or similar when you go and edit existing one.
My collection object is having duplicates. And if I take a look at the memory addresses of those duplicates with the debugger, I can see that the same objects are added twice as they reside on the same address.

It seems, that the data is bind to the collection twice, first in the form's bind() method and then once again in MergeCollectionListener.

Looking at the $originalData when onBindNormData is called in
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php#L142
I can see, that $originalData already contains the whole collection that came in through the request.

Later down the code calls add* method and adds the same objects.
Now the problem is not visible if all you do with the data is just call $em->persist($form->getData()
as doctrine will silently handle this, but if you need to traverse the collection and do some calculations
on it before persisting, then you have the problem as you'll do the whatever you want to do twice.

@stloyd

Hmmm, do you have some prevention of duplicates in your entity ? I.e.:

<?php

class User {
    public function addGroup($group) {
        if (!$this->groups->contains($group)) {
            $this->groups[] = $group;
        }
    }
}
@mvrhov

No, I don't. But at least for now I could implement this as a workaround.

@mvrhov

Along with the code above you need another loop before persisting and calling whatever code your add* method calls like setUser($group)

@stof
Symfony member

@mvrhov why another loop ? if your collection is the inversed side, simply add the needed code to update the owning side in your adder instead of just adding it in the collection. and btw, for the inversed side, duplicates are not really an issue as they won't be persisted by Doctrine (it is only an issue if you display your object directly but you generally redirect after handling a form anyway).

Note that I'm talking about symfony 2.1 here. the 2.0 code does not support inversed side of relations properly.

@mvrhov

I'm also talking about master or 2.1 here. And as I said I have to do some post processing on the collection and later on some data from that collection and post processing is put into the flash message.

@stof
Symfony member

well, then keep the contains check in the inversed side as well. But this does not forbid you to update the owning side

@webmozart
Symfony member

Can you check please whether this is fixed in #3819? Or has this issue resolved itself?

@mvrhov

I can't verify before Tuesday/Wednesday.

@mvrhov

I've updated to latest master and this still doesn't work. as I said the problem is that by the time the add* gets called the details are already in the collection but those details are not added via add but somehow bound inside form code.

@webmozart
Symfony member

@mvrhov Can you provide more information in order to reproduce this issue? Even better, a test case?

@mvrhov

Ah.. PR#3789 was merged today and I have trouble adapting the testcase.
But pre above PR testcase for MergeCollectionListenerTest is bellow

use Symfony\Component\Form\Extension\Core\CoreExtension;
use Symfony\Component\Form\FormFactory;
use Symfony\Component\Form\AbstractType;
use Doctrine\Common\Collections\ArrayCollection;

class MergeCollectionListenerTest_CarAxes
{
    private $axes;

    public function addAxis(MergeCollectionListenerTest_CarAxis $axis)
    {
        //we are not checking if it's already in collection on purpose
        //if (!$this->getAxes()->contains($axis)) {
            $this->getAxes()->add($axis);
            $axis->setCar($this);
        //}
    }

    public function removeAxis(MergeCollectionListenerTest_CarAxis $axis)
    {
        $this->getAxes()->removeElement($axis);
    }

    public function getAxes()
    {
        return $this->axes ?: $this->axes = new ArrayCollection();
    }
}

class MergeCollectionListenerTest_CarAxis
{
    private $car;

    public $prop1;
    public $prop2;

    public function setCar($car)
    {
        $this->car = $car;
    }

    public function getCar()
    {
        return $this->car;
    }
}

class MergeCollectionListenerTest_CarAxisType extends AbstractType
{
    public function buildForm(FormBuilder $builder, array $options)
    {
        $builder
            ->add('prop1', 'text')
            ->add('prop2', 'text')
        ;
    }

    public function getName()
    {
        return 'axis';
    }

    public function getDefaultOptions(array $options)
    {
        return array(
            'data_class' => __NAMESPACE__ .'\\MergeCollectionListenerTest_CarAxis',
        );
    }
}

    public function testDoubles()
    {
        $ff = new FormFactory(array(new CoreExtension()));

        $axes = new MergeCollectionListenerTest_CarAxes();
        $builder = $ff->createBuilder('form', $axes);
        $builder
            ->add('axes', 'collection', array(
                    'type'      => new MergeCollectionListenerTest_CarAxisType(),
                    'allow_add' => true,
                    'options'       => array( //FIXME we need the line below because of bug https://github.com/symfony/symfony/issues/3354
                        'data_class' => 'Symfony\Component\Form\Tests\Extension\Core\EventListener\MergeCollectionListenerTest_CarAxis',
                    )
                )
            )
        ;

        $builder
            ->getForm()
                ->bind(
                    array('axes' => array(
                        array('prop1' => '01', 'prop2' => '02'),
                        array('prop1' => '11', 'prop2' => '12'),
                    )
                )
        );

        $newAxis = function($prop1, $prop2) {
            $ret = new MergeCollectionListenerTest_CarAxis();
            $ret->prop1 = $prop1;
            $ret->prop2 = $prop2;

            return $ret;
        };

        $cmpAxes = new MergeCollectionListenerTest_CarAxes();
        $cmpAxes->addAxis($newAxis('01', '02'));
        $cmpAxes->addAxis($newAxis('11', '12'));

        $this->assertEquals($cmpAxes->getAxes()->count(), $axes->getAxes()->count());
        $this->assertEquals($cmpAxes, $axes);
    }
@mvrhov mvrhov added a commit to mvrhov/symfony that referenced this issue Apr 12, 2012
@mvrhov mvrhov Testcase for #3539 a1cecef
@mvrhov

I think I've adapted the testcase successfully, but the first assert doesn't fail because after the PR #3789 got merged the calculated diff is empty.
Before #3789 got merged both asserts failed, now only the second one fails.

Failing testcase is https://github.com/mvrhov/symfony/tree/3539_testcase

@webmozart
Symfony member

You forgot to set "by_reference" in the collection form to false. See #3839 and symfony/symfony-docs#1057.

@webmozart webmozart closed this Apr 12, 2012
@fabpot fabpot added a commit that referenced this issue Feb 5, 2014
@fabpot fabpot feature #10198 [Stopwatch] Allow getting duration of events without c…
…alling stop() (jochenvdv)

This PR was merged into the 2.5-dev branch.

Discussion
----------

[Stopwatch] Allow getting duration of events without calling stop()

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | [#10175](#10175)
| License       | MIT
| Doc PR        | [#3539](symfony/symfony-docs#3539)

Commits
-------

2efe461 Allow retrieving unstopped stopwatch events
d3d097d Include running periods in duration
6dfdb97
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment