Deduplicate order by while preserving semantics

[spawnia]

• Laravel Version: 8.x
• PHP Version: All versions
• Database Driver & Version: SQLServer vs. MySQL have different behaviour

Description:

Follow up to #37505 and #37647.

When applying multiple order by clauses on a builder with identical column names, Laravel currently includes multiple such clauses in the generated SQL. Clauses added by calling orderBy() on the Builder have precedence over those added by global scopes (i guess it applies to local scopes too?).

While MySQL just uses the last defined clause, SQLServer fails with an error complaining about the duplicates. Thus, only one clause should be present in the generated SQL. Semantically, it makes sense to me that explicit orderBy() calls override order by clauses set in global scopes. Similarly, later calls to orderBy() should override earlier calls.

Steps To Reproduce:

See test cases of #37505 and #37647. I think both are equally valid and worth fixing. The previously attempted solution did not manage to fix both.

[driesvints]

Welcoming PR's.

[Javdu10]

Hello @spawnia, is there any chance that you could try my branch patch-1 ?

[derekmd]

Call reorder()? https://laravel.com/docs/8.x/queries#removing-existing-orderings

[Javdu10]

Call reorder()? https://laravel.com/docs/8.x/queries#removing-existing-orderings

I wish this was the fix @derekmd,but unfortunately scopes are called at the end of the construction of the query. Thus overwrites reorders

This issue got ignored in the past so there are no regression test written, they have to be discovered from running applications, unless this issue is tackle by someone really knowledgeable of the framework, I can only offer to break things and move fast.

[spawnia]

Unfortunately, the fact that scopes are being applied last makes it hard to achieve the semantics I would expect. My mental model of global scopes is that they are supposed to be the baseline for queries of the model. Subsequent manipulation of the query builder should have precedence over them.

Mechanically, I get why Laravel does it. Adding the scopes from the beginning and applying them just before sending the query allows adding/removing scopes without doing much cleanup inbetween. Thus, I think we could go one of two ways:

1. Reify that scopes are applied last and accept that order by in scopes will have precedence. Write tests accordingly and direct the fix @Javdu10 proposed towards Laravel 9.x. At least, this will clarify the current semantics and fix a real issue for SQLServer users.
2. Change the mechanics of how global scope application and direct builder manipulation interact, such that global scopes apply first and direct builder manipulation has precedence. That would result in what I consider to be the most intuitive semantics, but requires non-trivial changes to a core part of Eloquent.

[driesvints]

@spawnia I agree with the above and would go for 1. 2 is unlikely to happen as it would indeed have massive implications across the ecosystem. I think it's best that we continue this discussion through PR's to both docs (to highlight the current 8.x behavior), the 8.x branch (tests that verify the current behavior) and master branch (to fix the issues for SQL Server users). If anyone can send those in so we can take the conversation from that'd be great. Thanks all.