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

Evaluate TypeRegistry as a runtime instance #5034

Closed
wants to merge 1 commit into from

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Nov 27, 2021

This is not an actual pull request, it's more like a way to annotate code to better discuss #2841 , more precisely what it would take to have the TypeRegistry no longer be a singleton.

3 big changes seem to be required here:

  • injecting the type registry in the Connection
  • injecting the type registry in the Platform
  • Dealing with Table::addColumn

Possible steps for the 2 first items:

  • have a type registry in the configuration
  • use it when instantiating the driver in DriverManager
  • the driver can then instantiate the platform with the type registry
  • the driver can provide the type registry to Connection

@greg0ire greg0ire mentioned this pull request Nov 27, 2021
9 tasks
@morozov
Copy link
Member

morozov commented Nov 27, 2021

injecting the type registry in the Platform

Does the type registry need to be part of the platform state? The platform can initialize the registry with its mappings w/o having it in its state. As for binding prepared statement parameters, I believe it's the types themselves that should do the binding, not the platform. It might allow for a more type-safe API in the future.

@greg0ire
Copy link
Member Author

The platform can initialize the registry

Did you mean "the platform can initialize itself with the registry"? Because it's what it's using Type::getType() for: initializing doctrineTypeComments or doctrineTypeMapping. Or are you suggesting we move these properties and the methods acting on them to the registry?

@morozov
Copy link
Member

morozov commented Nov 27, 2021

Did you mean "the platform can initialize itself with the registry"?

No, I don't think that the platform has to deal with the type registry at all except for providing its type mapping. Things are currently complicated because there's no difference in the DBAL between the primitive types (e.g. Integer) that may or may not be implemented natively by platforms and user-defined types that cannot.

If we had a notion of primitive types, we could require platforms to provide their mapping and let some other entity provide a common API for both primitive and platform-defined types. Otherwise, we're making the platform (relatively low-level abstraction) depend on an API that defines higher-level features (user-defined types).

3 big changes seem to be required here:
- injecting the type registry in the Connection
- injecting the type registry in the Platform
- Dealing with Table::addColumn

Possible steps for the 2 first items:
- have a type registry in the configuration
- use it when instantiating the driver in DriverManager
- the driver can then instantiate the platform with the type registry
- the driver can provide the type registry to Connection
@github-actions
Copy link

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

@github-actions github-actions bot added the Stale label Apr 18, 2022
@morozov
Copy link
Member

morozov commented Apr 19, 2022

Closing as this is not an actual pull request.

@morozov morozov closed this Apr 19, 2022
@greg0ire greg0ire deleted the refine-refactoring branch April 19, 2022 20:53
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants