-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add processing binary binding type #11734
base: 3.3.x
Are you sure you want to change the base?
Conversation
05d5144
to
05d6524
Compare
…essed parameter types
05d6524
to
88e20bf
Compare
You need to update the phpstan baseline, it ignores an issue you addressed. |
…abikor/doctrine-orm into processing-binary-binding-type
@@ -1940,6 +1946,8 @@ private function getArrayBindingType(ParameterType|int|string $type): ArrayParam | |||
ParameterType::STRING => ArrayParameterType::STRING, | |||
ParameterType::INTEGER => ArrayParameterType::INTEGER, | |||
ParameterType::ASCII => ArrayParameterType::ASCII, | |||
ParameterType::BINARY => ArrayParameterType::BINARY, | |||
default => throw new QueryException('Unsupported type for array parameter'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the default
branch here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default is necessary because not all ParameterType cases, such as BOOLEAN, LARGE_OBJECT, or NULL, are supported by ArrayParameterType. The default branch ensures these unsupported types are handled gracefully by throwing a clear exception, preventing unexpected behavior or runtime errors.
* @throws QueryException | ||
* @throws Exception |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what situations is this method raising these exception? Is the caller supposed to handle it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This answers neither the first nor the second of my questions. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method doesn't handle these exceptions directly. I included them to show which ones the method can raise. If you think this is unnecessary, I’ll remove them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exception removed. QueryException throws by this method
Updated PR Description
Problem Statement
When using the
findBy
method in the repository with binary values as parameters, an unhandled error occurs:Unhandled match case 16
.How to reproduce:
This error originates from the following code in the
BasicEntityPersister
class:[BasicEntityPersister.php#L1930-L1941](https://github.com/doctrine/orm/blob/3.3.0/src/Persisters/Entity/BasicEntityPersister.php#L1930-L1941).
The current implementation does not handle
ParameterType::BINARY
properly, leading to unexpected behavior instead of throwing an appropriate exception or processing binary values.Actual Behavior
ParameterType::BINARY
, the code fails withUnhandled match case 16
.Expected Behavior
findBy
method.Changes Made in This PR
BasicEntityPersister
class to processParameterType::BINARY
correctly.Impact of Changes
Unhandled match case 16
error for binary parameters.findBy
methods.