Skip to content

[Form] collection type is broken #3539

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

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

[Form] collection type is broken #3539

mvrhov opened this issue Mar 9, 2012 · 13 comments

Comments

@mvrhov
Copy link

mvrhov commented Mar 9, 2012

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

stloyd commented Mar 9, 2012

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

mvrhov commented Mar 9, 2012

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

@mvrhov
Copy link
Author

mvrhov commented Mar 12, 2012

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

@stof
Copy link
Member

stof commented Mar 12, 2012

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

mvrhov commented Mar 12, 2012

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

stof commented Mar 12, 2012

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

@webmozart
Copy link
Contributor

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

@mvrhov
Copy link
Author

mvrhov commented Apr 7, 2012

I can't verify before Tuesday/Wednesday.

@mvrhov
Copy link
Author

mvrhov commented Apr 10, 2012

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

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

@mvrhov
Copy link
Author

mvrhov commented Apr 11, 2012

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

mvrhov commented Apr 12, 2012

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

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

fabpot added a commit that referenced this issue Feb 5, 2014
…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
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

4 participants