Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Querying relations with extra conditions not working as expected #1272

Closed
NoelDeMartin opened this issue May 11, 2013 · 7 comments
Closed

Comments

@NoelDeMartin
Copy link
Contributor

I have been some time playing with the querying system for eloquent to get something done, and recently I found something that I think is a bug.

I have a table "Teams", with id and name columns. And I have a table "Matches" with local_id and visitant_id. What I want to achieve is to get all the matches a team has played, be it as local or visitant. And finally I come up with this.

On the Teams model:

class Team extends Eloquent {
    public function matches_as_visitant() {
        return $this->hasMany('Match', 'visitant_id');
    }
}

On the Teams controller:

$team = Team::with(['matches_as_visitant' => function($query) {
            $query->orWhere('local_id', '=', '2');
            $query->orderBy('local_id', 'asc');
        }])->find(2);

The query that is run with this is:

select * from `matches` where `matches`.`visitant_id` in (?) or `local_id` = ? order by `local_id` asc

After this I expect $team->matches_as_visitant to be actually all the matches, because the eloquent model already matches the visitant_id field and I add a orWhere part where it checks for the local_id. This should work but doesn't, I've been inspecting the framework code and I found at which point this actually "stops working".

In the HasOneOrMany class we have a method matchOneOrMany:

protected function matchOneOrMany(array $models, Collection $results, $relation, $type)
{
    $dictionary = $this->buildDictionary($results);

    // Once we have the dictionary we can simply spin through the parent models to
    // link them up with their children using the keyed dictionary to make the
    // matching very convenient and easy work. Then we'll just return them.
    foreach ($models as $model)
    {
        $key = $model->getKey();

        if (isset($dictionary[$key]))
        {
            $value = $this->getRelationValue($dictionary, $key, $type);

            $model->setRelation($relation, $value);
        }
    }

    return $models;
}

Basically the $results variable is a Collection with all the matches, but then the $models variable (that is then returned) only contains the matches played as visitant, and the local matches are lost.

@taylorotwell
Copy link
Member

Yeah, the matcher is matching on the foreign key to associate the child models with their parents, so it's losing the ones where you're matching on local_id, since you're essentially matching on two foreign keys at the same time.

I think you could accomplish this by defining a separate relationship called "allMatches" and then loading that, though you still wouldn't be able to eager load it. Eager loading is not really helping you in terms of performance in this particular use case anyways as you're only loading matches for one team, but you could do something like this:

public function allMatches()
{
     return $this->hasMany('Match', 'visitant_id')->orWhere('local_id', $this->id);
}

Then...

$team = Team::find(2);
$matches = $team->all_matches;

@NoelDeMartin
Copy link
Contributor Author

Ok I see, I really ended up eager loading them because I didn't know I could add more conditions to the eloquent method and this is the only I could think of to achieve what I wanted. I think it's a bit confusing that some query results are lost, because many times I guide myself dumping the raw query that is called to see what's happening.

Thank you for you help, this works great!

@ghost
Copy link

ghost commented Oct 30, 2013

For people interested, I managed to get this to work in reverse. The original wasn't working for me.

class NewTest extends Eloquent {
    public function allDropdowns()
    {
        return $this->belongsTo('Refmas',$this->col1);
    }
$t = NewTest::find(1);
        $a = $t->allDropdowns->where('id',$t->col1)
            ->orWhere('id',$t->col2)
            ->orWhere('id',$t->col3)
            ->orWhere('id',$t->col4)
            ->get();

       foreach($a as $b){
           echo $b->col4 ."<br>"; };

@SunilEver
Copy link

@taylorotwell
Let's assume Match has an extra column sponsor. If I use following query builder.
$team = Team::find(1);
$team->allMatches()->where('sponsor' ,'company')->get();
It is translated as
select * from matches where visitant_id = matches.id and matches.id is not null or local_id = matches.id and matches.id is not null and sponsor = 'company';

For the case where visitant_id matches matches.id and sponsor is not company I still get that record.
Please give a way out.

@tisuchi
Copy link

tisuchi commented Jun 2, 2017

@taylorotwell

That's really nice. But what if I need to load allMatches as a eager loading? Does it work?
For instance-

$team = Team::where('id', 2)->with('allMatches')->first();

@rajkananirk
Copy link

@your-baby-dev
Copy link

have you got answer to use it with eager loading ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants