[Form] Fixed undefined index in writeProperty when saving value arrays #5257

Closed
wants to merge 1 commit into
from

Projects

None yet

6 participants

@gertvr
gertvr commented Aug 14, 2012

When removing an item from an array and the previous value contains more than one identical copy of that item, the second unset fails with undefined index.

@gertvr gertvr [Form] Fixed undefined index in writeProperty when saving value arrays
When removing an item from an array and the previous value contains more than one identical copy of that item, the second unset fails with undefined index.
dab657b
@fabpot
Member
fabpot commented Aug 14, 2012

ping @bschussek

@webmozart
Member

Can you please add a test case?

@fabpot
Member
fabpot commented Oct 5, 2012

@gertvr Can you add a unit test to prove the issue (and avoid any future regressions)?

@stof
Member
stof commented Oct 13, 2012

@gertvr ping

@gertvr
gertvr commented Oct 13, 2012

I'm on holiday till the 22nd, I'll add it afterwards!

@fabpot
Member
fabpot commented Oct 27, 2012

@gertvr Will you have time in the coming days to add some unit tests?

@jfsimon
Contributor
jfsimon commented Mar 26, 2013

I made some tests and fellt on a problematic case which seems unsolvable. I'll try to explain, if I missed something, please tell me.

Let's take following model entities:

class Author
{
    /** @var Article[] */
    private $articles = array();

    public function addArticle(Article $article)
    {
        $this->articles[] = $article;

        return $this;
    }

    public function removeArticle(Article $article)
    {
        foreach ($this->articles as $index => $value) {
            if ($article === $value) {
                unset($this->articles[$index]);
            }
        }

        return $this;
    }
}

class Article
{
    public $title;
}

Now let build an author with some articles, then use PropertyAccessor to set articles to author:

$author = new Author();
$article1 = new Article();
$article2 = new Article();
$accessor = new PropertyAccessor();

// set initial condition of the use case
$author->addArticle($article1)->addArticle($article2)->addArticle($article1);

// and now, the problematic assignment
$accessor->setValue($author, 'articles', array($article1, $article2));

The problem is that now, we have only $article2 in the $author::$articles property. $article1 has been removed. Why? Because of the Author::removeArticle() implementation:

  • accessor gather values to add and remove from collection;
  • then it applies addAtricle and removeArticle on these elements.

In our case, accessor:

  • does nothing with the first $article1 instance, cause it's in the initial and set collections;
  • does nothing with the $article2 instance for the same reason;
  • removes second $article1 instance as it's in the initial collection, but not the set one.

Out Author::removeArticle() method removes all instances of $article1, not only the last one.

@stloyd
Contributor
stloyd commented Mar 26, 2013

What if you change addArticle() to:

    public function addArticle(Article $article)
    {
        if (!$this->articles->contains($article)) {
            $this->articles[] = $article;
        }

        return $this;
    }
@jfsimon
Contributor
jfsimon commented Mar 26, 2013

@stloyd if I do that, this will only affect the initial Author::$articles assignments. The accessor, in this case, never uses this method. The problem is that Author::removeArtcile() implementation is incompatible with the PropertyAccessor::writeProperty() one. I'm not sure to be clear, tell me if I'm not.

@webmozart
Member

@stloyd is right. Preventing duplicates in your collection should fix your problem. The property accessor currently expects collections to be free of duplicates.

@jfsimon
Contributor
jfsimon commented Apr 18, 2013

@bschussek that's why I told the original problem was unsolvable.
Of course, preventing duplicates fix the problem; but the duplication is an axiom of the original issue.
My message was just the demonstration of why collection must not contain duplicates.

@webmozart
Member

I see. As far as I understand #5013 would solve your problem then?

@webmozart
Member

that is, by disabling adders and removers and using the setter instead.

@fabpot
Member
fabpot commented Apr 20, 2013

@bschussek What do we do with this PR? Should we merge this fix?

@jfsimon
Contributor
jfsimon commented Apr 20, 2013

@fabpot I tested it and this PR does not fix the problem. As @stloyd said, collection must not contain duplicate objects. This issue is not fixable. I guess the only way to avoid this problem would be to do what @bschussek suggest: disabling adders and removers.

@fabpot fabpot closed this Apr 20, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment