@jasonmccreary Great stuff. My only reservation is whether an 'always-on' Concern trait like this might be needlessly filling up the global-scopes namespace with global scope names that get applied to every model.

What if this was an opt-in trait, like SoftDeletes?

@jonnott, sure. could go that way. The DX of simply setting a property better keeps with the underlying goal - less code. Once you start adding traits, setting properties, defining methods it quickly becomes the same amount of code for the user to write.

In fairness, nothing is "needlessly filled". I took care to not add anything if the model does not set the $orderBy property. So either solution would apply the same amount of scopes/macros.

...i.e.

namespace Illuminate\Database\Eloquent;

use DefaultOrderBy;

class User extends Model {

protected $orderBy = [ .. ];

}

In fairness, nothing is "needlessly filled". I took care to not add anything if the model does not set the $orderBy property. So either solution would apply the same amount of scopes/macros.

Oh yes, if (empty($this->orderBy)) { return; } is a nice touch.

@jonnott, sure. could go that way. The DX of simply setting a property better keeps with the underlying goal - less code. Once you start adding traits, setting properties, defining methods it quickly becomes the same amount of code for the user to write.

I think because none of the other Concerns/Has* traits define named global scopes, it feels a bit like by doing so this trait is infringing on something which feels like the 'domain' of the app's developer - i.e. the global-scope-names-namespace. This is in contrast to say the SoftDeletes trait, which uses a global scope class instead. What if the 'defaultOrderBy' scope was defined in a class? Am I being too pedantic?

..and where would it live .. Database/Eloquent/Concerns/Scopes/DefaultOrderBy.php ?

@jonnott, all fair points. Let's see what the Laravel team has to say. Glad to make tweaks as I feel this is a worthy addition. But I'd like to make sure they think so too first. 😅

@jonnott, all fair points. Let's see what the Laravel team has to say. Glad to make tweaks as I feel this is a worthy addition. But I'd like to make sure they think so too first. 😅

Fully agree about the worthiness of the addition. I'd use it loads!

But also wanting to be wary of the possibility of:

• Models having too many Concerns by default - is that a slippery slope?
• potential developer pain if trying name their own global scopes something which the default Eloquent Model has already 'gazumped' ;) .. if the core framework utilises a global scope name that a developer may have already used in their code .. is that a breaking change?!

Getting towards an almost philosophical level here, I'm aware 🤓 Looks like one for @taylorotwell et al to review the exact implementation detail for maybe..

.. is that a breaking change?!

@jonnott, indeed, that's why it's targeted for Laravel 9. 👍

@jonnott, indeed, that's why it's targeted for Laravel 9. 👍

I'd be interested to know if the core framework actually uses any named global scopes itself, or if the implementation of all framework-level global scoping is done through classes & FQCNs. I'd not be surprised if this is the case.

@jasonmccreary I branched and added a commit to put the global scope into a class instead .. see what you think @ 9.x...jonnott:model-orderby-property

@jasonmccreary, yeah, more closely follows the SoftDeletes. The only reason I didn't do this was the scope is now aware of the property. That felt strange to me. But weighed against the "magic string", it is better.

I also thought about making the scope class generic, so it could be used directly. But then realized there isn't really a strong use case for that. Again, the streamlined DX is ultimately what makes this worthwhile.

One concern I might have with this being implemented as a global scope is there are certain times in (maybe in the framework, maybe in packages, etc.) where we explicitly retrieve models with all global scopes disabled in order to be sure we can definitely retrieve them without any soft delete constraints getting in the way, etc. In those cases, this ordering would be removed - which I find... odd... because you may want the sorting to still be in affect even though you don't want any other global scopes in play.

I think the reason it starts to feel odd is this is not a "scope" at all, where I typically think of "scope" to mean something that "constrains" a query's results into some smaller result set via some where clause condition. This is a pure sorting operation.

@taylorotwell, agree. This was a bit of a proof of concept. If you feel this is a worthwhile addition, an implementation directly within Eloquent or the Builder may avoid the oddities you mentioned.

I think there is something worthwhile here in general if we nail out how it is implemented. I've definitely had situations where models are very chronological (think blog posts, comments, etc.) and I almost always want them in a given order.

@taylorotwell, cool. I also marked this was WIP. Let me see if I can ingrain this a bit more within Eloquent/Builder so it has less reaching implications you and @jonnott have noted.

@taylorotwell, cool. I also marked this was WIP. Let me see if I can ingrain this a bit more within Eloquent/Builder so it has less reaching implications you and @jonnott have noted.

I think maybe this functionality could just be worked right into where the query builder applies other normal ordering from ->orderBy()s to the query. Without having looked at that code in detail, not entirely sure how that would happen or if it's trivial to do. I'd imagine that's more like what @taylorotwell is getting at though.

The more I think about it, the more what Taylor's saying makes sense. Using global scopes to do things like this doesn't feel quite right, unless they're opt-in traits like SoftDeletes.

@taylorotwell, this has been updated to push everything down to the Eloquent Builder. The sorting from this property is applied before the query is executed.

However, given the infinite ways developers may write Eloquent queries and the interaction with the underlying Database Query Builder, I am not sure if this is applied everywhere it should be.

As noted in the updated PR description, I only apply this sorting if no other ordering has been set for the query. I believe this helps avoid situations where queries with explicit calls to orderBy sort unexpectedly because it's being merged with the sorting from this new property.

Put simply, this property allows the user to set a default sorting when not otherwise specified.

@jasonmccreary I still think calling this whole thing 'defaultOrderBy' rather than 'orderBy' might be less confusing for developers..

If it's just called 'orderBy' then people might think that's how you always define ordering for queries, rather than the $query->orderBy() method?

Remember that draft PR's aren't reviewed. Please mark as ready if you want a new review.

@driesvints, sorry, I had sent this to Taylor as I worried it might need more work. I have marked it for review so we can make those tweaks and hopefully get it into L9. 🤞

@driesvints, sorry, I had sent this to Taylor as I worried it might need more work. I have marked it for review so we can make those tweaks and hopefully get it into L9. 🤞

@jasonmccreary @taylorotwell Still I think the main issue with the re-done implementation is that there really needs to be a way to remove the default ordering for those edge-cases where you really need the resulting query (SQL) to have no ORDER BY clause at all (i.e. natural table order) even though you've set this property on the model. Examples might be within tests, for instance.

@jonnott, you should be able to call reorder (with no arguments) just as you would currently to remove all existing order by clauses.

If not, I can add some tests cases to ensure that works.

@jonnott, you should be able to call reorder just as you would currently.

Ah, ok. Looking at your code, I couldn't make out how calling reorder() would remove the default ordering too, as it only clears what's in the ->orders property on the builder, rather than removing the ->orderBy property of the model, which gets handled later in the query building process. I know you had the withoutDefaultOrder() method to do this, but then removed it.

Can you clarify if/how the default ordering would also get removed by ->reorder() ?

@jonnott, I'm not sure it does, tbh.

Thinking about it more, I don't see removing all orderBy including the default as a common use case if you were defaulting the order for a model. At least not without explicitly setting the desired order - which does overwrite the default.

That could be a breaking change. Which is why this is targeted for 9.0. But even so, you have to opt-in.

In your case, I would change your test cases to have an explicit order. Not try to remove all ordering.

@jonnott, I'm not sure it does, tbh.

I think what I'm really getting at here is consistency with how the rest of the query builder and Eloquent work.

Essentially, anything else that you add into a query, even things added to a model 'as default' (like global scopes) can be removed .. e.g. ORDER BYs with reorder(), global scopes with withoutGlobalScope(), etc .. therefore whatever things you've configured your model with, you can always, by calling the required methods, get back to a completely 'plain' SELECT query, i.e. SELECT * FROM {table}. Period. *** Nothing takes away your ability to get back to that basic query.

..apart from this new orderBy property. How it is now in this PR, once you've defined that as a property of your model class, there no way to get back to that pure SELECT * FROM {table} anymore. Whatever your reason for needing to do so is.. And someone, somewhere, will for sure arrive at a situation where they need to do that. That's why I feel not having that 'way out' isn't very Laravel-y, or consistent within Builder/Eloquent.

*** Correct me if I'm wrong on that one!

Really not trying to just pick holes here, genuinely want this to be a killer Eloquent feature.

@jonnott, at this point, it's pretty unlikely this will make it into the L9 release being 3 days away. Especially with all our back and forth.

I appreciate the feedback. It's likely there's more work to do here. But I'd really like to let the Laravel Team weigh in first to help guide us in the right direction.

If you have your own ideas of how this should work, I encourage you to use what I have here to open your own PR. 👍

If you have your own ideas of how this should work, I encourage you to use what I have here to open your own PR. 👍

I think what you've (re)-done is great. My suggested changes would be really simple, and essentially the same as what you already had in place before, but removed..

• I'd name the model property $defaultOrder so it doesn't get confused with the builder ->orderBy() method, and call the applying method in Eloquent/Builder applyDefaultOrder() to match.

• Add a protected boolean $withoutDefaultOrder = false property to Eloquent/Builder, and a method withoutDefaultOrder() to Eloquent/Builder which sets $withoutDefaultOrder to true, and return early from applyDefaultOrder() if either empty($this->orders) or $this->withoutDefaultOrder are true.

@jasonmccreary I think that what you've done so far, combined with my suggestions/changes, ..

• fully round this out as a neat & complete much-needed new feature
• provides clear, unambiguous naming & clarity for devs as to what it does
• make it a no-brainer to merge
• causes zero BC breaks
• could totally be targeted at both Laravel 8.x and 9.x without any issue

@jasonmccreary Seems like $defaultOrderBy / applyDefaultOrderBy() / $withoutDefaultOrderBy / withoutDefaultOrderBy() work equally well regards naming, and fits with how you've named the tests.

@jasonmccreary I've created a new commit with my suggested changes, which you could merge if you'd like .. jonnott@93e3ad3

I went for $useDefaultOrderBy as the Eloquent/Builder boolean property name in the end, so it doesn't have the same name as either of the new methods added to Eloquent/Builder (applyDefaultOrderBy() and withoutDefaultOrderBy()), which could make the naming confusing.

I guess the only thing needed further to this might be an additional test for withoutDefaultOrderBy() perhaps?

..and here's that test jonnott@c59f20c (same branch again on my fork)

@jasonmccreary Any thoughts?

@jonnott, nope. Waiting to see what the Laravel Team thinks.

Wait for Taylor to confirm this but I think it's best that we retarget to 10.x since we're releasing today. It's a bit tricky to still merge this one in.

Wait for Taylor to confirm this but I think it's best that we retarget to 10.x since we're releasing today. It's a bit tricky to still merge this one in.

100% agree this still isn't quite ready for today's 9.0 release. Even this morning it's needed further fixes and tests adding, thanks to @crynobone's helpful observation.

@driesvints, no worries. Things got a bit noisy here and I didn't really know which final tweaks this needed. This does work, but agree there are a lot of boundary cases to consider.

I'll close this for now. I'm hopeful we won't need to wait a year until 10.x to get this added. As a new property it would be opt-in. And to the point above, it shouldn't have any breaking changes once the right tweaks are made.