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

Add processing binary binding type #11734

Open
wants to merge 16 commits into
base: 3.3.x
Choose a base branch
from

Conversation

rabikor
Copy link

@rabikor rabikor commented Nov 26, 2024

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:

$repository->findBy(['uuid' => ['<binary>']]);
Screenshot 2024-11-26 at 13 45 58

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

  • For unsupported parameter types like ParameterType::BINARY, the code fails with Unhandled match case 16.
  • This error occurs instead of processing the value or raising an explicit, meaningful exception.
  • There is no way to handle this behavior externally due to the unprocessed case in the repository logic.

Expected Behavior

  • The system should either:
    1. Process binary values appropriately when passed to the findBy method.
    2. Throw a clear and descriptive exception for unsupported parameter types.
  • Ensure compatibility for scalar values (e.g., binary data) within the repository methods.

Changes Made in This PR

  • Updated the handling logic in the BasicEntityPersister class to process ParameterType::BINARY correctly.
  • Introduced proper exception handling for unsupported parameter types, providing more clarity on the issue to users.
  • Ensured backward compatibility for all currently supported parameter types.

Impact of Changes

  • Resolves the Unhandled match case 16 error for binary parameters.
  • Improves error messaging and developer experience when unsupported parameter types are used.
  • Enhances support for handling binary data in findBy methods.

@rabikor rabikor force-pushed the processing-binary-binding-type branch from 05d5144 to 05d6524 Compare November 26, 2024 12:04
@rabikor rabikor force-pushed the processing-binary-binding-type branch from 05d6524 to 88e20bf Compare November 26, 2024 12:05
@rabikor rabikor changed the title Added processing binary binding type and default behaviour for unproc… Added processing binary binding type Nov 26, 2024
@rabikor rabikor changed the title Added processing binary binding type Add processing binary binding type Nov 26, 2024
@greg0ire
Copy link
Member

greg0ire commented Dec 6, 2024

You need to update the phpstan baseline, it ignores an issue you addressed.

@rabikor rabikor requested a review from greg0ire December 10, 2024 08:39
@rabikor rabikor requested a review from derrabus December 10, 2024 08:45
@@ -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'),
Copy link
Member

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?

Copy link
Author

@rabikor rabikor Dec 10, 2024

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.

Comment on lines 1936 to 1937
* @throws QueryException
* @throws Exception
Copy link
Member

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doctrine\DBAL\Types\Type::getType method throws Exception
Screenshot 2024-12-10 at 11 54 45

Copy link
Member

@derrabus derrabus Dec 11, 2024

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. 😉

Copy link
Author

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.

Copy link
Author

@rabikor rabikor Dec 11, 2024

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

@rabikor rabikor requested a review from derrabus December 10, 2024 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants