php-activerecord icon indicating copy to clipboard operation
php-activerecord copied to clipboard

Eager loading bug for joins

Open anther opened this issue 11 years ago • 2 comments

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.

anther avatar Apr 04 '13 20:04 anther

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.

anther avatar Apr 04 '13 21:04 anther

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.

xak2000 avatar Jan 09 '20 05:01 xak2000