ping @bschussek

Can you please add a test case?

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

@gertvr ping

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

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

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.

What if you change addArticle() to:

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

        return $this;
    }

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

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

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

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

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

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

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