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 type inferral for backed enums in Parameter as part of QueryBuilder #9372

Closed
tomudding opened this issue Jan 12, 2022 · 10 comments
Closed

Comments

@tomudding
Copy link

Feature Request

Q A
New Feature yes
RFC no
BC Break no

Summary

With the release of v2.11.0 just a few hours ago, support for PHP 8.1's enums was added. This works great, however, I would like to see that the setParameter() function (or rather the Parameter which is created in that function) has support for automatically inferring the type of backed enums.

Here an example:

enum Suit: int {
    case Hearts = 0;
    case Diamonds = 1; 
    case Clubs = 2;
    case Spades = 3;
}

#[Entity]
class Card
{
    /** ... */

    #[Column(type: "integer", enumType: Suit::class)]
    private Suit $suit;

Then at some point you could want to do something along the of:

$qb = $em->getRepository(Card::class)->createQueryBuilder('c');
$qb->where('c.suit = :suit')
    ->setParameter('suit', Suit::Hearts); // instead of Suit::Hearts->value

$qb->getQuery->getResult();

However, now you get the following error:

Error: Object of class Suit could not be converted to string.

Where I would have expected it to say integer. string is given because it is the default type (\Doctrine\DBAL\ParameterType::STRING) in \Doctrine\ORM\Query\ParameterTypeInferer::inferType. Which has no knowledge of what a backed enum is.

Clearly, this is also shows that DBAL is incomplete, as it has no idea what to do with the enum (which type of backed enum does not matter, you could force \Doctrine\DBAL\Types\Types::INTEGER in setParameter and DBAL would complain about being unable to convert the enum into an integer). However, that is an separate issue and not what this feature request is about (but it does depend on each other).

@derrabus
Copy link
Member

Please try out #9373.

@greg0ire
Copy link
Member

Clearly, this is also shows that DBAL is incomplete

Just for the record, it's unclear to me why the DBAL should know about enums… reading

try {
$value = $this->_em->getUnitOfWork()->getSingleIdentifierValue($value);
if ($value === null) {
throw ORMInvalidArgumentException::invalidIdentifierBindingEntity();
}
} catch (MappingException | ORMMappingException $e) {
/* Silence any mapping exceptions. These can occur if the object in
question is not a mapped entity, in which case we just don't do
any preparation on the value.
Depending on MappingDriver, either MappingException or
ORMMappingException is thrown. */
$value = $this->potentiallyProcessIterable($value);
}
, I'm under the impression that you can use setParameter with an entity, and the DBAL has no knowledge of entities either.

@tomudding
Copy link
Author

Please try out #9373.

@derrabus Thank you for looking into this! I have tested the PR and it works as expected, much appreciated.

Clearly, this is also shows that DBAL is incomplete

Just for the record, it's unclear to me why the DBAL should know about enums… reading

try {
$value = $this->_em->getUnitOfWork()->getSingleIdentifierValue($value);
if ($value === null) {
throw ORMInvalidArgumentException::invalidIdentifierBindingEntity();
}
} catch (MappingException | ORMMappingException $e) {
/* Silence any mapping exceptions. These can occur if the object in
question is not a mapped entity, in which case we just don't do
any preparation on the value.
Depending on MappingDriver, either MappingException or
ORMMappingException is thrown. */
$value = $this->potentiallyProcessIterable($value);
}

, I'm under the impression that you can use setParameter with an entity, and the DBAL has no knowledge of entities either.

@greg0ire My apologies, totally an oversight and misunderstanding on my part! There is indeed no reason that DBAL should be expected to know anything about "complex" objects (potentially passed on from ORM), only data in one of its primitive types.

@pkly
Copy link

pkly commented Jan 31, 2022

I would also like to see this working, as I've just had the same issue, but this time with the findBy / findOneBy methods.

On that point, why not just handle this simple conversion inside of Dbal/Connection same as is done with arrays inside of executeQuery?
As a helper it could be done in a query builder but the type information really should be available when using things like findBy, otherwise crashes will occur.

@derrabus
Copy link
Member

Closing because this feature has been implemented in #9373.

@derrabus
Copy link
Member

@pkly Can you please give #9453 a try?

@pkly
Copy link

pkly commented Feb 1, 2022

@derrabus I'm unable to test them at this moment. I did look at both MRs and they look good to me, so as soon as 2.12 releases I'll update my project and check.

@tourze
Copy link

tourze commented Apr 10, 2022

Good feature, when can we test it on 2.12.x ?

@derrabus
Copy link
Member

Good feature, when can we test it on 2.12.x ?

Nobody's holding you back.

@pkly
Copy link

pkly commented May 5, 2022

Oh yeah I forgot to mention it but it seems it works perfectly fine ever since I upgraded to 2.12.
Many thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants