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

[5.3] Return collections from the query builder #10552

Merged
merged 2 commits into from Feb 19, 2016

Conversation

@JosephSilber
Copy link
Contributor

@JosephSilber JosephSilber commented Oct 11, 2015

As discussed in #10478

@JosephSilber JosephSilber force-pushed the JosephSilber:qb-collection branch from 3013ec5 to 0a67c9b Oct 11, 2015
@@ -176,7 +177,7 @@ public function testChunkCanBeStoppedByReturningFalse()
$builder = m::mock('Illuminate\Database\Eloquent\Builder[forPage,get]', [$this->getMockQueryBuilder()]);
$builder->shouldReceive('forPage')->once()->with(1, 2)->andReturn($builder);
$builder->shouldReceive('forPage')->never()->with(2, 2);
$builder->shouldReceive('get')->times(1)->andReturn(['foo1', 'foo2']);
$builder->shouldReceive('get')->times(1)->andReturn(new COllection(['foo1', 'foo2']));

This comment has been minimized.

@arrilot

arrilot Oct 11, 2015
Contributor

typo

@JosephSilber JosephSilber force-pushed the JosephSilber:qb-collection branch 2 times, most recently from 31cec77 to 526c058 Oct 11, 2015
@JosephSilber JosephSilber force-pushed the JosephSilber:qb-collection branch from 526c058 to 9c24a12 Oct 12, 2015
@GrahamCampbell GrahamCampbell added bug database and removed bug labels Oct 14, 2015
@taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Nov 10, 2015

Going to hold off on this. DB::select returns an array and the query builder is just a light wrapper around PDO queries, so it's sensible for it to return arrays and we shouldn't feel obligated to make it return Collections just because the ORM does.

@taylorotwell taylorotwell reopened this Nov 10, 2015
@taylorotwell taylorotwell changed the title [5.2] Return collections from the query builder [5.3] Return collections from the query builder Nov 10, 2015
@taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Nov 10, 2015

We'll aim for making this change in 5.3, which will allow us to put a deprecation notice in the 5.2 deprecation notes that get on query builder will start returning a collection. That will give everyone a full 6 months notice at least that they need to accomodate this.

@JosephSilber JosephSilber force-pushed the JosephSilber:qb-collection branch 3 times, most recently from 2df9ec1 to b03fa63 Nov 16, 2015
@JosephSilber JosephSilber force-pushed the JosephSilber:qb-collection branch 5 times, most recently from acc8b4e to 3336d4b Dec 6, 2015
@GrahamCampbell
Copy link
Member

@GrahamCampbell GrahamCampbell commented Dec 25, 2015

Looking pretty nice to me. 👍 🚀

@JosephSilber JosephSilber force-pushed the JosephSilber:qb-collection branch from 3336d4b to 2c61fb8 Jan 29, 2016
@jesseleite
Copy link

@jesseleite jesseleite commented Jan 29, 2016

We'll aim for making this change in 5.3, which will allow us to put a deprecation notice in the 5.2 deprecation notes that get on query builder will start returning a collection. That will give everyone a full 6 months notice at least that they need to accomodate this.

@taylorotwell Is this still a thing for 5.3?

taylorotwell added a commit that referenced this pull request Feb 19, 2016
[5.3] Return collections from the query builder
@taylorotwell taylorotwell merged commit e87e4a5 into laravel:master Feb 19, 2016
2 checks passed
2 checks passed
StyleCI The StyleCI analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@JosephSilber JosephSilber deleted the JosephSilber:qb-collection branch Feb 19, 2016
@acasar
Copy link
Contributor

@acasar acasar commented Feb 19, 2016

@JosephSilber @taylorotwell Can we make a helper method so that we can just call ->all() instead of ->get()->all() if we want an array?

@JosephSilber
Copy link
Contributor Author

@JosephSilber JosephSilber commented Feb 19, 2016

I personally don't think we need one, but if we do it should definitely not be all.

Maybe asArray or something. But meh.

@acasar
Copy link
Contributor

@acasar acasar commented Feb 19, 2016

Nah, that's too long imo. I'm either all or nothing :)

Of course in that case we'd also need to update Eloquent builder so that all wouldn't return an array.

@JosephSilber
Copy link
Contributor Author

@JosephSilber JosephSilber commented Feb 19, 2016

@acasar
Copy link
Contributor

@acasar acasar commented Feb 19, 2016

Exactly. If we do this, it should be added as an alias of get() because it will otherwise propagate to the QueryBuilder and we'd have inconsistent behaviour: User::all() vs. User::where(...)->all()

@JosephSilber
Copy link
Contributor Author

@JosephSilber JosephSilber commented Feb 19, 2016

Which would be counter to everything we're trying to accomplish here. We'll once again end up with identical methods that return different types.

How often do you need an array that you want a separate method for that?

@GrahamCampbell
Copy link
Member

@GrahamCampbell GrahamCampbell commented Feb 19, 2016

I agree this is unneeded. It's only encourage people to not upgrade properly.

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

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.