Drop Flyweight pattern

[greg0ire]

There is a growing need for types with parameters, and that pattern was
probably used out of concerns that are no longer relevant anyway.

The documentation needs to be updated, but sits in the wrong repository 🙈

EDIT: actually there is also one in this repository: https://www.doctrine-project.org/projects/doctrine-dbal/en/latest/reference/types.html#custom-mapping-types 🤔

[greg0ire]

Although this paragraph is a how-to, the one above it is as well, and the DBAL docs do not have a cookbook/how-to section. Not sure what to do.

[morozov]

It's fine to keep it here.

[derrabus]

Because this PR implements a new feature, it should target the 3.3.x branch.

[morozov]

Given that the types are no longer identified by their names, what's the point of the name?

[greg0ire]

Given that the types are no longer identified by their names, what's the point of the name?

If for some reason, you have 2 instances of EncryptedType, you will have to make EncryptedType::getName() dynamic because it's not possible to register 2 types with the same name, because a type instance is still identified by its name.

[morozov]

I still have a feeling that it isn't the next step in refactoring the type system. If there's no clear understanding of whether the type names are needed, and the names are potentially related to the types being flyweights, I'd pause here and clean up the naming.

The need to define the type name twice and declare it in the class dynamically depending on the implementation is a footgun that IMO needs to be addressed first of all.

If you believe this PR is the right next step, we can proceed.

[greg0ire]

@morozov there is no hurry for this PR, we can pause it. I noticed that there is no mention of DC2Type inside docs, this might be worth an explanation (in the sense of the website you showed me, https://documentation.divio.com). Once we have that, we can see if we can work around it, for instance by having the type registry "keys" be used instead of calling getName(). The current implementation of TypeRegistry::lookupName looks like it would just work. I didn't pay enough attention and didn't see it was implemented with array_search and an object, which I have never used like this in my life.

Consequently, there is no need to deprecate lookupName in this PR.

[morozov]

Now that the confusion about using the type name twice has been eliminated in #5208, would it make sense to retarget this patch for 4.0.x?

[greg0ire]

Yes it would, I was planning on doing it soon ™️

[morozov]

Code-wise looks good. Just a few nitpicks on documentation and tests.

[morozov]

IMO, encryption is a quite specific and sophisticated domain to be used to illustrate a concept of types. It's also not self-contain since it depends on a SecretStore which isn't declared in the example.

It's better to use a dummy example with a prefix that doesn't distract from the subject.

[greg0ire]

Ok, I tried to dumb it down to a less realistic but more simple use case.

[morozov]

"sameButDifferent" is really confusing to read. Why not just use some arbitrary keys like "type1" and "type2"? The fact that they are represented by the same object will be clear from the context and doesn't have to be expressed in the names.

[morozov]

It's fine to keep it here.