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

BaseActiveRecord::find() has inconsistent return types #2999

Closed
hiltonjanfield opened this issue Apr 6, 2014 · 29 comments
Closed

BaseActiveRecord::find() has inconsistent return types #2999

hiltonjanfield opened this issue Apr 6, 2014 · 29 comments

Comments

@hiltonjanfield
Copy link
Contributor

@hiltonjanfield hiltonjanfield commented Apr 6, 2014

Any call to find() which includes an attribute, such as ->find(5) or ->find(['id' => 5]), returns a result (a single model). Calling ->find() (without attributes) returns an ActiveQuery object.

The issue is that the if blocks use ->one():

return $query->where($q)->one();

and

return $query->where([$primaryKey[0] => $q])->one();

otherwise the function returns the ActiveQuery object created at the start of the function body:

$query = static::createQuery();
...
return $query;

This creates inconsistent behaviour (using ->find() to get an object versus ->find() to start a query) and prevents extending your query or adding relations as such:

Faces::find(['type' => 'frown'])->with('eyes')->one();

Although ->where() exists to narrow down attributes, it is confusing to have both find() and where() doing the job.

Two ideas to fix:
[removed] - see updated versions 4 comments down.

@samdark samdark added this to the 2.0 Beta milestone Apr 6, 2014
@samdark
Copy link
Member

@samdark samdark commented Apr 6, 2014

@qiangxue it's popping up quite regularly. What do you think about solutions proposed?

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Apr 6, 2014

I think find should accept both pk and array, a lot of similar methods just confusing . Maybe some changes can be done to support User::find([status: 2, active: 1])->all() to be able to query multiple AR entities with same method signature .

@restyler
Copy link
Contributor

@restyler restyler commented Apr 6, 2014

+1 for mandatory one() call.

@hiltonjanfield
Copy link
Contributor Author

@hiltonjanfield hiltonjanfield commented Apr 6, 2014

I think my main complaint is that find() and where() have the same functionality, but using find() like where() has different results than other uses of find(). This can lead to confusion.

find() by itself acts like the start of a query builder (like select() or similar functions in other ORMs), but find(...) becomes an finalized query. To me, a function can accept varying parameters but should have the same RESULT. So, to reiterate and expand upon the suggestions above:

  1. "Mandatory one()" -- find() should return an ActiveQuery object no matter what so you can add relations or conditions. This makes a call to ->one() mandatory to get a single result, which lends to the consistency of the entire database system.
  2. "Eliminate multiple functionality" -- find() and where() share the same functionality. find() should do nothing but return an ActiveQuery object, making use of where() mandatory. find()->where('id' => 5)->one();
  3. "pk()->one()" -- where() can reproduce the functionality of find([attributes]) easily, but not as easily for find(5). To reproduce, add function pk() along with the changes in option 2.
    find()->pk(5)->one(); (maintains mandatory one() calls). Allows calls such as find()->pk(5)->with('comments')->one();
  4. "pk()" -- Same as item 3, but taking into account that a PK call can only ever return one record. find()->pk(5); This breaks the consistency of one(), all(),, etc being the only functions to execute a query, but does make some sense. Relations could be included with find()->with('comments')->pk(5);

I feel like option 3 makes things the most consistent. It will break existing code, but in a way that keeps calls consistent.
Example calls:

// replaces Faces::find(5);
Faces::find()->pk(5)->one();

// replaces Faces::find()->where(['must_know_your_pk' => 5])->with('eyes')->one();
// (find(5)->with() was not possible)
Faces::find()->pk(5)->with('eyes')->one();

// existing functionality already covers Faces::find(['type' => 'frown');
// also covers find([...])->with() call which was not possible)
Faces::find()->where(['type' => 'frown'])->one(); 
Faces::find()->where(['type' => 'frown'])->with('eyes')->one();
@qiangxue
Copy link
Member

@qiangxue qiangxue commented Apr 6, 2014

How about adding two methods: User::one() and User::all()?

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Apr 6, 2014

@qiangxue how then specify with and other things ?

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Apr 6, 2014

You have to use the full find() syntax.

@samdark
Copy link
Member

