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 internal flag from TypeRegistry #4491

Merged
merged 1 commit into from Feb 2, 2021

Conversation

ostrolucky
Copy link
Member

Q A
Type improvement
BC Break no
Fixed issues -

Summary

This class is almost 2 years old and never received BC breaking change. We need to rely on its API so we can build DI integration in DoctrineBundle

I've chosen 2.12 because since there are no changes here between 2.12 and higher, it doesn't matter, unless we intend to do BC break in this class in 2.x but not 3.x (super unlikely). But it does matter for SA analysis for people still using DBAL 2.x.

Related: #2841, doctrine/DoctrineBundle#1246

SenseException
SenseException previously approved these changes Jan 30, 2021
Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

The API that should not be considered stable is stable? I don't know about the background why it wasn't considered stable though, but it seems that prophecy didn't come true.

@morozov
Copy link
Member

morozov commented Jan 30, 2021

As far as I understand from #3354, TypeRegistry is an internal implementation detail of the Type class whose API could and still can be used to register types and it hasn't been deprecated in favor of TypeRegistry. Why do consumers need to use the alternative internal API?

If the plan to replace Type with TypeRegistry, would it make sense to do as planned?

This class is almost 2 years old and never received BC breaking change. We need to rely on its API so we can build DI integration in DoctrineBundle
@ostrolucky
Copy link
Member Author

Why do consumers need to use the alternative internal API?

To be able to register instance of the classes. Type class is doing new $className() everywhere - that's unusable with DI. So that's why we need to use alternative API, because current API is bad. People are forced to use TypeRegistry, that's the only way how to register instance. But it's marked internal for some reason, even though it's stable for quite a while.

If the plan to replace Type with TypeRegistry, would it make sense to do as planned?

I think it would, but what work and what timeframe are we talking? I thought there was some blocker or unknowns if nothing was being done about this plan further, even though pieces were put in place. If this is only about replacing Type:: calls with Type::getTypeRegistry()->, I assume that should be pretty easy, but I'm not sure that's the plan here. Anything else might be complicated if BC is preserved and I don't want to wait much longer for this.

@morozov
Copy link
Member

morozov commented Jan 31, 2021

But it's marked internal for some reason, even though it's stable for quite a while.

Internal is not an antonym of stable.

If this is only about replacing Type:: calls with Type::getTypeRegistry()->, I assume that should be pretty easy, but I'm not sure that's the plan here.

I don't think anybody but @Majkl578 knows. I'd expect the type registry to be a dependency of the connection. Otherwise, it's not much better than the current static class.

@ostrolucky ostrolucky added this to the 2.12.2 milestone Feb 2, 2021
@ostrolucky ostrolucky merged commit f198185 into 2.12.x Feb 2, 2021
@morozov morozov deleted the typeregistry-deinternalize branch February 20, 2021 01:47
@morozov morozov modified the milestones: 2.12.2, 2.13.0 Apr 8, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants