Allow custom DBAL types autoconfiguration from PHP types

[michnovka]

Work in progress to implement #9561

I wanted to use this syntax in configuration:

doctrine:
    orm:
        typed_field_mapping:
            App\Objects\CustomObject: App\DBAL\Types\CustomTypeFQCN
            App\Objects\CustomObject2: custom_type2_using_name_from_dbal

The problem I am facing is that ClassMetadataInfo does not seem to have access to the Configuration object. I wonder what would be the proper strategy here?

I've explored some ideas, like adding another param to the ClassMetadataInfo::mapField($mapping) function, but even its callers do not have access to the Configuration object.

@beberlei @derrabus @greg0ire please, any ideas?

[michnovka]

I propose this solution:

• new Configuration option added with helper functions to get/set/add typedFieldMappings
• this mapping is inserted into ClassMetadata during construction (using ClassMetadataFactory)
• only if the field is not in the list of built-in types with automatic mapping (DateInterval, DateTime, DateTimeImmutable,
  array, bool, float, int) it is checked whether we have configured custom PHP type => DBAL Type mapping

I have also updated docs under Basic Mapping section.

TODO: DoctrineBundle needs to be updated to support config syntax

doctrine:
    orm:
        typed_field_mapping:
            App\Objects\CustomObject: App\DBAL\Types\CustomTypeFQCN
            App\Objects\CustomObject2: custom_type2_using_name_from_dbal

@derrabus @greg0ire please review

[michnovka]

@greg0ire all feedback is incorporated, please review

[michnovka]

btw, 2.14 release is very close, id like to include this in that release please. Can you add it to the milestone @derrabus please?

[beberlei]

So far so good, I have one improvement idea, but generally this is great, thank you for working on it

[michnovka]

@beberlei I took your suggestion and extended the implementation to allow overriding of the default types as well. Added test for this as well as documentation

[michnovka]

I have added better psalm type for the typedFieldMappings and associated functions for the PHP types which can be mapped. This PR is finished imo and ready for merge.

[michnovka]

I got one more idea, for another config option, but it should be part of this commit I think:

doctrine:
    orm:
        typed_field_mapping:
            App\Objects\CustomObject: App\DBAL\Types\CustomTypeFQCN
            App\Objects\CustomObject2: custom_type2_using_name_from_dbal
        automatic_enum_field_mapping: true #default false

If this is set, it would consider

#[Column]
public MyEnum $enumField;

as

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

(of course string or int based on enum backing type)

WDYT?

[michnovka]

well actually this is already implemented, and it is always used, i.e.

#[Column]
public MyEnum $enumField;

will work just fine. However I got another idea in the process. Imagine you have a custom DBAL type for enums. All your enums in your code are supposed to use this DBAL type. With the feature as is now in this PR, the only way is to list all your enums like

doctrine:
    orm:
        typed_field_mapping:
            App\Enum\Enum1: App\DBAL\Types\CustomEnumType
            App\Enum\Enum2: App\DBAL\Types\CustomEnumType
            # ...

Proposal:

Lets move the ClassMetadataInfo::validateAndCompleteTypedFieldMapping() function into a newly created class:

class DefaultTypedFieldMapper implements TypedFieldMapper

where

interface TypedFieldMapper
{
    /**
     * Validates & completes the given field mapping based on typed property.
     *
     * @param  array{fieldName: string, type?: mixed} $mapping The field mapping to validate & complete.
     *
     * @return array{fieldName: string, enumType?: string, type?: mixed} The updated mapping.
     */
    public function validateAndCompleteTypedFieldMapping(array $mapping, ReflectionProperty $field): array;

    /**
     * Registers a custom PHP type => DBAL Type mapping for typed Entity fields.
     * With this configuration option used there is no need to specify the Column::type
     * for given explicitly typed field.
     *
     * @psalm-param class-string|ScalarName   $phpType  the FQCN of PHP type to be mapped
     * @psalm-param class-string<Type>|string $dbalType either FQCN of DBAL Type or its name
     */
    public function addTypedFieldMapping(string $phpType, string $dbalType): void;
}

we would then allow this config as well:

