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

Incorrect Mapping of BackedEnum #10077

Closed
menhir2015 opened this issue Sep 29, 2022 · 16 comments
Closed

Incorrect Mapping of BackedEnum #10077

menhir2015 opened this issue Sep 29, 2022 · 16 comments
Labels

Comments

@menhir2015
Copy link

menhir2015 commented Sep 29, 2022

Bug Report

doctrine/orm/lib/Doctrine/ORM/Mapping/ReflectionEnumProperty.php is not able to inizialize enum from its value

Q A
Doctrine\Dbal Version 3.4.5
symfony* 6.1.3
api-platform/core 2.7.0

Summary

in version 3.4.4 BackedEnum are mapped correctly

Current behaviour

"App\\Enum\\VatClassEnum::from(): Argument doctrine/dbal#1 ($value) must be of type string, App\\Enum\\VatClassEnum given"

immagine

basilicaly the value of $value should be a string not a BackeEnum

How to reproduce

backed enum
enum VatClassEnum : string { case I = 'I'; case C = 'C'; }

create an entity with the following field
#[ORM\Column(length : 4, nullable : true, enumType : VatClassEnum::class)] private ?VatClassEnum $vatClass = null;

Expected behaviour

correct hydratation of entity

@derrabus derrabus transferred this issue from doctrine/dbal Sep 29, 2022
@derrabus derrabus added the Bug label Sep 29, 2022
@michnovka
Copy link
Contributor

Can you provide a test case for this?

@NoiseByNorthwest
Copy link

Same issue for me.

The enum is constructed from a string twice in the hydration process, leading to this error the second time. The first time in this function https://github.com/doctrine/orm/blob/2.13.2/lib/Doctrine/ORM/Internal/Hydration/AbstractHydrator.php#L407 and the second time here https://github.com/doctrine/orm/blob/2.13.2/lib/Doctrine/ORM/UnitOfWork.php#L2745.

@bobvandevijver
Copy link
Contributor

This issue is introduced with 2.13.2, 2.13.1 works fine.

@balazscsaba2006
Copy link

balazscsaba2006 commented Oct 1, 2022

It is probably related to #10041. @michnovka Any ideas why this happens?

@sad270
Copy link

sad270 commented Oct 1, 2022

Hello, I have the same issue. It is already fixed in 2.13.x-dev with #10058 I'm just waiting for the next release 😄

@mortezakarimi
Copy link

I fix this issue by changing initializeEnumValue function in vendor/doctrine/orm/lib/Doctrine/ORM/Mapping/ReflectionEnumProperty.php to the following:

/**
     * @param object     $object
     * @param int|string $value
     */
    private function initializeEnumValue($object, $value): BackedEnum
    {
        $enumType = $this->enumType;

        try {
           // Check if value already is instance of $enumType return it, else get value from $enumType
            if($value instanceof $enumType){
                return $value;
            }
            return $enumType::from($value);
        } catch (ValueError $e) {
            throw MappingException::invalidEnumValue(
                get_class($object),
                $this->originalReflectionProperty->getName(),
                (string) $value,
                $enumType,
                $e
            );
        }
    }

@bobvandevijver
Copy link
Contributor

Hello, I have the same issue. It is already fixed in 2.13.x-dev with #10058 I'm just waiting for the next release 😄

I can confirm this 😄

@insystema
Copy link

insystema commented Oct 5, 2022

Hi, al!
I also caught this bug.

I updated symfone application from 6.1.3 to 6.1.5 where doctrine/orm has been updated from 2.13.1 to 2.13.2.

I found where this bug occurs

\Doctrine\ORM\Mapping\ReflectionEnumProperty::setValue()

was

public function setValue($object, $value = null): void
{
    if ($value !== null) {
        if (is_array($value)) {
            $value = array_map(function ($item) use ($object): BackedEnum {
                return $this->initializeEnumValue($object, $item);
            }, $value);
        } else {
            if ($value instanceof \UnitEnum) {
                $value = $value->getValue();
            }
            $value = $this->initializeEnumValue($object, $value);
        }
    }

    $this->originalReflectionProperty->setValue($object, $value);
}

I corrected as

public function setValue($object, $value = null): void
{
    if ($value !== null) {
        if (is_array($value)) {
            $value = array_map(function ($item) use ($object): BackedEnum {
                return $this->initializeEnumValue($object, $item);
            }, $value);
        } else {
            if ($value instanceof \UnitEnum) {
                $value = $value->getValue();
            }
            $value = $this->initializeEnumValue($object, $value);
        }
    }

    $this->originalReflectionProperty->setValue($object, $value);
}

and it all worked.
I do not know right or wrong, I hope the doctrine/orm developers know their craft.

@yivi
Copy link

yivi commented Oct 6, 2022

@devtronic Nothing stops you from simply adding "conflict": { "doctrine/orm": "2.13.2" } to your composer.json while this is sorted out.

@greg0ire
Copy link
Member

greg0ire commented Oct 6, 2022

@devtronic I gave an estimate, don't tell me you're going to treat it as a deadline? Who are you, a project manager?

@curry684
Copy link

curry684 commented Oct 7, 2022

This also fundamentally broke several of my production environments overnight today. While the conflict workaround is fine of course I do think the severe impact of this bug warrants actually yanking the 2.13.2 release - I upgraded 3 days ago while the cause was determined 7 days ago, I wouldn't have been affected had it been yanked.

@greg0ire
Copy link
Member

greg0ire commented Oct 7, 2022

I don't think we can do that if people not affected by this bug are referring to that release in their composer.json. I've asked internally for a review on another bugfix we'd like to ship with 2.13.3, I'll try to get it released soon.

@greg0ire
Copy link
Member

greg0ire commented Oct 7, 2022

Very, very soon: https://github.com/doctrine/orm/releases/tag/2.13.3

@greg0ire greg0ire closed this as completed Oct 7, 2022
@KhorneHoly
Copy link

Thank you @greg0ire and everyone involved for your effort and the quick fix!

@curry684
Copy link

curry684 commented Oct 7, 2022

I don't think we can do that if people not affected by this bug are referring to that release in their composer.json

Only if someone already changed the definition to 2.13.2 or ^2.13.2, and it would give a clear error that there is no longer a release matching the constraints. If the constraint still matches 2.13.1 Composer would just happily downgrade on the next update. Yanking is a very valid scenario in the case of accidentally releasing breaking bugs and fully supported by both Composer and Packagist.

composer/packagist#152

edit: and thanks for the new release :)

@greg0ire
Copy link
Member

greg0ire commented Oct 7, 2022

Will consider it next time, thanks 👍

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

No branches or pull requests