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 support for polymorphic types #5147

Closed
wants to merge 3 commits into from
Closed

Conversation

filiplikavcan
Copy link

@filiplikavcan filiplikavcan commented Dec 29, 2021

Q A
Type feature
BC Break no

Summary

The idea is to use FQCN as type name. Let's have Foo\Child extends Foo\Parent and register Foo\Parent type Type::addType('Foo\Parent', SomeType::class). Then TypeRegistry would automatically register SomeType::class also for Foo\Child type. SomeType would also know whether it handles Foo\Child or Foo\Parent value.

Example (Enum type implementation)

class EnumType extends PolymorphicType
{
    public function getSQLDeclaration(array $column, AbstractPlatform $platform): string
    {
        $enum = $this->getName();
        $reflection = new ReflectionEnum($enum);

        if ($reflection->getBackingType() === 'string') {
            $minMax = array_reduce($enum::cases(), function(array $minMax, $case) {
                return [
                    min($minMax[0], strlen($case->value)),
                    max($minMax[1], strlen($case->value)),
                ];
            }, [PHP_INT_MAX, 0]);

            if (!isset($column['length'])) {
                $column['length'] = $minMax[1];
                $column['fixed'] = $minMax[0] === $minMax[1];
            }

            return $platform->getVarcharTypeDeclarationSQL($column);
        }
        else {
            return $platform->getIntegerTypeDeclarationSQL($column);
        }
    }
    
    public function convertToPHPValue($value, AbstractPlatform $platform): ?\BackedEnum
    {
        $enum = $this->getName();

        if (empty($value) || !enum_exists($enum)) {
            return null;
        }

        return $enum::from($value);
    }

    public function convertToDatabaseValue($value, AbstractPlatform $platform)
    {
        return $value instanceof \BackedEnum ? $value->value : null;
    }
}
Type::addType(\BackedEnum::class, 'EnumType'); // Every PHP 8.1 enum type extends BackedEnum

enum Role: string 
{
    case ADMIN = 'admin';
    case USER = 'user';
}

enum Gender: string 
{
    case MALE = 'male';
    case FEMALE = 'female';
}

class User
{
    #[\Doctrine\ORM\Mapping\Column(type: Role::class)]
    private Role $role = Role::USER;

    #[\Doctrine\ORM\Mapping\Column(type: Gender::class)]
    private Gender $gender = Gender::FEMALE;
}

@filiplikavcan
Copy link
Author

Related: #4930

@greg0ire
Copy link
Member

Hi! Thanks for working on this! This is clever, but maybe not in a good way: I had a hard time understanding your PR, and I think if somebody asks me to explain it tomorrow, I will have a hard time too…

My idea was to have #5036 + something to make it easier to register types for each enum people have to make this more straightforward, and then have such mapping : #[\Doctrine\ORM\Mapping\Column(type: new EnumType(enum: Gender::class))], which is longer but also more explicit.

In your proposal, if an object implements 2 interfaces, and both interfaces map to a different type, and both types are polymorphic, the first one wins. Obviously this is not meant to happen and is unlikely to, but if feels like a hack designed because it will work with enums as opposed to something generic that could be used to implement something else like the encrypted type showcased in #5036

@greg0ire
Copy link
Member

Also, regarding enums, we have this proposal at the ORM level: doctrine/orm#9304

@filiplikavcan
Copy link
Author

filiplikavcan commented Dec 31, 2021

In your proposal, if an object implements 2 interfaces, and both interfaces map to a different type, and both types are polymorphic, the first one wins. Obviously this is not meant to happen and is unlikely to, but if feels like a hack designed because it will work with enums as opposed to something generic that could be used to implement something else like the encrypted type showcased in #5036

Yes, the first motivation behind this PR was to implement Enum type. So maybe if it drops generic class/enum inheritance and supports only enums it would be both better to understand and without class type inheritance ambiguity?

I agree that a proper DI for types which would enable setting up any object type instance is the proper solution for other use cases (encryption, etc.).

ORM level for Enum is fine but I don't think that from ORM point of view the column is of a type string/int. It is Enum type and the string/int is just an implementation detail. That's one of the reasons I think dbal is a better place for implementing Enum types.

PS: I'm getting lost in this too. ;) I think what makes it harder to understand is that there are two "types": 1. type NAME 2. type IMPLEMENTATION. Type name used to be non FQCN string (like "json", "object", "int", etc.). This PR works with FQCN as NAME and then uses class inheritance as NAME inheritance. For example: BackedEnum => Role or App\Entity\Thing => App\Entity\Car. Then, once IMPLEMENTATION is registered for a parent NAME it is shared with all NAMEs (and each IMPLEMENTATION knows about a specific NAME).

type NAME type IMPLEMENTATION
"date" DateType
\BackedEnum::class EnumType
Role::class (extends \BackedEnum) EnumType (from implementation for \BackedEnum::class name)
Gender::class (extends \BackedEnum) EnumType (from implementation for \BackedEnum::class name)
App\Entity\Thing:class ThingType
App\Entity\Car::class (extends App\Entity\Thing) ThingType (from implementation for App\Entity\Car::class name)

@greg0ire
Copy link
Member

Let's see where the ORM PR goes, OK? It's very promising and if your end goal is to have enum support, it should make you happy :)

@greg0ire
Copy link
Member

greg0ire commented Jan 8, 2022

It got merged, let's close!

@greg0ire greg0ire closed this Jan 8, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants