Provide for existing type to be of same output type for ConvertToPHPValue

[TomHAnderson]

Q	A
Type	improvement
BC Break	no

Summary

The DoctrineObject Hydrator of DoctrineModule should use the Type objects to convert values into and out of arrays. Currently only a mediocre filter is used for the most basic of DateTime types.

Some of the existing Types (DateTimeType, DateTime, others) code in convertToPHPValue allow types of the same output type to be input data. This is desired. This PR fixes the types which do not allow such input.

Hydrators

It is a goal of this change to create hydrators which fully support custom types.

/**
     * Handle various type conversions that should be supported natively by Doctrine (like DateTime)
     *
     * @param  mixed  $value
     * @param  string $typeOfField
     * @return mixed
     */
    protected function handleTypeConversions($value, $typeOfField)
    {
        if (! $typeOfField) {
            return $value;
        }
        $type = Type::getType($typeOfField);
        $platform = $this->objectManager->getConnection()->getDatabasePlatform();

        return $type->normalizeToPHPValue($value, $platform);
    }

Currently this function exists as https://github.com/doctrine/DoctrineModule/blob/3e21660d9ddce4cc223ebc2062c9a2824181bd60/src/DoctrineModule/Stdlib/Hydrator/DoctrineObject.php#L531-L588 and does not support custom field types.

[Ocramius]

IMO this looks good: some performance overhead is involved, but I don't think it is relevant, and indeed does avoid double conversions.

Still, I don't think this is the correct fix for the underlying issue: types should be well-defined in and out, and these sort of "already PHP type shouldn't be converted again" shouldn't happen multiple times.

I think the patch is fixing a symptom, rather than the root cause of the issue.

[TomHAnderson]

Users are currently using the DoctrineObject hydrator to hydrate values they are creating themselves such as

$doctrineObject->hydrate([['simple_array field', 'element2']], $entity);

which, if the hydrator adhered strictly to Types would read

$doctrineObject->hydrate(['simple_array_field,element2'], $entity);

but this is counterintuitive because users are not thinking in raw database output. This patch fixes this behavior.

But the bigger issue is custom types. A custom type must use a Type object because the input and output values could be anything. In my case I created a datetime_microsecond type which was handled like a DateTime value but there was nothing to convert the value from a string to an object when hydrating the entity. Fixing custom types is the larger correction here.

[Ocramius]

but this is counterintuitive because users are not thinking in raw database output.

Right, but this is doctrine/dbal after all, so it is designed for RDBMS<->PHP conversions, and pushing semantics from a different layer into it increase complexity and implicit considerations by a lot.

[TomHAnderson]

I agree the considerations are increased. I chose this path for the hydrator because we were already handling the conversion in most cases and I corrected only the few which did not conform to the pattern the others already followed.

Modifying these in doctrine/dbal was chosen because when a user creates a custom type it is for this layer but needs handling in the hydrator. Tracing back to where a custom type acts I landed in dbal Types.

[Ocramius]

Aye, and that is fine to some degree, just unsure if we should really do it here, since we're no longer talking about what comes fro.m the RDBMS, and we're actually being too lax on the user.

Instead, types should probably be stricter and throw more (for example when something is already a DateTime, it should just crash).

That's my feedback btw - let's see what other DBAL maintainers think.

[TomHAnderson]

doctrine/DoctrineModule#647

[morozov]

I'm with @Ocramius on this one. I'd better see the code made stricter in 3.0 by not checking if the value is already of the desired type rather than add more complexity to the code and lose this opportunity.

[TomHAnderson]

This is an opportunity with two directions:

1. Remove existing code to deliberately break invalid parameters. This reduces the Type code, which is called often, by one if statement with a single return if the output type matches the input type.

• This will happen when users supply their own values for a hydrator such as an array for a simple_array type.

2. Expand the code (this PR) to cover input types equal output types. This supercharges Hydrators by allowing each extracted field to be processed through these Type classes.

• Hydrator changes are at the heart of this change.

[morozov]

This reduces the Type code, which is called often, by one if statement with a single return if the output type matches the input type.

It explicitly removes the responsibility of the API for which it wasn't designed.

Instead of being used for conversion of the data retrieved from the DB, the types are used for conversion of data provided by a developer which doesn't look like a valid argument. If we want to let the developer know if the value is already of the expected type, there should be a separate method for that which hydrator would call before conversion if it's a valid case for it.

[TomHAnderson]

I also see a separate class to handle this. But that new set of classes would mirror the existing Types and would be required in addition to any user custom Type.

I approached this from creating a new custom type. There was not a method of the hydrator to properly extract the value from my custom class. So should I have to write a second custom class which only the hydrator uses to handle extracting from that custom type? My custom type and many of the existing types already handled the job I needed them to do which is hydration.

So if a fleet of TypeSupport files is needed in order to properly fix the hydrator I would only do that if I cannot convince dbal of the elegance of this pr.

[morozov]

I also see a separate class to handle this.

No, I meant a new method of Type, not a new set of classes.

[TomHAnderson]

I created a new PR with the idea of a new function to handle type conversions on Type objects.

[Majkl578]

My main problem with this PR is that it complicates Type stuff even further, making duplicated calls part of the API (by adding normalization).
Also this behaviour may be problematic if the database and PHP types are the same (consider string transforming into another string).

I'd prefer this:

@morozov: [...] I'd better see the code made stricter in 3.0 by not checking if the value is already of the desired type rather than add more complexity to the code and lose this opportunity.

[Erikvv]

I like this approach with the extra method. It still allows restricting ->convertToPhpValue()

It's not a core responsibility of DBAL but from a type/hydrator perspective it has to be put somewhere.

If not here you get a more complex solution requiring extra configuration by the user and/or the author of the custom type has to supply conversion classes for every framework (not that I'm familiar with anything other than Zend).

I'm sure you can come up with a case that won't work. The user is free to pick any serialization format after all. But it certainly works in common cases like Enum and UUID.

Also this behaviour may be problematic if the database and PHP types are the same (consider string transforming into another string).

Not really an issue as you can regex for an expected token in the serialization format.

[greg0ire]

Approving because I'm having a hard time understanding in what cases specifically this will be useful and I don't want to prevent a merge.

[Majkl578]

Still 👎 from me here for the reasons stated above:

• complicates Type even further, instead of the overall aim to simplify/split it;
• rather than treating Types as single-pass converters, this now leverages them as multi-pass normalizers, which could/will lead to more duplicated calls.

Also this does not handle cases when input and output is of the same type. Take for example ReverseStringType, how do you know whether the value has already been converted or not?

[morozov]

The current implementation violates the SRP since every single implementation of normalizeToPHPValue() calls convertToPHPValue() internally. The diff between the two would constitute the logic I tried to describe in #3291 (comment):

If we want to let the developer know if the value is already of the expected type, there should be a separate method for that which the hydrator would call before conversion if it's a valid case for it.

However, even that doesn't stand the use case @Majkl578 described above. Any non-primitive type would have this issue.

[TomHAnderson]

The description for this PR states:

It is a goal of this change to create hydrators which fully support custom types.

In order to execute that goal I have designed this PR to where it is today. But the issues raised don't address the original goal; perhaps too many changes had to be made to properly support custom types. Do we need to roll back all the changes and begin by removing the type checking from convertToPHPValue then walking each part forward so instead of having such a large dartboard to address? Or is that first step of removing type checking going too far already?

[TomHAnderson]

I've started a thread to talk through all the reasons I created this PR one at a time.
PLEASE become part of this conversation: https://doctrine.slack.com/messages/CA8MNL2UQ/convo/CA8MNL2UQ-1551052461.002200/?