-
-
Notifications
You must be signed in to change notification settings - Fork 514
Create a TypeRegistry and enable injection of Type instances
#2966
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
base: 2.16.x
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a new TypeRegistry class to manage type instances and enables dependency injection of Type instances, deprecating the static methods on the Type class. This architectural change improves testability and allows for better isolation of type registries per DocumentManager.
Key changes include:
- Creating
TypeRegistryto replace staticTypemethods - Injecting
TypeRegistryintoDocumentManagerandClassMetadata - Deprecating all static methods on the
Typeclass
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Types/TypeRegistry.php | New class implementing type registration and retrieval |
| src/Types/Type.php | Static methods deprecated in favor of TypeRegistry |
| src/DocumentManager.php | Integration of TypeRegistry with initialization logic |
| src/Mapping/ClassMetadata.php | TypeRegistry injection and usage for type operations |
| src/UnitOfWork.php | Updated to use DocumentManager's TypeRegistry |
| src/Persisters/*.php | Updated to use DocumentManager's TypeRegistry |
| src/Hydrator/HydratorFactory.php | Updated to use DocumentManager's TypeRegistry |
| src/Aggregation/*.php | Updated to use DocumentManager's TypeRegistry |
| tests/Tests/Types/TypeRegistryTest.php | New test coverage for TypeRegistry |
| tests/Tests/Types/TypeTest.php | Updated tests for deprecated Type methods |
| tests/Tests/CaptureDeprecationMessages.php | New trait for capturing deprecation messages in tests |
| tests/**/*Test.php | Test files updated to use TypeRegistry |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0375c36 to
232d674
Compare
232d674 to
afd974b
Compare
alcaeus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial review done. I like the idea of a type registry, but it once again shows that we really need to clean up the dependency mess that we currently have.
The only concern I have is exposing the convertToDatabase method from DocumentPersister. I think we should mark it as @internal to ensure people don't treat it as part of the public API (unless the whole persister is already marked as such).
Enable having a distinct list of types for each instance of DocumentManager in 3.0 Enable injecting type instances constructed with parameters, which is necessary for generic embedded object transformations.
afd974b to
bba03d6
Compare
| /** | ||
| * The TypeRegistry is responsible for managing the mapping types supported. | ||
| */ | ||
| class TypeRegistry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be final?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I would have to create an interface since it's not internal. Which is more strict for BC than a regular class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I would have to create an interface since it's not internal.
Do you? Is every type supposed to be replacable/mockable? I think it's OK to marks something as final and postpone the decision of making it extensible vs having an interface to when a need for extensibility actually emerges. For something that looks like an map with extra methods, I'm not convinced extensibility is needed.
Feel free to ignore this if you have a hard rule about this in this project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with making it final. We don't need to mock it for internal tests (yet), and people should definitely be mocking it for their own tests considering it's a registry they can create and inject as they see fit. We can always make the class non-final in a future minor release if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've already a use-case for extending the TypeRegistry: in the bundle I add lazy-loading of type instances from the container: https://github.com/doctrine/DoctrineMongoDBBundle/pull/963/changes#diff-b090a1e775d0625ed9280a9140328e118d6ad52fafb71464bf7246ac937e8442
|
Some commits look like they should be squashed together. |
Yes, my plan was to have 2 separate commits to ease with rebase and merges, but the implementation changed a lot and that will not help anymore. I'll squash-merge the PR as we usually do on the MongoDB-ODM project. |
MongoDB has 1 ms of precision, we use UTCDateTime object to normalize the timezone and skip the microseconds
5391b88 to
271963e
Compare
alcaeus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM. Do you want to add the deprecations to the UPGRADE document now or do we do that in the 3.0.x branch when we remove deprecated functionality?
Summary
TypeRegistrythat replace the static method of theTypeclass. The name is the same as DBALTypeRegistry, with the same methods (exceptoverridebecauseregistercreates or replace the type, "upsert" is a MongoDB specificity 😉)TypeclassTypeRegistryinto theConfigurationand add the public methodgetTypeRegistry(). This method is used byUnitOfWork,DocumentPersisterand the aggregation builder.TypeRegistryintoClassMetadatainstances.The Bundle PR: doctrine/DoctrineMongoDBBundle#963