@samdark samdark commented Apr 6, 2014

Sounds good.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Apr 6, 2014

Ok, fine by me )

@samdark
Copy link
Member

@samdark samdark commented Apr 6, 2014

I can handle it if it's decided.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Apr 6, 2014

Yes, please go ahead. Thanks.

On Sunday, April 6, 2014, Alexander Makarov notifications@github.com
wrote:

I can handle it if it's decided.


Reply to this email directly or view it on GitHubhttps://github.com//issues/2999#issuecomment-39677357
.

@cebe cebe modified the milestone: 2.0 Beta Apr 6, 2014
@hiltonjanfield
Copy link
Contributor Author

@hiltonjanfield hiltonjanfield commented Apr 6, 2014

[redacted]
After a brief chat on IRC, I feel that re-using the same method name for different functionality is a mistake. How would you feel if someone created a class with a count() method that returned the number of times it had been accessed, rather than it's size?

Method names should be unique and descriptive to avoid confusion, especially among newcomers to Yii and those transitioning from Yii1. Re-using one() and all() with different functionality is confusing.

I still believe option 3 above is the best (Model::find()->pk(5);), which keeps method names short and distinctive, but if shortcuts are desired, at least use different names, like Model::findOne(5);, Model::findOne(['attributes' => $here]); and Model::findAll(['attributes' => $here]);.

@cebe
Copy link
Member

@cebe cebe commented Apr 6, 2014

👍 for findAll() and findOne()

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Apr 6, 2014

findOne() and findAll() sound good to me.

@samdark
Copy link
Member

@samdark samdark commented Apr 7, 2014

I don't think we really need findAll() since you need to set order, limit etc. all the time.

@samdark
Copy link
Member

@samdark samdark commented Apr 7, 2014

Ready for review.

@vova07
Copy link
Contributor

@vova07 vova07 commented Apr 7, 2014

My 5 cents. 1st example is more preferable for me. And also, no problem with current syntax, it's short and clear. For me is better to add one() of the end of my query, than write this like findOne. And at the same time find that return always all() records, isn't more consistent than the current syntax. I'm for keeping of that realy nice sintax that you have but with adjust it for more consistency like in 1st example or something similar.

@samdark
Copy link
Member

@samdark samdark commented Apr 7, 2014

@vova07 full syntax doesn't change at all so all these Post::find()->where(['a' => 'b'])->limit(10)->all() and Post::find()->where('a' => 'b')->one() are still there. find now always returns active query instance resulting in much cleaner code and IDE support.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Apr 7, 2014

@samdark can we do Post::find(['a' => 'b'])->all() ? Would be great to have it .

@samdark
Copy link
Member

@samdark samdark commented Apr 7, 2014

No.

@samdark
Copy link
Member

@samdark samdark commented Apr 7, 2014

That's easy to add but I'm not sure it's actually better API than Post::find()->where('a' => 'b')->all(). With where it reads as a sentence.

@vova07
Copy link
Contributor

@vova07 vova07 commented Apr 7, 2014

@samdark ok, sound good for me also. And what about findAll(), maybe then add this method to? We'll have find() for current syntax that always returns active query instance and two new metods findOne and findAll for consistency.

@samdark
Copy link
Member

@samdark samdark commented Apr 7, 2014

@vova07 I can't imagine any real situation where you don't care about limit and order in case of all() query. Do you have any cases?

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Apr 7, 2014

@samdark i think this shortcut is useful )

@vova07
Copy link
Contributor

@vova07 vova07 commented Apr 7, 2014

@samdark I'm fine with current syntax, but if we can find one active user for example, why we can't find all active users? For me findOne like findAll is unneeded methods.

@samdark
Copy link
Member

@samdark samdark commented Apr 7, 2014

@vova07 it's OK when you have 10 users but such query without a limit would kill any real world project.

@samdark
Copy link
Member

@samdark samdark commented Apr 7, 2014

@Ragazzo which shortcut? Parameters for find?

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Apr 7, 2014

@samdark yes

@vova07
Copy link
Contributor

@vova07 vova07 commented Apr 7, 2014

@samdark completely agree with you, It's good like it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.