Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove final constructor for Type #6705

Open
wants to merge 3 commits into
base: 4.3.x
Choose a base branch
from

Conversation

ruudk
Copy link

@ruudk ruudk commented Jan 13, 2025

Q A
Type improvement
Fixed issues #6704

Summary

This allows people to define a constructor when creating custom Doctrine\DBAL\Types\Type implementations.

They can only be registered using Type::getTypeRegistry()->register and not with Type::addType.

An extra guard in Type::addType explains this to the user.

@greg0ire
Copy link
Member

Note: this is another take on #5528

@morozov
Copy link
Member

morozov commented Jan 13, 2025

I've never used custom types as besides schema management, they are primarily designed for the ORM. I'll defer to Alexander.

@derrabus
Copy link
Member

If we do that, we turn Type::addType() into a potential footgun. Maybe it's time to deprecate the static methods on the Type class in favor of the TypeRegistry?

@ruudk
Copy link
Author

ruudk commented Jan 14, 2025

If we do that, we turn Type::addType() into a potential footgun.

How will it be a potential footgun? The current desired behavior is guarded, so I don't see how.

Maybe it's time to deprecate the static methods on the Type class in favor of the TypeRegistry?

I agree, but that can be done independently from this PR, no?

@derrabus
Copy link
Member

If we do that, we turn Type::addType() into a potential footgun.

How will it be a potential footgun? The current desired behavior is guarded, so I don't see how.

Yes, you've done all you could to give the developer proper feedback. Yet, we're about to create a class of types that cannot be registered with Type::addType() anymore, an issue which is only discoverable at runtime.

Maybe it's time to deprecate the static methods on the Type class in favor of the TypeRegistry?

I agree, but that can be done independently from this PR, no?

Sure, but still we should talk about it. We somehow ended up with an only partially implemented feature and I want us to work towards a proper solution this time instead of adding a small hack on top to make it somewhat work.

@ruudk ruudk force-pushed the ruudk/2025/1/remove-final-type-constructor branch from e6b5c83 to 355887c Compare January 16, 2025 08:29
@ruudk
Copy link
Author

ruudk commented Jan 16, 2025

@derrabus @greg0ire You're right. Let's deprecate Type::addType immediately. Updated the PR.

Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the tests I wrote in my initial PR were not sufficient to prove it's possible to do what's in the documentation. Maybe they should be improved?

UPGRADE.md Outdated Show resolved Hide resolved
Co-authored-by: Grégoire Paris <[email protected]>
@greg0ire
Copy link
Member

The docs should be updated, they mention addType.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants