active-record
active-record copied to clipboard
yiisoft
Make findByCondition() public or allow findOne()/findAll() query condition modification
I cannot even count how many times I've encountered this issue.
I want to call SomeRecord::findAll($pks) where $pks is an array of primary keys.
However, I just want to pass another condition into that query or set a custom index() or different order()... But I can't because findAll() goes straight to all().
Ok so, let me just call SomeRecord::findByCondition($pks)->myCondition()->all() then... Well darn, I can't do that either since it's protected.
Could we make findByCondition() public? Either that or add a second callable $query parameter to findAll() and findOne() (similar to how it's done in Query::with())?
How about the following?
SomeRecord::find()->where(['id' => $pks])->myCondition()->all()
@samdark That doesn't work. You are assuming the PK name is 'id'. It is not, it can be anything.
Can we please reopen this?
Would this cause confusion with extra conditions?
e.g.
SomeRecord::findByCondition($pks)->where('true=false')->all()
Would override the where set by findByCondition.
It would of course, but this problem is not unique to findByCondition(). Anytime you call where() you are overriding any previously set conditions (e.g. a soft delete condition set in activequery init()) which is the expected behavior of where() (as opposed to andWhere()).
We could also move the findByCondition() logic into ActiveQuery and name it primaryKey($key) (or andPrimaryKey())? It would solve this issue, avoid any confusion and be incredibly useful in general.
@vercotux
You are assuming the PK name is 'id'. It is not, it can be anything.
But you should know your PK name, so just replace id key with name of PK column.
@rob006 That is not true. There are countless situations where you do not know the PK name, such as creating a behavior or some other form of abstract component that is supposed to work with different models.
@vercotux If you don't know PK name you probably don't know anything about table schema. How do you want to add additional conditions, if you don't know what columns this table contains.
@rob006 Oh really? Then what are findAll() and findOne() doing in BaseActiveRecord if they "don't know PK names"? I think you may be confused. When I say I "don't know" the PK name I mean that it is a variable, which is a very common situation.
@vercotux I'm talking about column names, not values. If you don't know which column is used as PK, how could you create any other condition (since you should know nothing about any other column name in this table).
If you can't use SomeRecord::find()->where(['id' => $pks])->andWhere(['status' => 1])->all() because "I don't know that id is my PK column", then how could you know that you should use status as column name in andWhere()?
@rob006 I know you are talking about column names. I never mentioned any values. And of course you can't add conditions to any unknown columns. Nobody is trying to do that.
You do know which column is used as a PK. See how findByCondition() does it.
What you can do is add more conditions to the same PK column. You can also index it. You can specify a different order by direction. You can add a limit etc and plenty of other things. None of this requres you to know the full schema.
@vercotux You can't use indexBy() or orderBy() if you don't know column names. And I can't really imagine any useful usage of limit() if you can't set order.
Anyway, I'm against making findByCondition() public. We could add something like andWherePkIs($value) to ActiveQuery, but this is debatable. At least one real use case would be helpful.
@rob006
You can't use indexBy() or orderBy() if you don't know column names.
But you can know the PK name, so you can use these methods. You just need to re-implement exactly what is already implemented in findByCondition() first. And that's fine - you don't need to make this particular method public, just allow us to somehow reuse the logic that is inside.
There's tones of use cases for it. Here's a just few off the top of my head:
SomeRecord::find()->andWherePkIs($pks)->asArray()->all(); // selfexplanatory
SomeRecord::find()->andWherePkIs($pks)->limit(5)->offset(5)->all(); // paginate through $pks that are found
SomeRecord::find()->andWherePkIs($pks)->exists(); // check if any of the $pks exist
SomeRecord::find()->andWherePkIs($pks)->count(); // count how many of the $pks exist
SomeRecord::find()->andWherePkIs($pks)->column();
SomeRecord::find()->andWherePkIs($pks)->each();
SomeRecord::find()->andWherePkIs($pks)->myQueryMethod()->one();
If there was also a way to invert the condition you could do things like search for all records with an exclusion list:
SomeRecord::find()->andWherePkIsNot($exclude_pks)->all();
You could use that for example to very conveniently specify a unique validator 'filter' to exclude itself ($query->andWherePkIsNot($model->getPrimaryKey());).
What about?
SomeRecord::find()->where([SomeRecord::primaryKey()[0] => $pks])->myCondition()->all()
I saw no real case to change anything. Maybe, adding some condition syntax like ['#pk#' => [...]] will be better...
@vercotux Accessing PK when you don't know it's column name is not a common case. But if you really need it, you can create own helper for that.
@vercotux, i'm neutral to public findByCondition(). But I'm against a bunch of new methods: (and/or)wherePkIs, (and/or)wherePkIsNot... It's redundant.
@rob006
Accessing PK when you don't know it's column name is not a common case. But if you really need it, you can create own helper for that.
Then remove findOne() and findAll() because you are contradicting yourself.
@ptz-nerf Redundant to you because you've never dug deep enough to need that.
@FabrizioCaldarelli I gave you examples. You want me to paste the entire NDA'd project in here?
Also if it's SUCH A HASSLE making a tiny improvement that has no downsides (you just can't see the benefits yourself) then forget it. I'll just make my own improvements. Why waste my time.
I want to hear @yiisoft/core-developers and @yiisoft/reviewers opinions on the matter.
@vercotux I think that having multiple ways to solve a question can confuse the developer. As last solution you can dinamically get primary key from table schema and then apply andWhere() methods to $query object.
@FabrizioCaldarelli Oh so you are also in favor of removing findAll()! It does the same as AR::find()->all() so we better remove it "not to confuse the developer". What a stupid argument.
@vercotux, @FabrizioCaldarelli stop it please. Nothing good comes of it. Let's hear other opinions then decide. Thanks.
instead of having another method on the activerecord, we could add something like wherePrimaryKey() to the ActiveQuery.
User::find()->wherePrimaryKey(1)->one();
User::find()->wherePrimaryKey([1, 2, 3])->all();
we could even deprecate findByCondition() afterwards.
Im against such shortcuts. Especially for usecases like this. Its very rare & adds confusion.
