laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.42k stars 10.99k forks source link

[5.4] Query instance re-used between static scope invocations #21895

Closed jameshulse closed 6 years ago

jameshulse commented 6 years ago

Description:

The query passed to a local scope method on an eloquent model is re-used between static calls.

Steps To Reproduce:

In our code it looks something like this

// Payment.php
public function scopeSettled($query)
{
    // spl_object_hash($query); here returns the same value over the two calls
    return $query->where('status', 'Settled');
}
// PaymentController.php
$monthly_payments = Payment::settled()->where('created_at', '>=', '2017-10-01');
// $monthly_payments->count() returns 0

$yearly_payments = Payment::settled()->where('created_at', '>=', '2017-01-01');
// $monthly_payments->count() now returns 10 as the 'where' clause has been reapplied to the same query instance
sisve commented 6 years ago

A clarification that I missed when first commenting; the problem is described in a code comment.

$monthly_payments->count() now returns 10 as the 'where' clause has been reapplied to the same query instance

jameshulse commented 6 years ago

Sorry that was probably a bit unclear, to re-iterate: Queries can be mangled by subsequent calls to Payment::settled() if they have not been executed because the query instance is shared.

themsaid commented 6 years ago

I'm sorry but I can't understand what this issue is about, can you please explain in more details?

sisve commented 6 years ago

Note that he's not executing ->get() at the end of those lines. The issue is that the second call to ->where(...) is modifying the query instance that the first line has created. I have not verified this myself.

Miguel-Serejo commented 6 years ago

Unable to replicate on 5.5, running PHP 7.1.10

>>> $new_sponsors = App\Sponsor::active()->where('created_at', '>', '2017-11-01');
=> Illuminate\Database\Eloquent\Builder {#894}
>>> $old_sponsors = App\Sponsor::active()->where('created_at', '>', '2017-01-01');
=> Illuminate\Database\Eloquent\Builder {#907}
>>> $old_sponsors->count() //should return 1
=> 1
>>> $new_sponsors->count() //should return 0
=> 0
>>>  

Tried changing the order of operations around (set new after old, check counts only after setting both, etc.) and could not get the first query to return the second query's count.

jameshulse commented 6 years ago

@36864 thanks for attempting to replicate.

I have a feeling tinker is doing something special between lines that isn't happening when it is being run the normal laravel execution path (such as in a controller).

Could you please try your test in a simple controller action? Maybe return the counts as 2 fields on a json response.

themsaid commented 6 years ago

Can't replicate either, the output is select * fromuserswherestatus= ?, a new instance is assigned to $two

$one = \App\User::active()->whereName('themsaid');

    $two = \App\User::whereName('taylor');

    dd(
        $two->toSql()
    );
Miguel-Serejo commented 6 years ago

@jameshulse Sure.

Here's my test controller code:

$new_sponsors = Sponsor::active()->where('created_at', '>', '2017-11-01');
$old_sponsors = Sponsor::active()->where('created_at', '>', '2017-01-01');
dump($new_sponsors);
dump($old_sponsors);
dump($new_sponsors->count());
dump($old_sponsors->count());

And what gets dumped:

Builder {#473 ▼
  #query: Builder {#472 ▶}
  #model: Sponsor {#474 ▶}
  #eagerLoad: []
  #localMacros: []
  #onDelete: null
  #passthru: array:11 [▶]
  #scopes: []
  #removedScopes: []
}

Builder {#476 ▼
  #query: Builder {#477 ▶}
  #model: Sponsor {#526 ▶}
  #eagerLoad: []
  #localMacros: []
  #onDelete: null
  #passthru: array:11 [▶]
  #scopes: []
  #removedScopes: []
}

0

1

Different instances all around, got expected results.

Might be a 5.4 issue, can you try upgrading to 5.5 and see if the issue persists?

themsaid commented 6 years ago

Closing this for now but please feel free to continue discussions. I suggest that you test in a fresh 5.5 app since 5.4 is not supported anymore.