-
Notifications
You must be signed in to change notification settings - Fork 231
Enable service injection for custom field types and #[AsFieldType] attribute
#963
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: 5.6.x
Are you sure you want to change the base?
Conversation
927a0c8 to
e333c1a
Compare
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 enables service injection for custom field types by implementing a TypeRegistry system with per-DocumentManager type registries and autoconfiguration support via the #[AsFieldType] attribute. The implementation allows custom field types to be registered as services and automatically detected through attributes, while maintaining backward compatibility with the existing singleton approach.
Key Changes:
- Adds
type_registryconfiguration option to enable per-DocumentManager type registries - Implements service-based type registration through the
#[AsFieldType]attribute - Introduces
LazyTypeRegistryfor lazy-loading types from a service locator
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Attribute/AsFieldType.php | New attribute class for marking MongoDB ODM field types with autoconfiguration support |
| src/Types/LazyTypeRegistry.php | Lazy-loading type registry that extends TypeRegistry and uses Symfony's ServiceLocator |
| src/DependencyInjection/Compiler/TypeRegistryPass.php | Compiler pass that processes tagged services and builds per-manager type registries |
| src/DependencyInjection/Configuration.php | Adds type_registry boolean option and supports service-based type definitions |
| src/DependencyInjection/DoctrineMongoDBExtension.php | Registers type registry services and handles custom type configuration |
| src/ManagerConfigurator.php | Updates type loading to support both TypeRegistry and legacy Type::addType() approaches |
| src/DoctrineMongoDBBundle.php | Registers TypeRegistryPass compiler pass and adds proxy autoloader check |
| docs/cookbook/field_type.rst | New documentation for registering custom field types with examples |
| docs/config.rst | Updated configuration documentation with service-based type examples |
| config/schema/mongodb-1.0.xsd | Schema updates to support optional class attribute and new service attribute |
| tests/* | Test fixtures and assertions for the new type registry functionality |
| phpstan-baseline.neon | Updated baseline for new type-related PHPStan issues |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e333c1a to
874adfa
Compare
| ->end() | ||
| ->booleanNode('type_registry') | ||
| ->defaultFalse() | ||
| ->info('If true, create a distinct TypeRegistry for each DocumentManager and inject services tagged with "doctrine_mongodb.field_type". If false, use a shared TypeRegistry for all DocumentManagers.') |
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.
| ->info('If true, create a distinct TypeRegistry for each DocumentManager and inject services tagged with "doctrine_mongodb.field_type". If false, use a shared TypeRegistry for all DocumentManagers.') | |
| ->info('If true, create a distinct TypeRegistry for each DocumentManager and inject services tagged with "doctrine_mongodb.field_type" in them. If false, use a shared TypeRegistry for all DocumentManagers.') |
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.
So when this is set to false, there is still a TypeRegistry? This is confusing…
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.
There is always a TypeRegistry instance, but in one case it's the global shared one (for BC) and when the option is enabled it's 1 instance per document manager.
Do you find a better name for this option?
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.
Maybe global_type_registry? Or shared_type_registry?
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.
That was my initial idea, but I would like this option to be false by default. So, something like types_per_manager.
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 think you mean true by default, and I thought about this too, so I thought about specific_type_registry or per_dm_type_registry, none of which sound very appealing. If maybe global type registries are going to be deprecated, then I wouldn't sweat it and accept this situation. It's IMO better than an ambiguous setting name.
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.
If I ask Claude, It comes up with:
isolated_type_registriesscoped_type_registryseparate_type_registriestype_registry_scope(enum-style)enable_per_manager_typestype_registry_isolation
I think only scoped_type_registry is appealing. What do you think?
|
|
||
| return $typeConfig; | ||
| }, $config['types'] ?? []); | ||
| if (! $config['type_registry']) { |
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.
Is there a reason to use negative logic here?
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 started with a flag meaning the opposite, that's why it's negative here and the description is confusing. I'll fix that.
874adfa to
5a1509f
Compare
…o the Configuration
| }) | ||
| ->end() | ||
| ->end() | ||
| ->booleanNode('scoped_type_registry') |
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.
Unless we already have a distinct use case for scoped registries, I'd prefer not adding another config setting for this. Using multiple document managers is already dodgy enough, I wouldn't want to add extra complexity.
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.
This is mainly a backward compatibility issue. Currently, people can add types using the static Type::register() anywhere is their code.
Without this option. This option will be deprecated in the next major version. We will always have 1 instance of TypeRegistry per DocumentManager.
But, instead I can detect when services are used as type to enable the feature.
|
|
||
| public function has(string $name): bool | ||
| { | ||
| return $this->locator->has($name) || parent::has($name); |
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 assume there's no significant performance difference between the two (as both are essentially maps connecting a string to an object), so the order of operations does not matter here.
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.
Indeed, default types are a lot more used than the custom ones.
| return $this->locator->has($name) || parent::has($name); | |
| return parent::has($name) || $this->locator->has($name); |
| if ($this->locator->has($name)) { | ||
| return $this->locator->get($name); | ||
| } |
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.
Similar to the comment above, I assume there's no performance issue with checking for a service in the locator or repeatedly fetching the same service from it.
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.
This is the feature: we can override the default types this way.
| private function registerAutoloader(DocumentManager $documentManager): void | ||
| { | ||
| $configuration = $documentManager->getConfiguration(); | ||
| if ($configuration->isLazyGhostObjectEnabled() || $configuration->isNativeLazyObjectEnabled()) { |
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 this be an optimisation or bug fix in a separate PR? No strong feelings, it just seems out of place in this PR.
| #[AsFieldType(Money::class)] | ||
| final class MoneyType extends Type | ||
| { | ||
| // This trait provides a default closureToPHP() used to generate data hydratation classes |
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.
| // This trait provides a default closureToPHP() used to generate data hydratation classes | |
| // This trait provides a default closureToPHP() used to generate data hydration classes |
|
|
||
| private const TAG = 'doctrine_mongodb.odm.field_type'; | ||
|
|
||
| private const ALL = '\0'; |
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'll note that this is not actually a null byte, but rather the literal string \0. I would hope people don't actually use that.
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.
Null byte is better, isn't it?
| private const ALL = '\0'; | |
| private const ALL = "\0"; |
But this can be anything that is not a valid document manager name.
| ->children() | ||
| ->scalarNode('class')->isRequired()->end() | ||
| ->scalarNode('class')->end() | ||
| ->scalarNode('service')->end() |
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 will remove this new configuration. As a service needs to be already registered, adding a tag to the service is the way to register it. And the new #[AsFieldType] attribute makes it easy.
Bundle implementation of the
TypeRegistryfrom doctrine/mongodb-odm#2966In order to preserve BC in case people manipulate types with the methods such as
Type::add(), we use the singletonTypeRegistry::getSharedInstance().The new option
doctrine_mongodb.type_registryenables a single instance ofTypeRegistryperDocumentManager, and the autoconfiguration of services with the#[AsFieldType]attribute.This feature is very powerful, combined with the detection of the Field type from the property type.