[bug fix] Load keys on set strategy on embed-many and reference-many #194

Merged
merged 4 commits into from Dec 2, 2011

4 participants

@iampersistent

Currently, when loading "many" data using the set strategy, the key is lost when the data is loaded. This code corrects that problem.

@kriswallsmith
Doctrine member

Does this patch enable this sort of thing?

<?php

class Person
{
  /** @ReferenceMany */
  public $relations = array();
}

$me = new Person();
$mom = new Person();
$me->relations['mother'] = $mom;
@iampersistent

Yes it does. The persisting side of it was patched a couple of months ago , sadly I'm just now getting to using the loading side of it.

It doesn't do anything with the inverse side mapping or repository method mapping, it just didn't make sense to me in this context.

@kriswallsmith
Doctrine member

👍

@jwage jwage commented on an outdated diff Nov 24, 2011
lib/Doctrine/ODM/MongoDB/PersistentCollection.php
@@ -139,8 +139,12 @@ class PersistentCollection implements BaseCollection
$this->takeSnapshot();
// Reattach NEW objects added through add(), if any.
if (isset($newObjects)) {
- foreach ($newObjects as $obj) {
- $this->coll->add($obj);
+ foreach ($newObjects as $key => $obj) {
+ if ($this->mapping['strategy'] == 'set') {
@jwage
Doctrine member
jwage added a line comment Nov 24, 2011

Need === here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jwage jwage commented on an outdated diff Nov 24, 2011
...Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php
@@ -513,7 +513,11 @@ class DocumentPersister
$data = $this->hydratorFactory->hydrate($embeddedDocumentObject, $embeddedDocument);
$this->uow->registerManaged($embeddedDocumentObject, null, $data);
$this->uow->setParentAssociation($embeddedDocumentObject, $mapping, $owner, $mapping['name'].'.'.$key);
- $collection->add($embeddedDocumentObject);
+ if ($mapping['strategy'] == 'set') {
@jwage
Doctrine member
jwage added a line comment Nov 24, 2011

Need === here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jwage jwage commented on an outdated diff Nov 24, 2011
...Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php
$className = $this->dm->getClassNameFromDiscriminatorValue($mapping, $reference);
$mongoId = $reference[$cmd . 'id'];
$id = (string) $mongoId;
$reference = $this->dm->getReference($className, $id);
- $collection->add($reference);
+ if ($mapping['strategy'] == 'set') {
@jwage
Doctrine member
jwage added a line comment Nov 24, 2011

Need === here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jwage
Doctrine member

👍

@iampersistent

Fixed. @jwage, thanks for pointing those out.

@jwage jwage merged commit d36765c into doctrine:master Dec 2, 2011
@kriswallsmith
Doctrine member

I am getting a "Cannot apply $pull/$pullAll modifier to non-array" error when I remove an embedded document from an @EmbedMany(strategy="set") collection and flush. Any ideas?

@kriswallsmith
Doctrine member

From the trace:

MongoCollection->update(array('_id' => object(MongoId)), array('$pull' => array('socialProfiles' => null)), array('safe' => true))
@tecbot

can you show your content from mongodb for this field, pls?

I think after this pull request the content shows like this:

{
   "socialProfiles": {
   "0":  {
       "name": "foo",
    },
   "1":  {
       "name": "bar",
    }
  }
}

and this is no array, it is a embedded document for mongodb.
Expected:

{
   "socialProfiles": [
    {
       "name": "foo",
    },
    {
       "name": "bar",
    }
  ],
}

So the pull requests brokes the pull operation for the set strategy

@iampersistent

I was getting that same error when I first started work on the first PR for this I can't remember off hand if handling the delete goes through there also or somewhere else. That's where I'd start looking.

@tecbot

We need to use the unset operation if we should delete a entry for the 'set' strategy

@iampersistent

@kriswallsmith, now that I actually look at the code, the problem must be with line https://github.com/doctrine/mongodb-odm/blob/master/lib/Doctrine/ODM/MongoDB/Persisters/CollectionPersister.php#L141 we probably need to do a check to see if we are using the set strategy and if we are, then don't execute line 141. Just a guess. I might have some time to look at it on Sunday

@tecbot

@IamPersistent I think you're right with your guess, because it is a normal object for mongodb and a $unset query is enough to remove a field.

@kriswallsmith
Doctrine member

The $pull hack is necessary after an $unset query such as...

db.foo.update({ _id: id }, { $unset: { "favoriteColors.2": true }})

...but not after...

db.foo.update({ _id: id }, { $unset: { "socialIdentities.facebook": true }})

I think @IamPersistent is correct that we tell the difference by checking the strategy. I'll submit a PR.

@kriswallsmith kriswallsmith added a commit that referenced this pull request Dec 3, 2011
@kriswallsmith kriswallsmith added conditional around collection delete hack
the extra $pull query is not necessary when using the set strategy (see discussion on #194)
111de2c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment