Children are duplicated in parent's collection when using generated node names

[sarcher]

When adding children that use auto-generated node names to a parent's children collection, each child is duplicated upon calling DocumentManager::flush().

Consider two documents, the first of which has only a collection of children:

use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\ODM\PHPCR\Mapping\Annotations as PHPCRODM;

/**
 * @PHPCRODM\Document
 */
class ExampleParent
{
  /**
   * @PHPCRODM\ParentDocument
   */
  private $parent;

  /**
   * @PHPCRODM\Nodename
   */
  private $name;

  /**
   * @PHPCRODM\Children
   */
  private $details;

  public function __construct()
  {
    $this->name = 'example_parent';
    $this->details = new ArrayCollection();
  }

  // more stuff

}

And the second of which just stores some content:

use Doctrine\ODM\PHPCR\Mapping\Annotations as PHPCRODM;

/**
 * @PHPCRODM\Document
 */
class ExampleChild
{
  /**
   * @PHPCRODM\ParentDocument
   */
  private $parent;

  /**
   * @PHPCRODM\Id
   */
  private $id;

  /**
   * @PHPCRODM\String
   */
  private $content;

  public function __construct()
  {
  }

  // more code

}

Now, we can create a parent and add some children:

    // assume we have already retrieved $documentManager

    $exampleParent = new ExampleParent();
    $exampleParent->setParent($documentManager->find(null, '/'));

    $documentManager->persist($exampleParent);
    $documentManager->flush();

    // create some dummy children
    foreach (array('test_one', 'test_two', 'test_three') as $value) {

      $exampleChild = new ExampleChild();
      $exampleChild->setParent($exampleParent);
      $exampleChild->setContent($value);

      $exampleParent->getDetails()->add($exampleChild);

      $documentManager->persist($exampleChild);

    }

    echo '*** BEFORE FLUSH ***' . PHP_EOL;
    foreach ($exampleParent->getDetails() as $key => $detail) {
      echo $key . ': ' . $detail->getId() . ': ' . $detail->getContent() . PHP_EOL;
    }

    $documentManager->flush();

    echo PHP_EOL . '*** AFTER FLUSH ***' . PHP_EOL;
    foreach ($exampleParent->getDetails() as $key => $detail) {
      echo $key . ': ' . $detail->getId() . ': ' . $detail->getContent() . PHP_EOL;
    }

The result is not quite what we would expect!

*** BEFORE FLUSH ***
0: /example_parent/1929002033: test_one
1: /example_parent/599553769: test_two
2: /example_parent/286007372: test_three

*** AFTER FLUSH ***
0: /example_parent/1929002033: test_one
1: /example_parent/599553769: test_two
2: /example_parent/286007372: test_three
1929002033: /example_parent/1929002033: test_one
599553769: /example_parent/599553769: test_two
286007372: /example_parent/286007372: test_three

Before the DocumentManager::flush() operation, the ArrayCollection contains the three new Documents with non-specific indices. After the operation, the new node names are used as the indices, but the originals are not removed from the collection, resulting in a duplicate of each.

The offending code is in UnitOfWork::computeChangeSet() lines 1240-1247 as such:

$childNames = array();
if ($actualData[$fieldName]) {
    foreach ($actualData[$fieldName] as $nodename => $child) {
        $nodename = $this->getChildNodename($id, $nodename, $child, $document);
        $actualData[$fieldName][$nodename] = $this->computeChildChanges($mapping, $child, $id, $nodename, $document);
        $childNames[] = $nodename;
    }
}

It appears that the result of this block is that $actualData[$fieldName] will always end up with duplicated data if the node name changes as part of UnitOfWork::getChildNodename(), which is guaranteed if the child is new and uses a generated name.

It seems this could be fixed with a line of code to remove the old index upon insertion of the new one, but I am less familiar with the code as a whole so it's possible I am missing something or that this is intended (although it seems unlikely).

In the above sample code, if the $exampleParent->getDetails()->add($exampleChild); line is omitted, the children collection is never populated at all. This seems as though it may also be a bug, or perhaps just not yet implemented.

If these are in fact bugs I am happy to fix them and submit a pull request, I just don't want to do so without a better understanding of the intent.

[lsmith77]

can you turn this into a failing test case?

[sarcher]

First commit contains the functional tests; second commit contains the fix. I broke them up so you could more easily observe the failures against the current code.

Note the use of strcmp in the fixed code, since array keys can be either integers or strings we need a type-safe comparison between the two.

Comments welcome -- I think I understand things but I am pretty new to the code. This did fix the issue I was having, and all of the existing tests pass so I hope there is no regression introduced.

[lsmith77]

can you open a PR with your changes? makes reviewing easier

[sarcher]

Done, although it seems to have created a new issue, so clearly I am not doing it correctly. : )

[lsmith77]

that is the normal behavior .. so all is well :)

[dbu]

fixed in #390 right? please reopen if i am mistaken and there is something left.