[5.0] Eloquent belongsToMany chunk #8045
Conversation
codelight89
force-pushed
the
belongs-to-many-chunk
branch
from
50cdf5c to
371e17e
Compare
|
Tests? |
codelight89
force-pushed
the
belongs-to-many-chunk
branch
3 times, most recently
from
6b0c7fc to
e098e6c
Compare
| $callbackExecutionAssertor->shouldReceive('checkModelNameAttribute')->with('dayle')->once(); | ||
| $callbackExecutionAssertor->shouldReceive('checkModelNameAttribute')->with('graham')->once(); | ||
| $callbackExecutionAssertor->shouldReceive('checkForeignKey')->with('user_id')->times(3); | ||
| $callbackExecutionAssertor->shouldReceive('checkOtherKey')->with('role_id')->times(3); |
There was a problem hiding this comment.
Wooooow. That's not a good amount of mocking. Better not to mock this.
codelight89
force-pushed
the
belongs-to-many-chunk
branch
from
71e7345 to
e098e6c
Compare
|
I don't know what "cs" means, but check it, please. =) |
|
Code style/coding standard. You need to put all braces on their own line. |
|
Done. And what about this bug in other versions of laravel? |
There are no plans to backport this fix to 4.x. If it's merged into 5.0, we always merge 5.0 -> 5.1, so it'll be included in all 5.x releases. |
| // On each chunk result set, we will pass them to the callback and then let the | ||
| // developer take care of everything within the callback, which allows us to | ||
| // keep the memory low for spinning through large result sets for working. | ||
| call_user_func($callback, $result); |
There was a problem hiding this comment.
Shouldn't this check for false like the query builder?
There was a problem hiding this comment.
there is no check in the eloquent builder
There was a problem hiding this comment.
Yes there is. Follow the link Joseph provided.
There was a problem hiding this comment.
Oh, right I see. Maybe the Eloquent builder needs a check then?
There was a problem hiding this comment.
BTW, is there any reason this logic is duplicated all over the place?
There was a problem hiding this comment.
Not sure, would be nice to abstract it. Would a trait do the job?
There was a problem hiding this comment.
Any reason the eloquent builder doesn't just call it directly on the base query builder?
There was a problem hiding this comment.
This chunk functions are equivalent except called get()
There was a problem hiding this comment.
Actually the same realization of method (like in Query Builder) must work in BelognsToMany class too. Maybe it is a reason to create ChunkableTrait with this method and use it in Query Builder, Eloquent Builder and BelongsToMany Relation?
GrahamCampbell
changed the title
|
So you will merge this pull request? Or better to do it as I described above (with trait). |
|
Taylor is the one who decides what gets merged, particularly for sensitive components like the database component. |
codelight89
force-pushed
the
belongs-to-many-chunk
branch
from
bb946df to
1a4bb2b
Compare
|
Is it not possible to just add the select fields and then call the usual chunk method on the Eloquent builder? Why duplicate the logic? |
|
@taylorotwell, I don't think it can be solved this way. We need to hydrate pivot relation for every chunk result that we pass to callback. |
|
Yeah you could pass it to the callback, and then call the user supplied callback from that callback. Make sense? :) |
codelight89
force-pushed
the
belongs-to-many-chunk
branch
from
1a4bb2b to
33687a1
Compare
|
@taylorotwell, check it. :) |
|
Is this tested? Do we need an integration test here? |
codelight89
force-pushed
the
belongs-to-many-chunk
branch
from
dad9a01 to
c39cbce
Compare
|
@taylorotwell, I added a small test. Think that's enough. |
[5.0] Eloquent belongsToMany chunk
Fixes this issue #6281