doctrine:
    orm:
        typed_field_mapper: App\ORM\CustomTypedFieldMapper
        typed_field_mapping:
            App\Objects\CustomObject: App\DBAL\Types\CustomTypeFQCN
            App\Objects\CustomObject2: custom_type2_using_name_from_dbal

such custom typed field mapper would be able to check e.g. for inheritance using instanceof, compare type names or whatever flexibility is desired. Notice that we can still add custom typed field mapping in the config, this would be passed to the TypedFieldMapper using its addTypedFieldMapping function. The mapper then decides whatever to do with this information.

The DefaultTypedFieldMapper would behave in the exact same way as it is implemented now in this PR. But say you want to assign all classes from your App\CusomtObjects\Stringable namespace to the string DBAL type. With this feature, all you need is a custom TypedFieldMapper and no need to list all the types individually.

WDYT @greg0ire @derrabus @beberlei ?

[stof]

Notice that we can still add custom typed field mapping in the config, this would be passed to the TypedFieldMapper using its addTypedFieldMapping function. The mapper then decides whatever to do with this information.

this requires the TypedFieldMapper to be a mutable object, preventing sharing the same instance between multiple configuration objects (as that would merge the mappings of each configuration). This looks like a bad design to me.

I would rather keep this simple mapping as a constructor argument of the DefaultTypedFieldMapper instead (and custom mappers are free to implement their config as they want)

[michnovka]

this requires the TypedFieldMapper to be a mutable object, preventing sharing the same instance between multiple configuration objects (as that would merge the mappings of each configuration). This looks like a bad design to me.

The main reason I wanted to implement is this way is that I still want to allow config like this:

doctrine:
    orm:
        typed_field_mapping:
            App\Objects\CustomObject: App\DBAL\Types\CustomTypeFQCN
            App\Objects\CustomObject2: custom_type2_using_name_from_dbal

where the default type mapper is used, but a list is provided for some additional mappings.

I dont understand why mutability of the TypedFieldMapper is important, one instance will be created and passed into Configuration object, then this will be passed to ClassMetadataInfo from the ClassMetadataFactory in most cases. I do not expect multiple TypedFieldMappers to be used in a project, unless you are working with ClassMetadataInfo object directly. Most people will just use the DoctrineBundle and specify one TypedFieldMapper if they want some advanced type mapping logic, but even more of the people will likely just use the DefaultTypedFieldMapper and only specify few additional types explicitly.

[michnovka]

@stof ok, I think I get your point now, so how about this:

<?php

declare(strict_types=1);

namespace ORM\Mapping;

use Doctrine\DBAL\Types\Type;
use Doctrine\ORM\Mapping\ClassMetadataInfo;
use ReflectionProperty;

/** @psalm-import-type ScalarName from ClassMetadataInfo */
interface TypedFieldMapper
{
    /** @param array<class-string|ScalarName, class-string<Type>|string> $typedFieldMappings */
    public function __construct(array $typedFieldMappings = []);

    /**
     * Validates & completes the given field mapping based on typed property.
     *
     * @param  array{fieldName: string, type?: mixed} $mapping The field mapping to validate & complete.
     *
     * @return array{fieldName: string, enumType?: string, type?: mixed} The updated mapping.
     */
    public function validateAndCompleteTypedFieldMapping(array $mapping, ReflectionProperty $field): array;
}

[stof]

@michnovka I would not enforce the signature of the constructor. Custom mapper will legitimately have different configuration needs.
And the Configuration class should take an instance anyway so it won't care about the constructor signature.

[stof]

@michnovka don't only think about how things look like in DoctrineBundle. The doctrine/orm project is usable in other contexts as well. So the design of the API must make sense on its own.

[michnovka]

@stof ok, so can you please suggest what you meant with the mutability? I dont see how you want to share the instance between multiple configurations AND at the same time have different behavior. Nothing prevents you to make more instances of your CustomTypedFieldMapper and use them in different configuration scenarios with different default typedFieldMapping settings. So if you see some issue in this, can you please give me a concrete example so that I can understand the problem and think of a solution?

[michnovka]

@stof the problem is, that I want to keep an option to only specify the additional typed_field_mappings, whilst using the DefaultTypedFieldMapper. (or any other TypedFieldMapper). It has a real-use case where you want some logic to be implemented by the CustomTypedFieldMapper, but at the same time maybe explicitly type some mappings. I can either pass this in constructor, or in some other method.

