doctrine / doctrine-laminas-hydrator

Doctrine hydrators for Laminas applications
https://www.doctrine-project.org/projects/doctrine-laminas-hydrator.html
MIT License
33 stars 19 forks source link

Backed Enum Hydration ends up in a type conversion #56

Closed ezkimo closed 2 years ago

ezkimo commented 2 years ago

The Doctrine Hydrator can not handle backed enum typed entity properties, Let us discuss the following example.

enum Suit: string {
    case Hearts = 'H';
    case Diamonds = 'D';
    case Clubs = 'C';
    case Spades = 'S';
}

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

    #[Column(type: 'string', enumType: Suit::class)]
    public Suit $suit;

    public function getSuit(): Suit
    {
        return $this->suit;
    }

    public function setSuit(Suit $suit): void
    {
        $this->suit = $suit;
    }
}

As you can see the get and set method are typed as Suit and not as string because the value is an instance of the Suit enumeration. For this purpose I created a hydrator strategy.

namespace Marcel\Hydrator\Strategy;
use Laminas\Hydrator\Strategy\StrategyInterface;

class SuitHydratorStrategy implements StrategyInterface
{
    public function hydrate($value, ?array $data)
    {
        return Suit::from($value);
    }

    public function extract($value, ?object $object = null)
    {
        ...
    }
}
use Doctrine\Laminas\Hydrator\DoctrineObject as DoctrineHydrator;
use Marcel\Entity\Card;
use Marcel\Hydrator\Strategy\SuitHydratorStrategy;

$hydrator = new DoctrineHydrator($entityManager);
$hydrator->addStrategy('suit', new \Marcel\Hydrator\Strategy\SuitHydratorStrategy());

$card = new Card();
$hydrator->hydrate([ 'suit' => 'H' ], $card);

This will end up in a type conversion error since DoctrineObject::handleTypeConversions($value, ?$typeOfField) does not recognize backed enums. The $value param holds an instance of the Suit enum, which implements the BackedEnum interface. One could easily avoid this behaviour by adding the following line of code.

protected function handleTypeConversions($value, ?string $typeOfField)
{
    ...
    if (interface_exists('BackedEnum') && $value instanceof BackedEnum) {
        $value = $value->value;
    }
    ...
}
driehle commented 2 years ago

Thanks for reporting this issue. Indeed, such hydration is not handled properly, mostly because backed enum typed properties are a rather new feature of Doctrine. Could you please provide a PR including the fix and relevant test cases based on the 2.2.x branch? I'll be happy to review and merge your PR in the 2.2 and 3.0 series.

HeyRatFans commented 2 years ago

I have been looking into adding enum support on the side too (unofficially of course - having been playing around with 8.1's enums and Doctrine's new enum and attribute support).

Unless I am mistaken, @ezkimo I do not think your suggestion will work for hydration?

My understanding is that to accomplish hydration for enums we would need access to the ClassMetadataInfo::getFieldMapping() method to be able to determine the enumType from the attributes/annotations of the field itself, at least in the case of a non-typed setters and properties.

Unfortunately, this method is not defined on the ClassMetadata interface in Doctrine/Persistence, only in the concrete implementation in Doctrine/ORM (and some but not all the ODM packages!) and as such would need to be added upstream in Doctrine/Persistence. I am uncertain whether doing this would be a breaking change or not.

An alternative option could be to use Reflection to get the enum class name and use BackedEnum::tryFrom(), but this would enforce the use of typed setters and properties (for both by-reference and by-value hydration) to be used in the target Entity. This is not a terrible thing IMO.

driehle commented 2 years ago

This is a common issue in the hydrator library, we have the same issue for isNullable:

https://github.com/doctrine/doctrine-laminas-hydrator/blob/a1c99f488d05c394bc5347f4f396cf4053f81409/src/DoctrineObject.php#L726-L742

Here isNullable() is only present in Doctrine\ORM\Mapping\ClassMetadata (inherited from Doctrine\ORM\Mapping\ClassMetadataInfo), but not in Doctrine\Persistence\Mapping\ClassMetadata. Same for getAssociationMapping().

I would suggest using a similar approach for getFieldMapping().

HeyRatFans commented 2 years ago

It looks like this would work, but would be untestable due to PHPUnit rightfully not allowing us to mock getFieldMapping on Doctrine\Persistence\Mapping\ClassMetadata.

I think @ezkimo solution of requiring a Strategy for each enum be implemented coupled with something like the below to DoctrineObject::hydrateValue could be a solution (note: returning the enum instead of enum->value here as previously proposed)

if (interface_exists('BackedEnum') && $value instanceof BackedEnum) {
    return $value;
}

The only other option would be the previously use of Reflection, which would only work by ignoring any attributes/annotations and trusting the entity has the correctly typed property/setter on the entity class.

driehle commented 2 years ago

Since Enums have never been supported till now, we will consider this a new feature and bring it into the upcoming 3.1.0 release. However, we will need someone to write a few notes for the docs (see #57, #59).

driehle commented 2 years ago

Version 3.1.0 of doctrine-laminas-hydrator has just been released!