php-activerecord
php-activerecord copied to clipboard
Eager loading bug for joins
The Problem
If there's an eager load with a join, the relationships for "repeat" entries are not added to $__relationships, thus causing a lazy load to happen.
I'm making up this example on the fly and simplifying the code, so hopefully this makes sense.
class Person
has_many papers
has_many job_assignments
class Job_Assignment
belongs_to person
class Paper
belongs_to person
belongs_to color
class Signature
has_one paper
$options['joins'] = array('job_assignments');
$options['include'] = array('papers'=>array('signature'));
$people = Person::all($options);
The eager load will work for every entry that is not "duplicated" through the search.
So if a person is loaded twice due to the LEFT JOIN
.
i.e. $person[0]->id == $person[1]->id is TRUE
$person[0]->papers[0]->signature
will be fine, but
$person[1]->papers[0]->signature
will perform a lazy load on Paper->Signature
My Fix
Right now my fix is to override Model.php
and get rid of the line that clears $__relationships
in public function __clone()
. None of the tests break in this case, and I'm sure that when I write tests for this it will show that it fixes the issue.
There seems to be an effort to keep the same object instance from showing up in two different $__relationships arrays, as shown by the test test_eager_loading_clones_related_objects
. I'm not sure why it was a problem, but the current code for cloning objects breaks eager loading for joins.
Looks like some others have already had and posted a similar fix to this problem. https://github.com/kla/php-activerecord/issues/219
No one's written regressions for this though so maybe I can write something for the fix.
I'm not sure why the clone of relationship is necessary in the first place (see comments to this commit where this behavior was introduced 205bb3f67426a197675ee1b15e445a6066bf3d68), but if we clone the first level of relationships, it is better to be consistent and clone nested levels too.
So, I think the proper fix is to change Model::__clone
method from this:
public function __clone()
{
$this->__relationships = array();
$this->reset_dirty();
return $this;
}
to this:
public function __clone()
{
$this->__relationships = array_map(function($rel) {
return clone($rel);
}, $this->__relationships);
$this->reset_dirty();
return $this;
}
This would recursively clone all relationships (they are Model
instances too) instead of just wiping them out.