Framework: Return collection from the query builder

[JosephSilber]

This issue was created to get community feedback.

---

It would be awesome if we could have the query builder return a collection:

$collection = DB::table('posts')->get();

Given that PHP in some situations infamously does not treat ArrayAccess objects as arrays, this would be breaking BC. This will obviously be targeted for the next release of Laravel.

Pros

1. Makes it consistent with Eloquent. You'll find countless questions on StackOverflow asking why some collection method can't be called on a query builder result.
2. It's just much cleaner than this: collect(DB::table('posts')->get()). Laravel prides itself on making all these tiny interactions more enjoyable. This should not be any different.

3. Native PHP arrays are just useless :smile:

   Cons

4. It's a breaking change. Some people's tests might fail because they're using array_* or typehinting for an array. If their code can't be updated to use a collection, they'll have to append ->all() to their results.

If people don't have good test coverage, they'll have to search their codebase for ->get( and manually tack on ->all() wherever necessary. Not the end of the world, but definitely a breaking change.

---

Overall I think this change is well worth it and would love some feedback from the community. If you use the query builder directly, please leave a comment below with your thoughts either way, or if I missed any pros/cons.

Thank you :heart:

[crynobone]

:-1:

Not a fan of this, in my use case we appreciate that the query builder use simple array because it just what we need, and only use Collection if we need to. Having it return as Collection and then manipulate it again as another collection is just redundant.

If both QB and Eloquent a collection, what would even be the benefit to choose QB when you already including additional process to it. We used QB because we want fast array, you can throw that it just 0.001 sec per x but it the main reason why I would use QB for certain things instead of Eloquent (if not why would even anyone bother to use QB).

Why is Model::find(x)->update() isn't the same with Model::update(), why not "consistency" a factor here?

PHP arrays are just evil

What?

Native PHP arrays are just useless

Collection methods are wrap around array_* native functions. It's not useless.

[hipsterjazzbo]

:+1:

I'm a huge fan of this. Consistency is so valuable, and I wind up doing collect(DB::table('posts')->get()) almost 100% of the time. I really don't see any practical downsides, other than the "We can make do without it" arguement that gets brought up a lot. That doesn't really make sense to me.

[rizqidjamaluddin]

The idealist in me would very much enjoy this. It makes most sense being a collection, and it would provide good consistency (it's weird for one get() to be a collection and another an array).

As you said, though, the backwards compatibility penalty is in a really bad shape (and hugely because of PHP's wonky non-object arrays). I know a great deal of legacy code that typehints QB return values with arrays (again, because PHP to my knowledge has no clean and consistent way to typehint that accepts both arrays and collections, grumble grumble). This same issue applies to code that uses the array_x functions. This is a fairly major compatibility break, as it can easily affect pretty much any codebase, component or package that uses the Query Builder.

So while I agree with the sentiment, I don't think it'll be possible unless we solve the compatibility issue.

An alternate resolution I've pondered in the past would be to provide a configurable return class for the QB (and, be extension, probably Eloquent as well). This has an additional side-benefit: users would be able to use their custom collection classes (their own classes, another framework's collection, an extension of laravel's collection etc). This doesn't solve the _whole_ issue, though, and potentially make it worse: a person might be using a package that demands the array-returning QB while another package wants it returning collections.

[jesseleite]

Makes it consistent with Eloquent. You'll find countless questions on StackOverflow asking why some collection method can't be called on a query builder result.

It's weird for one get() to be a collection and another an array

IMHO, collections and consistency between query builder and Eloquent. Though I assume this would have to be for 5.2 because breaking, with mention in upgrade notes?

[jbrooksuk]

I'm with @HipsterJazzbo, every time I run DB::select or whatever, I tend to wrap it in a new collection.

[GrahamCampbell]

If we do make this change, 5.2 is a good place to make it IMO.

[GrahamCampbell]

I was meaning rather than holding off until 5.3, I'd rather we did it in 5.2 if at all.

[JosephSilber]

Even Laravel Doctrine is now returning collections!

[phroggyy]

I'll have to agree with both sides here; @crynobone makes a very good point that arrays are, for accessing data, faster. And indeed, collection methods merely wrap around array functions. That being said, I believe if performance is an issue, you've got more issues such as running an entire framework (or inefficient SQL queries or tons of other things that aren't completely optimised for special scenarios).

Now, I would appreciate this actually being implemented. However, if you want a middle ground, you can always offer the option; allow the user whether to have collections returned or not (and default to true). This would allow the users who actually want arrays returned, for performance or else, to get that.

Other than that, I'm totally in favour of this.

[jesseleite]

@phroggyy Where would you give them the option to set collections/arrays as default return type? /config/database.php? I agree though, if performance is an issue, you probably have more problems affecting performance than just Collection instantiation on your result set.

That said, I'm still in definitely favour of Collections for consistency between Eloquent and Query Builder get() methods.

[phroggyy]

Database config or a static variable on the Collection.
ke 7. lokakuuta 2015 klo 22.44 Jesse Leite [email protected]
kirjoitti:

@phroggyy https://github.com/phroggyy Where would you give them the
option to set collections/arrays as default return type?
/config/database.php? I agree though, if performance is an issue, you
probably have more problems affecting performance than just Collection
instantiation on your result set.

That said, I'm still in definitely favour of Collections for consistency
between Eloquent and Query Builder get() methods.

—
Reply to this email directly or view it on GitHub
https://github.com/laravel/framework/issues/10478#issuecomment-146321947
.

[JosephSilber]

Sorry to everyone who proposed it, but having it be configurable is a terribly stupid idea.

Not only will it lead to _even more_ confusion, but it'll lead to terrible inconsistency for packages and such.

Having it configurable is not on the table. The question is whether we should change it or not.

---

Performance is a non-issue.

This is _nothing_ like Eloquent. Eloquent has to spin up separate model objects for each record. If you fetch hundreds or thousands of records, that could occasionally be a problem.

A collection is a single object, regardless of how many records you fetch. You can always just call all() on the results to have a simple array. You'd have created one single throw-away object. No biggie.

[phroggyy]

Very good point. So stating clearly this time: I'm all in favour of this
for consistency.
ke 7. lokakuuta 2015 klo 23.05 Joseph Silber [email protected]
kirjoitti:

Sorry to everyone who proposed it, but having it be configurable is a
terribly stupid idea.

Not only will it lead to _even more_ confusion, but it'll lead to
terrible inconsistency for packages and such.

Having it configurable is not on the table. The question is whether we

should change it or not.

Performance is a non-issue. This is _nothing_ like Eloquent. Eloquent has
to spin up models for each record in the database. If you're querying
thousands of records, that could occasionally be a problem. But a
collection is a single object, regardless of how many records you have.

—
Reply to this email directly or view it on GitHub
https://github.com/laravel/framework/issues/10478#issuecomment-146329192
.

[GrahamCampbell]

Performance is a non-issue.

Yeh, returning collections rather than arrays takes very little time to execute in comparison to any database queries.

[arrilot]

:+1:
The upgrade doesn't look too painful to me.
And if you are not ready for such difficulties you can always stay on L5.1 LTS.

[crynobone]

Yeh, returning collections rather than arrays takes very little time to execute in comparison to any database queries.

Regardless if you trying to process 10,000 rows or 100,000 rows? We use query builder for low level analytics process, and extra seconds is not what we're looking for.

[JosephSilber]

@crynobone Yes. Even if you're processing 17 billion records. It doesn't matter.

Like I said, this is not Eloquent.

You get one single wrapper collection object, and that's it. It's still the same array underneath it all.

[rizqidjamaluddin]

I'd support it be a BC change on the next BC-accepting version of Laravel (5.2 presumably). I also agree that making it configurable is a tad strange - though it's with precedent, given Model::newCollection. But I digress.

Worth noting that packages will want to maintain a 5.1 and 5.2 branch separately for this purpose, given 5.1's LTS. No different than packages maintaining an L4 branch really.

The performance penalty for a single collection object is pretty much negligible since it basically just wraps the array - if you want to get out of the collection using ->all(), the only loss is the bit of memory set for that temporary collection.

[crynobone]

<?php

ini_set('memory_limit', '1024M');

require "vendor/autoload.php";

use Symfony\Component\Stopwatch\Stopwatch;

function formatSeconds($ms) {
    return ($ms/1000000).'sec';
}

function formatBytes($size, $precision = 2) {
    $base = log($size, 1024);
    $suffixes = array('', 'k', 'M', 'G', 'T');

    return round(pow(1024, $base - floor($base)), $precision) . $suffixes[floor($base)];
}

$stopwatch = new Stopwatch();

$data = array_fill(0, $argv[1], [
    'uuid' => 1,
    'latitude' => 0,
    'longitude' => 0,
    'other' => '<foo>5</foo><bar>3</foo>',
    'speed' => 50,
]);

$stopwatch->start('iterate');

$speeding = [];

foreach ($data as $item) {
    if ($item['speed'] > 100) {
        $speeding[] = $item['uuid'];
    }
}

$iterate = $stopwatch->stop('iterate');

var_dump(
    formatSeconds($iterate->getDuration()), formatBytes($iterate->getMemory()), formatBytes(memory_get_peak_usage(true))
);

vs

<?php

ini_set('memory_limit', '1024M');

require "vendor/autoload.php";

use Illuminate\Support\Collection;
use Symfony\Component\Stopwatch\Stopwatch;

function formatSeconds($ms) {
    return ($ms/1000000).'sec';
}

function formatBytes($size, $precision = 2) {
    $base = log($size, 1024);
    $suffixes = array('', 'k', 'M', 'G', 'T');

    return round(pow(1024, $base - floor($base)), $precision) . $suffixes[floor($base)];
}

$stopwatch = new Stopwatch();

$fill = array_fill(0, $argv[1], [
    'uuid' => 1,
    'latitude' => 0,
    'longitude' => 0,
    'other' => '<foo>5</foo><bar>3</foo>',
    'speed' => 50,
]);

$stopwatch->start('iterate');

$data = Collection::make($fill);
$speeding = [];

foreach ($data as $item) {
    if ($item['speed'] > 100) {
        $speeding[] = $item['uuid'];
    }
}

$iterate = $stopwatch->stop('iterate');

var_dump(
    formatSeconds($iterate->getDuration()), formatBytes($iterate->getMemory()), formatBytes(memory_get_peak_usage(true))
);

Sure you can say the performance increase is minimal, but peak memory?

With only memory_limits=1024M, Collection can make out to 5.3 mill records while simple array can reach double that value at 10.6 mill. Now this is only single process.

[JosephSilber]

@crynobone don't loop over the collection. Change the loop in your second example to this:

foreach ($data->all() as $item) {
    if ($item['speed'] > 100) {
        $speeding[] = $item['uuid'];
    }
}

Like I said, if you really care about this tiny difference in performance, get the array from the collection right away. The only overhead you'll now have is creating and throwing away the collection object, which is truly negligible.

[crynobone]

don't loop over the collection. Change the loop in your second example to this:

If you actually try it, doing that or even

$simple = $data->all();

foreach ($simple $as $item) {
    // note that now I'm not doing anything in the loop, still no different.
}

Still generate minimal different.

[JosephSilber]

@crynobone your code keeps a reference to both the raw array and the collection. Having them both in the same scope prevents the array from being garbage collected.

Try this:

$data = array_fill(0, $argv[1], [...]);

$stopwatch->start('iterate');

$data = Collection::make($data)->all();

$speeding = [];

foreach ($data as $item) {
    if ($item['speed'] > 100) {
        $speeding[] = $item['uuid'];
    }
}

BTW, which version of PHP are you using to run these?

[crynobone]

$ php -v
PHP 5.6.13 (cli) (built: Sep 30 2015 07:12:20)
Copyright (c) 1997-2015 The PHP Group
Zend Engine v2.6.0, Copyright (c) 1998-2015 Zend Technologies
    with Zend OPcache v7.0.4-dev, Copyright (c) 1999-2015, by Zend Technologies
    with Xdebug v2.3.2, Copyright (c) 2002-2015, by Derick Rethans

[JosephSilber]

@crynobone I tested this locally.

Using the code I provided you in my last comment, the memory usage is cut in half - making it no different than the raw array.

[phanan]

I'd :+1: for this. It's not that turning the DB result into a collection is irreversible – you can always access the original array with Collection::all() and proceed with native, lower-level functions – so why not?

[jbrooksuk]

you can always access the original array with Collection::all() and proceed with native, lower-level functions – so why not?

Yeah, Collections are Arrayable anyway.

[rizqidjamaluddin]

To be clear, Arrayable doesn't let you swap classes into functions that accept PHP arrays, because, y'know, PHP. Real pity, because if it did we wouldn't be having this conversation!

[jbrooksuk]

To be clear, Arrayable doesn't let you swap classes into functions that accept PHP arrays, because, y'know, PHP. Real pity, because if it did we wouldn't be having this conversation!

I still can't wrap my head around why. You'd think that Arrayable === array, but nope.

[rizqidjamaluddin]

Because Arrayable is a Laravel construct. PHP has a variety of "this-should-act-like-an-array" classes as well (ArrayObject, SplFixedArray, and other specific SPL classes for things like stacks, queues and heaps), but PHP does arrays separately from how it does objects, so unfortunately they won't be fully compatible for a while.

[jbrooksuk]

I was thinking of ArrayAccess, my bad.

[GrahamCampbell]

As long as something is iteratable, then we're good, for the most part.

[arrilot]

You still will need to find during 5.1 => 5.2 upgrade every DB::table() call and append ->all() to all those queries.
You will have no confidence that nothing broke otherwise unless you have a great test coverage

[GrahamCampbell]

You will have no confidence that nothing broke otherwise unless you have a great test coverage

That is true for all code, and rather highlights a poor app, rather than anything bad going on with the framework.

[jesseleite]

As @JosephSilber pointed out, a single Collection object wrapped around an array is not a performance issue.

Also, RE: package development and keeping up with breaking changes... This is a perfect time to introduce breaking changes, 5.2 being the first major release after LTS. Give package devs time to adapt before the next LTS release. Keep the framework progressing.

Collections for consistency between Eloquent and Query Builder get() methods.

I don't see any problem with @JosephSilber's suggested change.

[GrahamCampbell]

As @JosephSilber pointed out, a single Collection object wrapped around an array is not a performance issue.

Yes it is. It's just not significant. ;)

[GrahamCampbell]

Also, RE: package development and keeping up with breaking changes... This is a perfect time to introduce breaking changes, 5.2 being the first major release after LTS. Give package devs time to adapt before the next LTS release. Keep the framework progressing.

Exactly.

[Sladewill]

This would add more consistency between how Laravel handles data fetching.

[freekmurze]

:+1: I think the pro's outweigh the cons. Veterans will easily adapt to this behaviour and for newcomers it's easier. Push forward!

[mikebronner]

What if we made this configurable via the fetch setting in config/database.php, by adding another option for collections? It wouldn't be a PDO constant, but maybe those could all be extracted out to strings, like many of the other database settings, maybe array, object, collection?

This way it wouldn't break existing projects, and it the default setting could be changed to collection in L5.2 for new projects.

I am in favor of this change as well (it first tripped me up when I started working with DB as I expected collections to be returned, like it already has been stated above, because of consistency).

:+1:

[mhayes14]

:+1: Simply for the consistency with Eloquent.

[JosephSilber]

@mikebronner making it configurable is not on the table: https://github.com/laravel/framework/issues/10478#issuecomment-146329192

Thanks for the feedback though :+1:

[dwightwatson]

All for this. More consistent with Eloquent and I think returning a collection would be the expected behaviour.

[mikebronner]

@JosephSilber Completely agree if the intent is to get rid of that configuration option entirely. :)

[imjohnbon]

Consistency is invaluable. I support this.

[ShawnMcCool]

I favor the collection approach. I just can't stand working with plain arrays.

[nicolasbeauvais]

:+1: for consistency

[johnathanmdell]

:+1: in favour of consistency

[winkbrace]

I am in favor too. Please give us a collection straight up.

[davidhemphill]

I like the idea of returning Collection objects. I have an idea for those folks who care about squeezing every bit of array performance.

What if we simply added an asArray() method that could be called before a Collection object would returned? Then, the fetching would simply return the array instead of wrapping the array in a collection object.

Assuming this would land in 5.2, the folks who don't want to change their code could use LTS and eventually upgrade by find/replacing DB::table('foo') with DB::table('foo')->asArray().

I don't know, might be dumb and dangerous, but thought I'd shoot it out there. :-)

[devinfd]

I prefer the collection option but if backwards compatibility becomes a major issue then perhaps just add the ability to do 'DB::table('posts')->collect();' which in the background it just does a get and wraps that result in a collection.

[crissi]

Makes sense - do it. getAsArray() would be nice to satisfy everybody

[peterpan666]

Was thinking the same as @davidhemphill but with a ->asCollection() method or ->collect()... You got the idea

[JosephSilber]

asArray or getAsArray are all completely unnecessary. It accomplishes nothing more than calling all on the collection would:

$array = DB::table('posts')->get()->all();

The performance concerns are all unfounded. See above.

If creating _a single object_ and discarding it immediately is a concern for you, you shouldn't be using a framework such as Laravel, or even the query builder itself.

---

Again, please let's put this performance non-issue to sleep. This is a discussion about whether it's worth doing this breaking change in the name of consistency/elegance.

Please let's stick to that in subsequent comments.

[davidhemphill]

@JosephSilber I don't have a dog in the fight, but I wouldn't assume to know everything about someone else's performance problems or non-issues. I was merely offering a diplomatic solution to an issue that it appears others have. :-)

Moving on.

[KennedyTedesco]

That's a good idea for the next version. :+1: I kinda like make it configurable too.

[lewiswharf]

:+1: to return collection.

[a7y]

:+1: for consistency with Eloquent.

[clarkeash]

:+1: sounds good

[dcrystalj]

what about having global config for not breaking change?

[a7y]

Having a config option dictate return type would be horrendous.

[unicodeveloper]

:+1: for consistency with Eloquent.

[vlakoff]

I'm using query builder and precisely enjoy the fact that it just returns good old plain arrays.

I'm not against this change but there should be a way to alter it (sorry if it's a "terribly stupid idea"), which doesn't have to be a supplementary and adding confusion config setting, it could be more of an "advanced feature" by configuring the query builder through some method call/overriding.

Just return collections by default as it pleases a clear majority of people, and give choice to the other.

[lowerends]

:+1: for returning collections.

[vlakoff]

Nevermind my previous comment. This change is great as is.

The configuration ability would so much complicate the codebase, for so little benefit, if any. It would be a terribly stupid idea.

[billmn]

In favour of return Collection too :+1:

[trideout]

What about having a getCollection() method on the query builder? This would maintain BC as well as add the functionality.

[vlakoff]

If you really, really want it back, it's easily feasible. I wouldn't recommend this, though.

use Illuminate\Database\Query\Builder;

Builder::macro('getArray', function ($columns = ['*']) {
    return $this->get($columns)->all();
});

alternative:

DB::query()->macro('getArray', function ($columns = ['*']) {
    return $this->get($columns)->all();
});

[ghost]

Consistency of the output from ->get() is a simple gain. The BC issues are negligible. I vote for going ahead. And as always, I feel sooner is better.

[JulienTant]

:+1: for consistency

[joseaneto]

:+1: for this

[hsleewis]

:thumbsup:

[zackkitzmiller]

Hugely :+1: on this change. Consistency is hugely important across internal APIs.

[ryanhungate]

+1 on ...->getCollection(); as an alternative to ...->get(); It shows forward progress by adding helper methods vs changing what we're all used to doing...

[rizqidjamaluddin]

Having an alternate getCollection method is counterproductive to the whole consistency approach, though, only making things more confusing - "get() returns a collection in eloquent, but not in query builder, where you have to use getCollection(), which isn't in eloquent".

Same deal with a getArray - just use get()->all(), as shown above, as it's quite literally to the same effect unless numbers pop up that indicate a massive performance loss from this.

If you want absolute efficiency, then you probably just want to throw raw queries or directly hit PDO.

Also, packages can be compatible with both the array and collection return values just by wrapping the result with new Collection(DB::table()->get()) or collect(DB::table()->get()) - this will return the same collection in both versions. And if you want an array, you can just naturally do collect(DB::table()->get())->all(). This means you packages won't need to maintain different branches just because of this difference. A bit of an ugly stopgap, but it's there.

[ivanhoe011]

:+1: consistency is totally worth breaking a few old tests...

[imalitavakoli]

simply +1
I'd like it.

[nazieb]

+1 for consistency

[Mathius17]

+1 to ->collect() or ->asCollection() and return an array as default for backwards compatibility.

Or ->array() or ->asArray(), and return a collection by default for consistency.

[kishanterry]

:+1: But add it as an option ->collect() or ->collection()

[phroggyy]

@mathius17 @kishanterry: the whole point is changing the default behaviour. You could just as well do ->get()->all() to get the array rather than having a special toArray() method

[rizqidjamaluddin]

Quick query: would this change only apply to get()? How about other functions that also have the same inconsistency, like lists(), which (iirc) return array in QB and collection in Eloquent as per 5.1?

[manogi]

:+1: I like consistency

[smallhadroncollider]

I think returning a Collection feels most intuitive.

[RemiCollin]

:+1: for consistency.

@rizqidjamaluddin : good point. these should also be taken into account.

[yasinkocak]

:+1: Return Collections

[micheleangioni]

Returning collection would be more consistent :+1: Adding a ->all() call to existent code is enough to fix the broken code/tests. However, following Semver this should be a Laravel 6.0 feature (even though I'm aware Laravel does not follow Semver strictly).

[JosephSilber]

@rizqidjamaluddin well of course. If you take a look at the accompanying PR, you'll see that lists has also been updated.

[rizqidjamaluddin]

@JosephSilber ah, didn't see the related PR. Good stuff, makes sense.

[joshhornby]

What is the reason for this being in 5.2 not 6.0? This is quite a big breaking change so including in a point version release seems odd

[hannesvdvreken]

So nobody mentioned generators (yield) yet? What if ->get() would iterate over a cursor and yield the results, in future implementations like 5.2 or 5.3? Wrapping it in a collection would prevent us from performance improvements. (please read carefully: I didn't say wrapping it in a collection is slower, just that not wrapping it would allow for performance boosts).

[jesseleite]

What is the reason for this being in 5.2 not 6.0? This is quite a big breaking change so including in a point version release seems odd

The way Laravel handles versioning, 5.2.0 should allow breaking changes if sufficiently noted in Laravel's upgrade guide. Also...

RE: package development and keeping up with breaking changes... This is a perfect time to introduce breaking changes, 5.2 being the first major release after LTS. Give package devs time to adapt before the next LTS release. Keep the framework progressing.

[ghprod]

:+1:

[SimoTod]

+1 returning Collection.

[JosephSilber]

@hannesvdvreken generators _might_ make sense in the future for Eloquent collections. For the query builder, generators won't help you at all. The collection just wraps the simple array we get from PDO.

The only way to save memory is to do multiple database queries. That's what chunk is for.

If you're talking about not using fetchAll at the PDO level, that's a different thing. 99.9999% of the time you _do_ want to use fetchAll. If we ever decide that we want to add the other functionality, we can add an iterate method on the query builder. Or something like that.

Returning a collection now would not affect that at all.

[GrahamCampbell]

Well, anything we can do with generators, we can already do with iterators, all be it, with a bit of pain.

[rockymontana]

+1 for consistently
This kind of inconsistency actually adds up on dev time and it's easier to remember what the last method call gives you than having different return types on calls that basically does (even if it diff in theory) the same thing.

[ryross]

I don't think I've ever wrapped db results in a Collection. I don't see much reason to change it and cause work for a bunch of folks. If you do use it, i could see how it would be nice to have, but there's a precedence here. On the other hand, consistency is very nice.

[daftspunk]

Returning an array is consistent and should be the norm. Returning a collection should occur only in special cases. The expectation is an array not a collection. Perhaps we should comb over every other method to ensure they never return an array, only collections.

This very proposition is deeply disappointing, it is a systematic dismantling of a beautiful framework and its charm. Simplicity and efficiency are lambs to the slaughter under the banner of consistency. This is design by committee at its worst.

The lists method has already been butchered, returning a Collection object for a simple key-value pair. The conversation is already over, there is nothing to discuss, this madness needs to be implemented for consistency's sake.

[rnd-forests]

:+1: That would be great!

[clevonnoel]

:+1: Simply for the consistency. This will be awesome

[GrahamCampbell]

Hmm, I kind of agree with @daftspunk in that we're already overusing collections.

[ShawnMcCool]

I can see it both ways. But, I think that focusing on arrays is a bit of PHP-specific madness that has more downsides than benefits. I don't have a strong opinion on what the "default" behavior should be.

But, I'll be using collection objects because I focus on higher-order functions like map, filter, and fold.

[rizqidjamaluddin]

I'll agree that I have some annoyances with collections being used to represent all sorts of different things (ordered sets, hashes, sets) and being used all over the place to some confusion. But at this point I'm just seeing it for consistency with few drawbacks, and is already widely used and accepted within the community (though most people don't even know they're working with collections when doing things like $model->related->map()). And PHP's anemic arrays don't provide any expandability so there's not many choices from here.

[adrianthedev]

+1 to ->collect() or `->asCollection()1 and return an array as default for backwards compatibility.

Also a setting in database.php config could help set the default 'db-facade-return-type' => 'array|collection'

I believe that performance is more important.

[ShawnMcCool]

Keep in mind that the performance hit is the instantiation of a single small object.

It's really important that a method returns a reliable expected type. Configuration of this is opening up a whole world of pain.

[franzliedke]

I agree that configuring this is a bad idea: calling code (e.g. in packages) will have to make assumptions or complex checks. That's a no-go.

I wouldn't underestimate the pain of refactoring this, either. Having lots of query builder code is not some form of "bad code", it can be quite common.

That's why I vote for a new method with the different return type, too. ->collect() is the nicest, but may cause some confusion because it has another meaning in the functional world. But hey, @taylorotwell is the naming expert ;)

[rizqidjamaluddin]

The main problem with collect is it introduces _more_ inconsistencies:

| Method | Query Builder | Eloquent |
| --- | --- | --- |
| get() | Array | Collection |
| collect() | Collection | Collection? |
| lists() | Array? | Collection? |

[ShawnMcCool]

It's also reasonable to just use the collect() function as already is done.

[GrahamCampbell]

Configuration of this is opening up a whole world of pain.

No way we should allow that, lol.

[jesseleite]

I agree @ShawnMcCool. Config option will cause pain, and collect(), etc. is unnecessary if Collection is default output. With a Collection, we can easily convert output to array or JSON via the toArray() and toJson() methods respectively. I still vote for consistency between Eloquent get() and Query Builder get() methods.

[GrahamCampbell]

I think the general opinion here is "Collections are good."

[GrahamCampbell]

Feel free to continue discussing though if you want. :)

[ShawnMcCool]

I like wearing a hat. However, I feel like I can only get away with it in the winter; only a cap at that.

[daftspunk]

The collect() function is the poor [wo]man's cast. Ideally PHP will introduce the ability to cast to custom objects in the future:

(Collection) $query->get();

Until that time those who want collections should need to cast as such using the collect() helper. Bending the framework to overcome this awkward limitation is just absurd. A Collection [custom] object should be returned when the array [also] contains custom objects, like an Eloquent\Model, otherwise, if the array contains generic types (stdClass, string) then it should be preserved as an array. This makes sense in terms of like-for-likeness and is my definition of consistent. As much as this goes against the tide, my vote would be to fall back to Taylor's original vision and revert the lists method to returning an array.

[crynobone]

calling code (e.g. in packages) will have to make assumptions or complex checks. That's a no-go.

Calling code already need to make complex checks, regardless what happen here. However for those waiting to go with agnostic packages wouldn't want to include illuminate/support or illuminate/contracts to check for Collection or Arrayable.

I think the general opinion here is "Collections are good."

Yes, let dismiss that many prefer separate method to avoid breaking BC, because you know 5.1 is LTS so we can do many breaking change for 5.2. And Laravel will be so different by the time 5.6 is available (when 5.1 LTS end) that nobody can actually upgrade if they don't do it now.

[ShawnMcCool]

True that, regarding breaking changes.

[JosephSilber]

Just fielded another one of these on Stackoverflow. I see this all. the. time.

http://stackoverflow.com/questions/33265427/filtering-laravel-resultset-collections-fails

[CurosMJ]

What about having something like DB::table('users')->toCollection() ?

[GrahamCampbell]

@CurosMJ We already have that though?

[GrahamCampbell]

Just dump the stuff in the collect function.

[CurosMJ]

@GrahamCampbell toCollection() feels syntactically sweet than using collect().

[GrahamCampbell]

I think it feels wrong for it to know how to decorate itself.

[JosephSilber]

The idea here is about unifying the API. Any talk of adding toCollection() or collect() methods to the query builder are off topic. Of course we could always add those if we don't return a collection by default, but that doesn't really help much with the inconsistency between Eloquent and the query builder.

So please people, stop suggesting additional methods to the query builder. That's definitely an option if we don't return a collection by default.

Let's stick to the topic at hand: is consistency and elegance worth the breaking change or not?

I think this thread has run its course. The vast majority of people want this change, while there's definitely a minority that is against it.

This is now in @taylorotwell's court.

[GrahamCampbell]

Yeh.

[franzliedke]

Let's stick to the topic at hand: is consistency and elegance worth the breaking change or not?

If that's the only question, then I'll vote no, for the reasons outlined further above.

These reasons probably also affect your "vast majority". The people willing to jump in on this discussion or following Taylor on Twitter are surely only a small fraction of those actually using the framework (and also quite likely willing to refactor a bit more during upgrades, especially if they follow development so closely as to hang around here). And I'm pretty sure they like painless upgrades that don't introduce sneaky BC breaks like this. Just warning you guys not to underestimate how many people this will affect, and probably in a lot of lines of code.

[brunogaspar]

Yap, i'm with @franzliedke

[jesseleite]

@franzliedke, It's not a "sneaky BC" if it's added in major Laravel version, and noted in upgrade guide. 5.1 had quite the upgrade path, but we all lived. It's looking like 5.2 will be a subtler/easier upgrade. I disagree with "sneaky BC".

That said, a simple search for ->get( would make this an easy upgrade. The price of forcing users to upgrade is far less than allowing users to be continually confused by inconsistent get() output between Query Builder and Eloquent.

[GrahamCampbell]

5.2 is meant to have some resistance to update though. Like, filters and other deprecated stuff is gone, and we're encouraging upgrading to symfony 3. That's one reason 5.1 is LTS.

I'm kind of gutted we allowed symfony 2.8 as well as 3.0, but that's another story.

[franzliedke]

@JesseLeite I guess what I meant to say is that refactoring this kind of change is a much larger pain than simple interface changes. That's because this type of code may be found all over Laravel projects...

[GrahamCampbell]

There's no doubt that this will be a pain, but I think it's worth it. People don't have to upgrade any time soon if they don't want to anyway.

[franzliedke]

Hmm, yeah, 5.1 being LTS is a good argument. Anyway, I've said enough, we'll see what Taylor makes out of this.

[drsii]

Let's stick to the topic at hand: is consistency and elegance worth the breaking change or not?

I would like to point out that this highlights a greater issue with Laravel's versioning and release process.

I think i'm the largest vendor of Laravel packages. Thats just to say We've some experience how these "minor" changes have effected our downstream users over the years.

Don't get me wrong, I'm all for progress, I like the idea presented here. But just want to point out a few things to think about.

• Don't underestimate the amount of work you're putting on application developers with this change.
• LTS versions are great but realize with each breaking change the less likely that package developers can keep pace.
• repeat after me: you are a wonderful and intelligent developer. Head up, shoulders back, you look great today.

[GrahamCampbell]

Laravel does not have any issues with versioning. People just incorrectly assume it's SemVer.

If people use ~5.1 in a version constraint in a package, they are doing it wrong. That's like saying you want 2.*|3.*|4.*|5.* in Symfony. It clearly makes no sense.

[GrahamCampbell]

@drsii I do agree with you though, actually, on most of what you said, and maybe we need to make our versioning super duper clear. I think one reason people often set the wrong version constraint is that they actually don't understand what ~ does in composer because they didn't read the docs.

[drsii]

@GrahamCampbell I think establishing a clear versioning process would be a wonderful step forward.To clarify my position you have somewhat of a perfect storm here. Most of this should probably be addressed in a separate ticket. A few more points to consider.

• Laravel has allowed the average developer to become involved in the PHP language.
• The PHP community has embraced composer, which is premised on semantic versioning.
• Laravel relies on composer, but the versioning does not follow major.minor conventions which the vast majority of the developer community has adopted.
• @taylorotwell times releases with symfony. which is very clear about it's release process. http://symfony.com/doc/current/contributing/community/releases.html

Bottom line. Wether we're seasoned engineers, budding developers, or enthusiastic entrepreneurs when it comes to breaking changes consistency and elegance will never trump the time and money it will inflict. Anything you can do to lesson the pain makes progress much more appealing.

[CurosMJ]

@drsii :+1:

[jesseleite]

RE: breaking changes [...] consistency and elegance will never trump the time and money it will inflict

@drsii I see where you are coming from. However, Laravel's consistent/elegant syntax, behaviour, API, etc. is a big part of why many use Laravel. Developer happiness seems to be a mantra. This is why we have versioning systems that allows both minor (non-breaking) changes, and major (breaking) changes. I consider @JosephSilber's proposition of consistency and elegance a good thing, and of course it should be done on a major release with proper upgrade notes. 5.2 is Laravel's next major release (see @GrahamCampbell's comments on Laravel not using semantic versioning). If a dev isn't ready to pour time/money into an upgrade, there's nothing wrong with using 5.1 LTS after the release of 5.2+. However, if a dev wants to take advantage of a new major version, an upgrade path must be followed.

We shouldn't be scared of major release progression. IMO, 5.2 is the perfect time to introduce breaking changes (since it's the first major release after 5.1 LTS).

[rockymontana]

Totally agree with Jesse.
Spot on if you ask me.

On 25 Oct 2015, at 02:45, Jesse Leite [email protected] wrote:

RE: breaking changes [...] consistency and elegance will never trump the time and money it will inflict

@drsii I see where you are coming from. However, Laravel's consistent/elegant syntax, behaviour, API, etc. is a main draw for me, and a big part of why many use Laravel. Developer happiness seems to be a mantra. This is why we have a versioning system that allows both minor (non-breaking) changes, and major (breaking) changes. I consider @JosephSilber's proposition of consistency and elegance a good thing, and of course it should be done on a major release with proper upgrade notes. 5.2 is Laravel's next major release (see @GrahamCampbell's comments on Laravel not using semantic versioning). If a dev isn't ready to pour time/money into an upgrade, there's nothing wrong with using 5.1 LTS after the release of 5.2+. However, if a dev wants to take advantage of a new major version, an upgrade path must be followed. We shouldn't be scared of major release progression.

—
Reply to this email directly or view it on GitHub.

[crynobone]

What's the point of LTS if every minor release (4.x, 5.x) introduce major breaking changes?

[drsii]

@JesseLeite Yep, like I said, i'm all for progress and I like this idea. and +1 it. The issue I raise isn't about progress, its that Laravel is, in a way, setting everyone up to fail at progressing.

Laravel: Major.Major.Minor/Patch
Majority: Major.Minor.Patch

Laravel isn't clear about this as far as I know, which is all I am saying and @GrahamCampbell seems to understand.

[jesseleite]

@crynobone, As @GrahamCampbell and @drsii have stated, 5.1 -> 5.2 is a major release for Laravel. Laravel does not follow semver.

[taylorotwell]

Some more thoughts: could we introduce a collect method onto the query builder in 5.2:

$results = DB::table('users')->collect();

This essentially would be an alternative to get which would simply return a Collection instead of an array. We could then perhaps introduce a notice in the documentation that get will eventually return a collection in the next major version of Laravel (6.0). Of course, 6.0 has no definite release date so I don't know when that will be. I have not even started working on a next major release since the 5.x release is pretty solid.

Alternatively we could say that get will start returning Collections in 5.3, which would be May 2016, giving package developers and applications 6 months to prepare for the change?

[ryanhungate]

100% in support of the ->collect() method.

[rizqidjamaluddin]

Again, the issue with collect() is that in just confuses the inconsistencies further:

| Method | Query Builder | Eloquent |
| --- | --- | --- |
| get() | Array | Collection |
| collect() | Collection | Collection? |
| lists() | Array? | Collection? |

I would say that's even worse than having nothing at all for now.

[GrahamCampbell]

I think we should just make the break in 5.2 given 5.1 is LTS.

[taylorotwell]

@rizqidjamaluddin I disagree with that. As of right now, query builder always returns arrays, and Eloquent always returns Collections. Introducing the collect method into query builder makes it obvious that it's a special method to help you get a collection from the query builder which normally doesn't return them. That's why it's specifically called collect.

[rizqidjamaluddin]

Sure, if you want to define it that way - but again, that's the whole inconsistency thing we were tackling in the first place; adding collect() just seems to reinforce the inconsistency rather than try to resolve it. (Which is, of course, an option in itself. Just noting that it's not just a stopgap.)

I'd say it's just adding another layer of confusion between them, and anyone who just wants a collection out of the query builder can do so perfectly fine, as this is just a third syntax they could use. I was under the impression that the discussion from the get-go wasn't primarily about allowing collections to return from the query builder - as that's already perfectly possible - but about eliminating the inconsistency.

Though, of course, the new syntax is a side-benefit, so if that outweighs the it-not-solving-the-inconsistency problem, sure, it doesn't do any harm besides the aforementioned reinforcing thing.

[taylorotwell]

Any discussion of "incosistency" is a little silly though. They will _still_ be inconsistent in that Eloquent will return Eloquent collections and query builder will return plain collections. The supposed current "inconsistency" is not an accident or something. Therefore I don't think we should view this as "reconciling inconsistencies" but rather just determining the behavior we want from query builder. It was _intentionally_ originally designed to only mess with plain arrays because it's meant to just be a simple query builder.

[rizqidjamaluddin]

By "inconsistency" I meant the point brought up in the first post:

Makes it consistent with Eloquent. You'll find countless questions on StackOverflow asking why some collection method can't be called on a query builder result.

_If_ the current status quo is defined as consistent, then point 1 is by definition void (as in "intentional"/"not a bug"), and seeing as we're left with 2 and 3, then yes, I'd agree that ->collect() would solve those problems.

I was just assuming that the status quo _isn't_ defined as consistent, though, since if it were, the whole thread would have been solved minutes after it was opened. Point 1 is invalid as everything is OK, point 2 and 3 stands, add ->collect() to resolve that. I could live with it.

To be clear, I think both of the following are "consistent", just in different ways:

| Method | Query Builder | Eloquent |
| --- | --- | --- |
| get() | Array | Collection |
| lists() | Array | Collection |
| collect() | Collection | n/a |

| Method | Query Builder | Eloquent |
| --- | --- | --- |
| get() | Collection | Collection |
| lists() | Collection | Collection |

Both sides of the discussion are right in their own way. So at this point it's just come to which one we want to go with. Based on the last comment, though, I take it it's the former.

... Unless you're suggesting we stick to the former in 5.2, then softly switch to the latter in 6.0?

[abrahamgreyson]

In this morning I still agree with make BC break in 5.2. now I think a gentle upgrade will more user friendly. Such as jQuery's live() method, deprecated in 1.7 and removed in1.9, gave user some time to prepare. get() return collection must be done, only question is how to do it elegantly.

[GrahamCampbell]

Users already have 2 years to prepair in 5.2. If we did it in 5.3, then they only have 6 months to prepair, or they have to downgrade to 5.1 again.

[JosephSilber]

I agree with @grahamcampbell. The best time for this change is now, right after the LTS release.

And like @rizqidjamaluddin said, adding a collect helper wouldn't help with the inconsistency.

It'll only make it worse.

[taylorotwell]

Let's not mention the word "inconsistency" again in this thread. The only
question is: do we want to force all applications to upgrade this for 5.2
or not?

On Mon, Oct 26, 2015 at 11:44 AM, Joseph Silber [email protected]
wrote:

I agree with @grahamcampbell. The best time for this change is now, right
after the LTS release.

And like @rizqi said, adding a collect helper wouldn't help with the
inconsistency.

—
Reply to this email directly or view it on GitHub
https://github.com/laravel/framework/issues/10478#issuecomment-151204677
.

[ryanhungate]

I've said my part before... but the answer is no way. It's not worth it. To be honest, I don't even see the merit for arguing on something like this at all... next major release, sure, not a problem. Let's all try not to forget that most of, if not all of us that use Laravel are operating a real business and need stability...

[franzliedke]

Users already have 2 years to prepair in 5.2. If we did it in 5.3, then they only have 6 months to prepair, or they have to downgrade to 5.1 again.

I don't think that makes anything better; since 5.2 through 5.5 will be _major_ releases; BC-breaking changes will occur on any of these, making upgrades from 5.2 to 5.6 a possibly huge undertaking anyway. Knowing this in advance won't help much.

The deprecation idea is noteworthy; but since we already have consistency (hehe), it might not be needed.

I :+1: ->collect().

[crynobone]

Users already have 2 years to prepare in 5.2.

Why would you think LTS user even want to upgrade to an unmaintained version. LTS user would prefer to upgrade to another LTS version, which should be Laravel 5.5.

Will the collaborator plan to provide a detail LTS to LTS upgrade note? Or you going to say "It's up to community to PR this upgrade note, we not going to do it...".

[rizqidjamaluddin]

To be honest, I don't even see the merit for arguing on something like this at all... next major release, sure, not a problem.

Aren't 5.x releases being counted as a "major release"? Laravel doesn't follow semver, after all.

I do have some worries about LTS-to-LTS upgrade procedures and support, but that's a discussion for another day...

[crynobone]

I do have some worries about LTS-to-LTS upgrade procedures and support, but that's a discussion for another day...

Why wait, changes are being made and 5.2 going to be release within/roughly a month. 5.3, 5.4 and 5.5 also going to be a "major" release, by the time the LTS users start to notice/worry about all this changes who going to be handling it? Pretty sure many of us here going to be upgrading to the latest version as/when it is available, so we wouldn't care.

[JosephSilber]

Preventing the framework from progressing forward because of a fear about LTS users' upgrade path is quite harmful.

I fully agree that it is onto the framework maintainers to provide LTS users with an upgrade path, fully documented every step of the way.

But allowing any of that to affect the framework's progress would mean that the LTS release has done us more harm than good.

[rizqidjamaluddin]

Why wait, changes are being made and 5.2 going to be release within/roughly a month.

I just meant that I think it deserves its own topic and discussion, as this issue is about a very specific change in mind. It's pretty unrelated to this, as well, more of being a related concern than should be a causal effect.

[jesseleite]

Preventing the framework from progressing forward because of a fear about LTS users' upgrade path is quite harmful.

I agree @JosephSilber. LTS releases may be a necessary evil to sell a framework to business superiors, but they sure knot the community's knickers. Why are we so scared of breaking changes in a major release? If I had an app that I wasn't ready to pour time/money into upgrading, I'd happily leave it on 5.1.X LTS for a few years. However, if I can spare the time/money to follow a well documented upgrade path, I want the latest greatest version of Laravel. Don't let LTS hold the framework back from reaching it's full potential.

The only question is: do we want to force all applications to upgrade this for 5.2 or not?

In my opinion, yes and why not 5.2? Look at Laravel's upgrade guides. 4.1 to 4.2 forced new PHP version. 5.0 to 5.1 changed a bunch of Collection method behaviours. Breaking change is not new to Laravel. 5.1 LTS will be supported for few years yet, so people who aren't willing to spend time/money to follow the upgrade path can stick with 5.1 LTS. That's what the LTS release is there for :P

[franzliedke]

Why are we so scared of breaking changes in a major release?

Because we have them so often, and that's a pain. It's not like progress cannot be made without breaking BC. Just look at Symfony and Ember, for example. ;)

[GrahamCampbell]

I'd argue that actually symfony has had a very hard time dealing with BC.

[InstanceOfMichael]

@drsii and @daftspunk and @ryanhungate are right, this is a terrible, hotheaded, poorly thought out idea. It's just that useless ->lists() to Collection change all over again.

You still will need to find during 5.1 => 5.2 upgrade every DB::table() call and append ->all() to all those queries.

and anything like Eloquent::query()->getQuery()->get()

Developer happiness seems to be a mantra.

I think adding ->collect() to both Eloquent and DB would be best. ->get() is already vague on it's own, ->collect() would syntactically match ->paginate() pretty well. Otherwise, it's more like you actually hate developer happiness.

I completely understand @drsii 's position, where he sees $20K+ worth of labor drowning itself while less experienced and trendier developers are flocking over to some really useless BC-breakage fawning "Oh it's so pretty!"

Ultimately, no matter how much prettier your code is going to be, this change is _really expensive_ and _really useless_.

[franzliedke]

That's a nice idea, @InstanceOfMichael.

Wouldn't adding ->collect() to both query builders give us the "consistency" some here would like? We could then just go on to recommend that syntax in the future, but don't have to break BC...

[dwightwatson]

We could, but PHP arrays suck. That's why returning a collection is better for developers.

I don't get the argument against upgrade time. The find/replace for the lists() change last time can be done in less than 15 minutes, even on a large project. This is just the same.

[InstanceOfMichael]

The find/replace for the lists() change last time can be done in less than 15 minutes, even on a large project.

That's great that your "large" project has less than 15 minutes of "find/replace" for ->lists(), including your entire ecosystem of 4.x-5x libraries and dependencies. I'm glad that ->lists() only cost you $30.

[GrahamCampbell]

To be fair, there is plenty of other stuff that will be more of a ball ache than this if you're upgrading from 4.x -> 5.2, like filters are actually removed now.

[InstanceOfMichael]

To be fair, there is plenty of other stuff that will be more of a ball ache than this if you're upgrading from 4.x -> 5.2, like filters are actually removed now.

Sure, that's true, but middleware wasn't a useless change.

[crynobone]

To be fair, there is plenty of other stuff that will be more of a ball ache than this if you're upgrading from 4.x -> 5.2, like filters are actually removed now.

To be fair, you have at least 2 "major" version to prepare for that (5.0, 5.1) before filters is completely remove. And middleware wasn't perfect (replacement for filter) in 5.0, until parameter support was added in 5.1.

[taylorotwell]

Discussion question: will there _ever_ be a point in the future that is a better time to start returning Collections from query builder? If so, when is that time?

[taylorotwell]

No thoughts?

[jesseleite]

@taylorotwell Already offered my thoughts, but IMO there will not be a better time. I think 5.2 being the first major release after 5.1 LTS is even better because it gives package dev plenty of time to adapt between now and next LTS release.

[franzliedke]

Technically, the work it takes to refactor this should be equally painful whenever it is required.

On the other hand, when officially "deprecating" a certain use of that method, that will give people more time, so they can refactor without too much pressure.

It's still pain that we do not have to inflict at all. ;)

[InstanceOfMichael]

It's still pain that we do not have to inflict at all. ;)

The change is also fundamentally useless. It also says to the Laravel community and onlookers that Laravel is a framework for small create-and-forget projects.

[ryanhungate]

@taylorotwell - I would say that returning a collection by calling ->get() does not make sense. I have actually always felt that way about the eloquent collections as well...

I wonder if we changed the approach here and said "How many people would object to returning an array of Eloquent models while calling ->get() instead of the query builder" and then force a ->collect() method on both types?

EDIT:
I meant that you would get an array on both Eloquent models AND the query builder paths.
I meant that to get a collection on either one of these scenarios that you would have to call a collect() method which actually returns the collection... basically wrapping the collect(get()) at once for simplicity.