If I pass it in constructor, the object CAN be immutable (but no way to force this with PHP, somebody can make their own TypedFieldMapper implementation which will ignore this requirement and be mutable). or

If I pass it in some additonal method, I can do this:

    /**
     * @param array<class-string|ScalarName, class-string<Type>|string> $typedFieldMappings
     */
    public function setTypedFieldMappings(array $typedFieldMappings = []): self;

Which would return a new instance always, thus imitate immutability (like DateTimeImmutable).

But I think we should not attempt to force immutability. For all we know, mutability can be desired under some scenario. If somebody wants to have 2 different configs for one TypedFieldMapper, then they should create 2 separate instances, not share them.

Like I said, we cannot enforce that any implementation of TypedFieldMapper interface wont break the requirement of immutability (by adding some modifying setter), there is no immutable keyword for the interface which might enforce such desired behavior.

For that and the fact that some scenario might benefit from mutability, I dont want to satisfy this requirement.

Thoughts?

[stof]

@michnovka to me, this shows a need for a ChainTypedFieldMapper allowing to combine multiple mappers to use them together.

But I think we should not attempt to force immutability.

I don't want to enforce immutability. I want to avoid requiring mutability. that's not the same.

[michnovka]

@stof and what about this:

<?php

declare(strict_types=1);

namespace ORM\Mapping;

use ReflectionProperty;

interface TypedFieldMapper
{
    /**
     * Validates & completes the given field mapping based on typed property.
     *
     * @param  array{fieldName: string, type?: mixed} $mapping The field mapping to validate & complete.
     *
     * @return array{fieldName: string, enumType?: string, type?: mixed} The updated mapping.
     */
    public function validateAndCompleteTypedFieldMapping(array $mapping, ReflectionProperty $field): array;
}

this does not require mutability

then the yaml config can look like

doctrine:
    orm:
        typed_field_mapper: 'default'
            arguments:
                $typedFieldMappings:
                    - App\Objects\CustomObject: App\DBAL\Types\CustomTypeFQCN
                    - App\Objects\CustomObject2: custom_type2_using_name_from_dbal

or

doctrine:
    orm:
        typed_field_mapper: App\ORM\CustomTypedFieldMapper
            arguments:
                $typedFieldMappings:
                    - App\Objects\CustomObject: App\DBAL\Types\CustomTypeFQCN
                    - App\Objects\CustomObject2: custom_type2_using_name_from_dbal
                $someSecondArgumentOfConstructor: true

or

doctrine:
    orm:
        typed_field_mapper: App\ORM\CustomTypedFieldMapper2
            arguments:
                $totallyDifferentConstructorArgument: false

The doctrine-bundle syntax is not as nice as originally proposed, but it does allow for greater flexibility. Not all TypedFieldMappers will require the $typedFieldMappings argument, so it makes no sense to force it on all (Ive realized this thanks to you @stof ).

And I can also make a class like

class ChainTypedFieldMapper implements TypedFieldMapper
{

    public function __construct(TypedFieldMapper ...$typedFieldMappers){
        // ... store them in some collection and process them all accordingly with validateAndCompleteTypedFieldMapping
    }

    // ...

which could have this support in doctrine bundle:

doctrine:
    orm:
        typed_field_mapper: 'chain'
            arguments:
                $typedFieldMappers:
                    - 'default':
                        arguments:
                            $typedFieldMappings:
                                - App\Objects\CustomObject: App\DBAL\Types\CustomTypeFQCN
                                - App\Objects\CustomObject2: custom_type2_using_name_from_dbal
                    - App\ORM\CustomTypedFieldMapper2:
                        arguments:
                            $totallyDifferentConstructorArgument: false

[stof]

you are over-thinking the DoctrineBundle configuration syntax. A custom mapper will be defined as a service seperately and will only be referenced in the config (or registered through a tag applied on it depending on the final solution). But anyway, that's a discussion to have in DoctrineBundle after we know what the ORM API is.

[michnovka]

@stof regardless of the doctrinebundle config, the other things I proposed in the above comment address your feedback completely, so do you agree with that proposal?

[michnovka]

I can no longer vote to merge this PR, since a far superior version has implemented here - #10313. We will continue discussion there