Deprecate Type::getName()

[greg0ire]

Q	A
Type	improvement
BC Break	no
Fixed issues	n/a

Summary

The consequence of this is that it no longer need to be implemented in
child classes. This means users no longer need to make sure the name
they use in getName() is the same they use when registering the type.

Type already has a dependency on TypeRegistry, which should make this
change OK and should not make it harder to get rid of getName() entirely
in the future.

Apart from rare scenarios where one would use a type without registering
it, that implementation should work reliably. That scenario happens
quite frequently in tests, which is why there were some changes in the
test suite.

This should unlock #5036 without requiring we get rid of getName() entirely, which would be a bigger endeavor.

Note: the ORM has docs on types: https://www.doctrine-project.org/projects/doctrine-orm/en/2.10/cookbook/custom-mapping-types.html#custom-mapping-types

They will probably need to be replaced with a link to the DBAL docs otherwise things are going to get confusing, especially if there are more consequential refactorings in the type system.

[greg0ire]

The only possible BC-break in this PR is removing getName() from existing types. It will not work if the type is not registered, or is registered with a different name (which is not supported). If we think this is too risky, it can be reverted without loosing the benefits of having a generic getName() implementation.

Obviously, I think it should be fine.

[morozov]

I'd revert it. This code itself doesn't cause any problems to be removed in a minor release.

[morozov]

The introduction of a dependency of a value on a registry doesn't look right, even temporarily. If we want to deprecate and eventually get rid of Type::getName(), we should do this.

In its current state, this pull request removes some code at a cost of introducing a risk of BC break.

Instead, what if we should identify the scenarios that rely on Type::getName() and deprecate them. For instance, deprecate registering a type with a name different than the value returned by Type::getName().

[greg0ire]

The introduction of a dependency of a value on a registry doesn't look right, even temporarily. If we want to deprecate and eventually get rid of Type::getName(), we should do this.

I don't understand this paragraph. What value are you referring to? Did you mean to write "we should not do this."?

If we were to drop Type::getName(), we would probably replace calls to it by Type::getTypeRegistry()->lookupName($type), wouldn't we?

[morozov]

Sorry for the confusion. I was referring to the return value of Type::getName().

Did you mean to write "we should not do this."?

I meant that if we want to deprecate and eventually get rid of Type::getName(), we should do that (provide the upgrade path and deprecate it).

If we were to drop Type::getName(), we would probably replace calls to it by Type::getTypeRegistry()->lookupName($type), wouldn't we?

If we have a type registered under a name, we can use the type name in the API and look up the type by name. Why do we need to do the reverse lookup? What does it even mean given that the types will be no longer flyweights?

[greg0ire]

If we have a type registered under a name, we can use the type name in the API and look up the type by name. Why do we need to do the reverse lookup?

Well for instance here:

dbal/src/Platforms/AbstractPlatform.php

Lines 445 to 454 in ebd986b

	public function isCommentedDoctrineType(Type $doctrineType)
	{
	if ($this->doctrineTypeComments === null) {
	$this->initializeCommentedDoctrineTypes();
	}
	assert(is_array($this->doctrineTypeComments));
	return in_array($doctrineType->getName(), $this->doctrineTypeComments, true);
	}

we have access to the type instance, but not to its name in the registry, so we would have to change the signature of this method to also provide the name, which will be a BC-break, and we will also need to change the calling methods if it does not have access to the name, etc. etc.

Or maybe we should use spl_object_id to identify the Type instance in that particular scenario, I don't know. I guess we would have to check for each scenario if we have an alternate why of fulfilling it.

[morozov]

Why can't we just call $type->requiresSQLCommentHint($this)?

[morozov]

This is a step backward, IMO. Testing a type shouldn't require the usage of a type registry.