Additional psalm param typehint for orderBy argument in findBy method #8586
Conversation
| @@ -185,7 +185,7 @@ public function findAll() | |||
| * @param int|null $offset | |||
| * | |||
| * @psalm-param array<string, mixed> $criteria | |||
| * @psalm-param list<string>|null $orderBy | |||
| * @psalm-param list<string>|array<string, string>|null $orderBy | |||
There was a problem hiding this comment.
Good catch, my bad! Should list<string> be removed? Also, EntityPersister::loadAll() seems to have the same issue, and EntityPersister::getSelectSQL() could be improved.
There was a problem hiding this comment.
If list<string> can not be, so it can be removed
There was a problem hiding this comment.
Should i update EntityPersister::loadAll()?
There was a problem hiding this comment.
Yes please, and EntityPersister::getSelectSQL() too please 🙏
|
@greg0ire Updated! |
|
@orklah @vladyslavstartsev @simonberger please review, and please also tell me if you would rather I don't ping you for such reviews. |
|
I don't mind being pinged :) Ideally, this line in doctrine/persistence should be updated to match It could be the occasion to update some other case of $orderBy as well like in AbstractEntityPersister::getHash |
|
Maybe we could go even further, see doctrine/persistence#165 ?
Maybe this PR should only fix what's wrong so |
A list of string is incorrect, it actually looks like this: ['someField' => 'DESC', 'someOtherField' => 'ASC'…]`
|
I updated the PR according to that plan. |
|
Thanks @KartaviK ! |
Because of
findBy([], ['somField' => 'DESC']...);