$xpdoObject->remove() & relationships #72

Open
rtripault opened this Issue Aug 31, 2015 · 9 comments

Projects

None yet

3 participants

@rtripault
Collaborator

Summary

First of all, i'm not sure if this is a bug (for my specific case) or rather the expected behavior.
After moving all relationships from one object to another, removing the object where all relationships were initially "attached to", the relationships are also deleted (i believe because of some kind of caching).

Step to reproduce

<?php
$object1 = $xpdo->getObject('class', 'pk1');
$object2 = $xpdo->getObject('class', 'pk2');

foreach ($object1->getMany('relationships') as $r) {
    $r->set('relationship_object_id', $object2->get('id');
    $r->save();
}

$object1->remove();

Observed behavior

All related objects are also deleted whith $object1->remove()
$object2->getMany('relatioships) is empty

Expected behavior

Related objects should be attached to $object2.
I believe this is because getMany uses some caching mechanism. If so, $xpdoObject->remove() should not make use of the caching (or provide some argument to "bypass" the caching mechanism).

@opengeek
Member

Doesn't matter which ones they are "attached" to, if you remove an object with composite relationships, those related objects will be remove()'d by either of their parents unless you kill the relationships from one of the instances that carries them.

@rtripault
Collaborator

Well, that's somehow what i meant, if the code, i would have expected $r->set('relationship_object_id', $object2->get('id'); to "kill" that relationship from parent object1 to object2

@opengeek
Member

Why would it kill the relationship; because I'm get()ing the field? You would need to manually remove the relationships by unsetting the FK values in $object1.

@opengeek
Member

Wait, I didn't realize you were talking about a link table (too early I guess). Though odd, I think you need to unset the related objects in $object1 because you are changing it only in a copy of the object within that foreach loop. The original related objects in $object1 would remain unchanged.

@rtripault
Collaborator

I guess i did not make it understandable enough, let's rewind :)

object1 & object2 are different objects (same class, let's say x).
those objects can have relationships (composite, let's say alias is relationships) to other objects (let's say class y).
y.relationship_object_id is the column defining the "related parent id/pk".

When iterating relationships (using $object1->getMany('relationships')) to change the "related parent id" (using $relation->set('relationship_object_id', $object2->get('id')); $relation->save();), it appears $object1 still "hold" the related objects, while they should no more "belong to it".

If we call $object1->remove() (on the existing instance), previously related objects will also be deleted (i believe this is because xPDOOBject::_getRelatedObjectsByFK first checks xPDOObject::_relatedObjects[$alias]).
If i retrieve a new instance of object1, previously related objects won't be deleted.

Does that makes more sense that way ?

@opengeek
Member

It does make sense, again, because you are essentially working on a copy of the related objects, not the ones actually attached to $object1. There is no shared cache for those objects in memory to prevent a stale object from acting the way $object->remove() does. I can't think of any other way to prevent such a thing, but PR's welcome. :)

@rtripault
Collaborator

The only solution that came to my mind so far, is to add an additional parameter to "force retrieve" related objects from DB and not the "parent" cache, ie.

protected function & _getRelatedObjectsByFK($alias, $criteria= null, $cacheFlag= true, $skipCache = false) {
        $collection= array ();
        if (!$skipCache && isset($this->_relatedObjects[$alias]) && (is_object($this->_relatedObjects[$alias]) || (is_array($this->_relatedObjects[$alias]) && !empty ($this->_relatedObjects[$alias])))) {
            $collection= & $this->_relatedObjects[$alias];
        } else {
            $fkMeta= $this->getFKDefinition($alias);
            if ($fkMeta) {
                $fkCriteria = isset($fkMeta['criteria']) && isset($fkMeta['criteria']['foreign']) ? $fkMeta['criteria']['foreign'] : null;
                if ($criteria === null) {
                    $criteria= array($fkMeta['foreign'] => $this->get($fkMeta['local']));
                    if ($fkCriteria !== null) {
                        $criteria= array($fkCriteria, $criteria);
                    }
                } else {
                    $criteria= $this->xpdo->newQuery($fkMeta['class'], $criteria);
                    $addCriteria = array("{$criteria->getAlias()}.{$fkMeta['foreign']}" => $this->get($fkMeta['local']));
                    if ($fkCriteria !== null) {
                        $fkAddCriteria = array();
                        foreach ($fkCriteria as $fkCritKey => $fkCritVal) {
                            if (is_numeric($fkCritKey)) continue;
                            $fkAddCriteria["{$criteria->getAlias()}.{$fkCritKey}"] = $fkCritVal;
                        }
                        if (!empty($fkAddCriteria)) {
                            $addCriteria = array($fkAddCriteria, $addCriteria);
                        }
                    }
                    $criteria->andCondition($addCriteria);
                }
                if ($collection= $this->xpdo->getCollection($fkMeta['class'], $criteria, $cacheFlag)) {
                    $this->_relatedObjects[$alias]= array_diff_key($this->_relatedObjects[$alias], $collection) + $collection;
                }
            }
        }
        if ($this->xpdo->getDebug() === true) {
            $this->xpdo->log(xPDO::LOG_LEVEL_DEBUG, "_getRelatedObjectsByFK :: {$alias} :: " . (is_object($criteria) ? print_r($criteria->sql, true)."\n".print_r($criteria->bindings, true) : 'no criteria'));
        }
        return $collection;
    }

That way, when using xPDOObject->remove we could "force" pulling fresh data.
That might not be pretty, and furthermore, it would only take care of the "remove" situation.

@opengeek
Member

I think we would need to use the existing $cacheFlag parameter set to false to indicate that the results should not be retrieved from nor stored in cache and alter the behavior similarly to the way you did using your proposed $skipCache parameter. I need to put some more thought into this...

@wshawn
wshawn commented Sep 3, 2015

To get around this I have used clone in the past. That way the first object can die and leave the cloned object unaffected.

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