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

Fix enum hydration when fetching partial results #9657

Merged
merged 7 commits into from
Apr 16, 2022

Conversation

michnovka
Copy link
Contributor

This is the first attempt to fix the issue #9622

@michnovka michnovka changed the title First attempt on fix of issue #9622 Fix enum hydration when fetching partial results Apr 12, 2022
@michnovka
Copy link
Contributor Author

I suggest the following:

in AbstractHydrator use:

case isset($cacheKeyInfo['isScalar']):
    $type = $cacheKeyInfo['type'];

    if (isset($cacheKeyInfo['enumType'])) {
        $value = $type->convertToPHPEnumValue($value, $this->_platform, $cacheKeyInfo['enumType']);
    } else {
        $value = $type->convertToPHPValue($value, $this->_platform);
    }

    $rowData['scalars'][$fieldName] = $value;

    break;

And in DBAL extend the Type class:

    /**
     * Converts a value from its database representation to its PHP enum representation
     * of its type
     *
     * @param mixed $value The value to convert
     * @param AbstractPlatform $platform
     * @param string $enumType
     * @return BackedEnum
     * @throws ConversionException
     */
    public function convertToPHPEnumValue($value, AbstractPlatform $platform, $enumType)
    {
        try {
            return $enumType::from($value);
        }catch(ValueError $error){
            throw new ConversionException("Cannot convert value to PHP enum", 0, $error);
        }
    }

This will work well for most types. I will overload the method in those, where it would be problematic, such as SimpleArrayType.

If somebody wants to have PHP enum support in their type, they can overload this method too.

There is no BC break.

@beberlei
Copy link
Member

This is a good fix given our current data structures.

It shows though that our reflection and type apis are not the best abstraction anymore.

I was thinking about something new for ORM 3 but had no time sofar. For 2.x we can use this approach

@michnovka
Copy link
Contributor Author

@beberlei So can I make the DBAL PR? I need it merged there before I can finish this in ORM, otherwise tests will fail.

@beberlei
Copy link
Member

I am reviewing on mobile phone, the current patch looks like it can work without dbal changes? The fix must be orm only

@michnovka
Copy link
Contributor Author

The current fix has the limitations which I described, it will ONLY work with string type. If we use SimpleArrayType or any custom type with enumType (which is supported and even present in our tests) it will throw. I have described above a solution which requires Type change but allows fix for ALL types, including giving ability to custom types to make a fix the way they want.

@beberlei
Copy link
Member

Couldnt you switch over the scalar type inside the enumMappibngs case and reimplement the simple array case? We are already in workaround mode here so local patching is fine. Add a comment that it reimplements the ReflectionEnumPriperty code there

@michnovka
Copy link
Contributor Author

I can and it was my first approach, but we will fix only these 2 cases. I realized enumType can be used on any type, so that solution was allowing that. So workaround it is?

@michnovka
Copy link
Contributor Author

This is ready for merge. @beberlei please review

@derrabus derrabus requested a review from beberlei April 15, 2022 13:44
beberlei
beberlei previously approved these changes Apr 15, 2022
Copy link
Member

@beberlei beberlei left a comment

Choose a reason for hiding this comment

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

Go from overall implementation pov, havent reviewed style and other non functional requirements.

@derrabus derrabus added this to the 2.12.0 milestone Apr 16, 2022
@derrabus derrabus added the Bug label Apr 16, 2022
@derrabus derrabus merged commit 2fe4067 into doctrine:2.12.x Apr 16, 2022
@derrabus
Copy link
Member

Thank you @michnovka!

derrabus added a commit that referenced this pull request Apr 19, 2022
* 2.12.x:
  Deprecate the doctrine binary (#9661)
  ScalarColumnHydrator: prevent early-bail on falsy values (#9663)
  Fix enum hydration when fetching partial results (#9657)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants