ActiveQuery::one() limiting database records #58

[uaoleg]

Q	A
Is bugfix?	✔️
New feature?	❌
Breaks BC?	❌
Fixed issues	#58

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.76%. Comparing base (3c75ff1) to head (07ca678).

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #20266      +/-   ##
============================================
- Coverage     64.95%   63.76%   -1.19%     
  Complexity    11396    11396              
============================================
  Files           430      430              
  Lines         36925    36925              
============================================
- Hits          23984    23545     -439     
- Misses        12941    13380     +439     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[terabytesoftw]

The tests fail, what problem are we solving here?

[uaoleg]

@terabytesoftw failures are not related to the change. Solving the following issue: yiisoft/active-record#58

[rob006]

I don't think that adding implicit limit to one() was an agreed solution. There are some performance-related problems with this mentioned in #4348 (and yiisoft/active-record#58).

[uaoleg]

@rob006 is there any benchmarks proving LIMIT 1 can slowdown query?

[rob006]

@uaoleg I'm not sure if there are recent examples, but note mentioned in yiisoft/active-record#58 (comment) still exists in MySQL documentation, so this is not only about performance - such change could affect behavior in some cases.

[uaoleg]

@rob006 looks like the ORDER-LIMIT "feature" should not affect the first row =)
Also for me it's hard to imagine someone is using ->one() with sorting by something like category.
Actually I always was sure this method is a shorthand for ->limit(1)->all()[0] ?? null.
I believe it's worth to break some synthetic edge cases rather than have such inefficient query builder method.

[mtangoo]

I think this need to be closed. It solves the issue that wasn't the issue with referred ticket #58.
Any objection to closing this PR? @uaoleg @rob006 @terabytesoftw @samdark

[uaoleg]

@mtangoo I still believe it's rediculous to add ->limit(1) along with ->one(). Can someone imagine it could be ->limit(2)->one()?

[mtangoo]

@mtangoo I still believe it's rediculous to add ->limit(1) along with ->one(). Can someone imagine it could be ->limit(2)->one()?

You see, that is not the problem. That ticket was about something else, hence this is not the solution given the circumstance. If you want to discuss that, then you should open discussion under 2.2, since I doubt that behavior will change for 2.0.x

Since you were solving wrong issue, I think I can close this.
I will leave it for any objection before closing it!

[samdark]

I'd expect LIMIT 1 to be added if not all the edge cases mentioned by @rob006...

Also for me it's hard to imagine someone is using ->one() with sorting by something like category.

Well, replace "category" with "rating" and it will make perfect sense to select the top item based on rating.