Refactor type system

[greg0ire]

Goals

• use FQCNs instead of strings to identify types
• have people implement one or more interfaces depending on their needs, instead of forcing them to use inheritance
• move as much code as possible out of the abstract type (and ideally remove it completely)

Todo

From @Majkl578 on slack:

just a quick thought of a possible approach:

1. introduce TypeInterface with required methods (or different name)
2. make types implement it
3. introduce some kind of Types enum-like empty class for implictly supported types, just like i.e. GeneratorType in ORM develop (edit: maybe this could stay in Type to lower BC implications and make Type an enum class in 3.0)
4. introduce non-static TypeRegistry with no default types hardcoded in it - this would allow different connections/platforms use different type mappings (as @Ocramius suggested in https://symfony-devs.slack.com/archives/C3FQPE6LE/p1504362019000073)
5. make TypeRegistry work with type instances instead of their class names (sounds good, but unsure about consequences)
6. migrate Type:: to Types enum, keep BC by aliasing them and deprecate in 2,7 Extract constants for built-in types from Type to Types #3356
7. migrate Type:: mechanics to TypeRegistry, keep BC by using private static TypeRegistry internally in 2.7 (some kittens may die during this process) Extract type factory and registry from Type into TypeRegistry #3354
8. In 3.0, deprecate proxy methods from Type To TypeRegistry in favor of using the TypeRegistry directly.
9. in 4.0 drop these BC hacks (so no more kittens die)

My thoughts:
Instead of introducing an ISP-violating TypeInterface rightaway, split it from the start into small interfaces, which should make canRequireSQLConversion and requiresSQLCommentHint irrelevant. Here is such an interface, please give me some feedback. Also, get rid of the methods that we do not really need like __toString or (getName)

[greg0ire]

use FQCNs instead of strings to identify types

In light of the need to have so-called parameterized types, this seems wrong. If we are going that way, the type registry should look more like a PSR-11 container (which it does already). FQCNs could still be used to identify types by default, but one should be able to have a type called Doctrine\DBAL\Types\Enum.Country.

[Ocramius]

TBH, just register My\Enum\Type\Country, or My\Enum\Type<My\Domain\Thing\Country>.

EDIT: to be clear, being able to have a class-string<DBALType> would already help some validation a lot, especially in attributes.

[greg0ire]

Oh yes, that second one sounds like a great idea, it would make it easier for static analysis tools to get make sense of the output of the type registry! So Doctrine\DBAL\Types\Enum<MyProject\Country>

[Ocramius]

Right, so:

/** @template-extends SomeEnum<Country> */
class MyCountryEnum extends SomeEnum
{
    // ...
}

therefore you would register it as MyCountryEnum::class :D

[greg0ire]

Ah sorry, I should have made it a bit more obvious that I was referring to PHP 8 enums

It would look more like this:

<?php
enum MyProject\Country
{
    // …
}

/**
 * @template E of enum-string
 */
class Doctrine\DBAL\Types\Enum
{
    /** @param E $enum */
    public function __construct(string $enum)
    {
        // ...
    }

    /** @return E */
    public function convertToPHPValue()
    {
    }
}

Then you'd be able to register Doctrine\DBAL\Types\Enum<MyProject\Country> without at any point creating a custom DBAL type that would know need to explicitly reference your enum. More on this at #4930 (comment)

[Ocramius]

IMO just subclass and provide @template-extends: reduces the problem, and you can use class-string<DBALType> directly, removing the whole custom naming problem.

I just had an exact problem around custom names today:

/** @Column(type="some_custom_thing") */

Then proceed to have "some_custom_thing" mean different things depending on how your system is bootstrapped: total mess.

Better:

/** @Column(type=MySpecificDomainType::class) */

This removes ambiguity, and allows using tools like https://github.com/qossmic/deptrac on your system, preventing usage of DBAL types unknown to the current namespace (where not allowed).

If it is static, declare it static, and use a class name. I would totally be for random strings if a PSR-11 container was used instead, but it isn't the case.

[greg0ire]

just subclass

If I have 10 enums, won't that mean 10 classes with very similar code? Won't that get old real fast?

Then proceed to have "some_custom_thing" mean different things depending on how your system is bootstrapped: total mess.

Make sure you write your memoir someday 😄

I would totally be for random strings if a PSR-11 container was used instead, but it isn't the case.

Well maybe this could be considered. We're not far from it:

dbal/src/Types/TypeRegistry.php

Lines 59 to 65 in 1879160

	/**
	* Checks if there is a type of the given name.
	*/
	public function has(string $name): bool
	{
	return isset($this->instances[$name]);
	}

dbal/src/Types/TypeRegistry.php

Lines 29 to 41 in 1879160

	/**
	* Finds a type by the given name.
	*
	* @throws Exception
	*/
	public function get(string $name): Type
	{
	if (! isset($this->instances[$name])) {
	throw Exception::unknownColumnType($name);
	}
	return $this->instances[$name];
	}

[Ocramius]

Won't that get old real fast?

No, usually just a class with no body suffices:

/** @template-extends SomeEnumType<Country> */
final class MyCountryEnumType extends SomeEnumType
{
    // literally nothing here
}

[greg0ire]

Ok, so the code resides in SomeEnumType… how does that code figure out that when it reads DE in the database, it needs to return Country::DE? Would it read it from the annotation/attribute somehow?

[Ocramius]

Good point - indeed need to configure an enum reference or such 🤔

Probably:

/** @template-extends SomeEnumType<Country> */
final class MyCountryEnumType extends SomeEnumType
{
    public function __construct() {
        parent::__construct(Country::class);
    }
}

[greg0ire]

Yes, and we're back to square one: types cannot be parameterized yet. Maybe I should open another RFC just for that.

[Ocramius]

@greg0ire that is parametrized? MyCountryEnumType::class is a class-string<SomeEnumType<T of Enum>>

[greg0ire]

No I meant SomeEnumType, but indeed MyCountryEnumType would work, and having an abstract SomeEnumType laying around would not hurt I suppose… there would be no need to register it.

[kadet1090]

This will still require us to write another (even if simple) class for every enum. That classes would be really repetitive, boring and provide no real value except from being workaround for not supporting type parametrization in DBAL / Doctrine as a whole.

Not to mention that this solution would be only really viable for cases when one parameter is required, like with enums. If for example I would want to have "encrypted" type, which can accept cipher parameter and secretName (which then would be used to obtain given secret from some kind of secure store service) parameters it'd force me to create class for every single parameter combination. And that does not seem fine for me.

[Ocramius]

That classes would be really repetitive, boring and provide no real value except from being workaround for not supporting type parametrization in DBAL / Doctrine as a whole.

The type is parametrized: you register an actual instance of it. As for repetition: it's still a degree safer than what we had before, which is a registry of strings of unknown shape => shape is now known and well defined.

And that does not seem fine for me.

If the flyweight pattern is to be perpetuated around DBAL types, then there's no solution in sight for that anyway.

[greg0ire]

I wonder how important it is to perpetuate that pattern.

[Ocramius]

Probably not much, since PHP's memory usage is not such a huge problem as per PHP 7.0 (and this stuff was designed for 5.2.x and then 5.3.x).

The breakage would be ginormous, but having the TypeRegistry as runtime instance rather than singleton would suffice.

That opens the door for people to provide their own TypeRegistry, with blackjack and decorators.

[greg0ire]

The breakage would be ginormous

@Ocramius @morozov @derrabus please have a look at #5034 for an evaluation of said breakage. You will probably have better ideas than I did.

[greg0ire]

@Ocramius it's unclear to me how having the TypeRegistry as a runtime instance would get us rid of the flyweight pattern though… lifting that restriction seems like it would just require changing Type::addType / drop Type::__construct() and change TypeRegistry::register() to not throw when a type is already registered. Or did you mean that people would implement a TypeRegistry without that restriction and inject it?

It's also unclear to me what consequences lifting the "flyweight" restriction would have… do you see any there any?

I understand that restriction as follows:

• there can only be one instance of a type
• a type cannot have a state

[filiplikavcan]

Hi, I've created a PR which would allow to automatically register all child class (enum) type names by registering their parent class (enum) type name. Then it would be possible to implement one EnumType, register it for BackedEnum type name and it would be automatically used for any backed enum. See the example implementation here: #5147

[tigitz]

@greg0ire Just to give some food for thought on this topic.

We use a specific type that deserialize json into a given object in the convertToPHPValue using symfony\serializer.

Similar to the mechanics of https://github.com/goodwix/doctrine-json-odm.

What we would like to achieve from a consumer perspective is this:

class Foo
       public function __construct(
        #[ORM\Column(type: new JsonSerializeType(Bar::class, serializationContext: [])]
        private Collection $bars,
    ) {
    }

Or even better

class Foo
       public function __construct(
        /**
        * @param Collection<Bar>
        */
        #[ORM\Column(type: new JsonSerializeType(serializationContext: [])
        private Collection $bars,
    ) {
    }

That would require to have the ReflectionProperty of Foo::$bar passed to the Type somehow so that the type itself can read the docblock and proceed to the conversion accordingly.

Not sure if it's out of the scope of what has been described here and sorry it's not the case.

[greg0ire]

I think it's in scope. For me the next step towards that would be this: #5036

[noemi-salaun]

@greg0ire
Is there any plan to map native php enumerables to mysql's enum type? And is it in the scope of this issue?
It hurts to see a column of type varchar(255) when the enum is clearly defined and doctrine has all the info to create a perfect mysql enum :/

[greg0ire]

No plan AFAIK, and I don't think it's in scope of this issue, which is about providing a more powerful type system, not more powerful types.
Regarding enum's, I personally think they should be avoided: http://komlenic.com/244/8-reasons-why-mysqls-enum-data-type-is-evil/

[noemi-salaun]

Thanks for the quick answer 😄

I see the debate. But isn't it overkill to use a varchar(255) to store values that rarely exceed 10 characters?
I can manualy set the length of my column with attribute/annotation for the ORM but I would rather just write

#[Column]
private MyEnum $myEnum;

[greg0ire]

It is overkill yes: it takes more space than necessary.