Add error handling when misuse of find() with array values #11237
Conversation
|
The current behavior is not a bug. For DX improvements, please target our next feature release which is 3.1. |
| if (array_filter($identifier, 'is_array')) { | ||
| throw new UnexpectedValueException('Unexpected identifier value: Expecting scalar, got array.'); | ||
| } |
There was a problem hiding this comment.
I think, we can handle this case in the mapper function below. And we could throw that exception if we encounter anything that is not a scalar or stringable.
There was a problem hiding this comment.
Done in new PR targeting 3.1.x branch #11285
There was a problem hiding this comment.
@derrabus Unfortunately we cannot do that because it breaks the find by custom id object feature :
$ vendor/bin/phpunit
1) Doctrine\Tests\ORM\Functional\CustomIdObjectTypeTest::testFindByCustomIdObject
UnexpectedValueException: Unexpected identifier value: Expecting scalar, got object.
2) Doctrine\Tests\ORM\Functional\CustomIdObjectTypeTest::testFetchJoinCustomIdObject
UnexpectedValueException: Unexpected identifier value: Expecting scalar, got object.
3) Doctrine\Tests\ORM\Functional\CustomIdObjectTypeTest::testFetchJoinWhereCustomIdObject
UnexpectedValueException: Unexpected identifier value: Expecting scalar, got object.
4) Doctrine\Tests\ORM\Functional\PaginationTest::testCustomIdTypeWithoutOutputWalker
UnexpectedValueException: Unexpected identifier value: Expecting scalar, got object.
# etc...
There was a problem hiding this comment.
But those custom IDs need to be stringable, otherwise the implode below would fail. I don't understand the issue here.
There was a problem hiding this comment.
In this PR I'm just checking that the "array" type is invalid.
So the only check we can do inside the mapper function is to check that $value is an array
if (is_array($value)) {
throw new UnexpectedValueException(...);
}This bit of code is not dealing with other types like objects.
Do you want me to move the is_array($value) condition in the mapper function then ?
There was a problem hiding this comment.
In this PR I'm just checking that the "array" type is invalid.
Which is a bit arbitrary. If we add validation here, let's do it properly and catch everything that we don't accept.
This bit of code is not dealing with other types like objects.
But it should.
There was a problem hiding this comment.
So you mean we should do something like
if (! is_scalar($value) && ! ($value instanceof Stringable)) ...?
There was a problem hiding this comment.
@derrabus I commited in the new PR targeting 3.1.x, I hope that's what we want 🤞
Created new PR targeting |
Issue
When we (inexpertly) use
$em->find(...)with array values in identifer, it returns a PHP warning..See all details and how to reproduce it in the issue : #11236.
Solution
In this PR