Add TypeRegistry to Configuration and use it in all internal type resolution - #7342
Add TypeRegistry to Configuration and use it in all internal type resolution#7342GromNaN wants to merge 20 commits into
TypeRegistry to Configuration and use it in all internal type resolution#7342Conversation
7ca5478 to
b5bd71c
Compare
9e8eef6 to
adbd44c
Compare
|
As discussed during the SymfonyLive, it would be great to support lazy-loading of type instances injected in the TypeRegistry, to reduce the cost of instantiation the connection service (especially when types have dependencies, which might lead to instantiating a bigger object graph). As far as DoctrineBundle is concerned, the easier way would probably involve injecting a PSR-12 ContainerInterface (with ids being the type names) and a list of type names (or a map of type names to ids in the container to allow more flexibility about those ids). This would allow us to use the Symfony ServiceLocator which performs such lazy-loading (it would of course mean that the constructor should not retrieve type instances, as that would defeat the lazy-loading). |
|
Thanks for the reminder @stof! I implemented both approaches:
Both paths converge into a unified |
d87fe57 to
0b6f833
Compare
|
I find it weird to pass multiple ContainerInterface. A single container can hold all the types (under different indexes). |
|
@GromNaN do you plan to change the way the case of a ContainerInterface gets supported ? See my previous comment that was not answered from April. |
0b6f833 to
b9a6f7a
Compare
…instance-based lookups All internal type resolution (Connection, Statement, AbstractPlatform, SchemaManagers, MetadataProviders) now goes through Configuration::getTypeRegistry() instead of the global Type::getType() / Type::hasType() / Type::getTypesMap() static methods. Table receives an optional Configuration so addColumn() uses the instance registry when available, falling back to Type::getType() for user code without a Configuration. ColumnEditor::setTypeName() similarly falls back to Type::getType() for user code; internal callers (MetadataProviders) now use setType() with the configuration registry.
…in types by default The constant is now TypeRegistry::BUILTIN_TYPES_MAP (public). Any new TypeRegistry() is pre-populated with all built-in type instances; additional types passed to the constructor are registered on top and may override built-ins. Type::getTypeRegistry() is simplified to new TypeRegistry() with no arguments.
SchemaConfig now carries a TypeRegistry that is populated by AbstractSchemaManager::createSchemaConfig() from the connection configuration. Schema::createTable() forwards it to each Table so that Table::addColumn() resolves types from the per-connection TypeRegistry instead of falling back to the global static registry. Table's constructor parameter is changed from ?Configuration to ?TypeRegistry directly, since Configuration was only needed to reach its TypeRegistry. A TODO comment is added to ColumnDiff::hasTypeChanged() noting that the current class-based comparison is insufficient now that types are services: same-class aliases (json / json_object) produce false negatives, and distinct service instances of the same class would not be detected. The fix (identity comparison) is left for a follow-up.
- Accept ServiceProviderInterface<Type> or array<string, Type|ContainerInterface> as constructor argument; built-in types are now lazy too - Store unresolved types as class-string (built-ins) or ContainerInterface in a $factories array; instances are created on first get() and cached - Replace $instancesReverseIndex spl_object_id map with WeakMap<Type, string> for O(1) reverse lookup: 84 ns/op vs 86 ns/op (spl_id) vs 171 ns/op (array_search) for 30 registered types, while also being GC-friendly
b9a6f7a to
801c8ea
Compare
Replace the array<string, ContainerInterface> form with a single ContainerInterface plus an explicit map of type names to service IDs, as requested in review. This drops the only Symfony reference from src/, so container support no longer implies symfony/service-contracts. Requiring the map also removes a footgun: deriving type names from ServiceProviderInterface::getProvidedServices() silently assumed that the locator keys were type names, so keying a locator by service ID would have registered service IDs as type names. While here, two simplifications to the registry internals: - Built-in types are read straight from the BUILTIN_TYPES_MAP constant rather than copied into a per-instance $factories array. The constant is immutable and shared, and lookup order already gives user types and container types precedence. - Drop the $instancesReverseIndex WeakMap. It was an exact inverse of $instances, kept in sync by hand across five call sites, and its weak semantics never applied because $instances holds strong references. lookupName() now uses array_search(), which has no internal callers. Together these cut registry construction from ~0.67us to ~0.06us and memory from ~1557 to ~117 bytes per instance, which matters because there is now one registry per connection. Also restore UnknownColumnType for unknown names in get(); TypeNotFound reads "Type to be overwritten ... does not exist" and belongs to override() only.
801c8ea to
5ee4350
Compare
`Column::getType()` returns a `Type` instance. In DBAL 4 the canonical identifier of a type is its name, not its class or instance: `Type::getName()` was removed in favor of `TypeRegistry::lookupName()`, and consumers that only need the name (ORM `DatabaseDriver`, RSM, schema comparison, reverse engineering, dumps to cache) end up doing a useless instance -> name round-trip via the global static registry. Expose the type name directly on `Column`: - `Column::setTypeName(string): self` and `Column::getTypeName(): string` (throws `TypesException` if the name cannot be resolved). `_typeName` is the source of truth. - `Column::setType(Type)` deprecated (still populates `_typeName` eagerly so unregistered types now fail early instead of silently). - `Column::getType()` deprecated. - `AbstractPlatform::getType(Column)` protected helper introduced as the single call site for `Type::getType()`, so a future `TypeRegistry` injection has one hook. Migrated `OraclePlatform`, `PostgreSQLPlatform`, `DB2Platform` and `PostgreSQLSchemaManager` off `Column::getType()`. - `ColumnDiff::hasTypeChanged()` now compares type *names* instead of instance classes. - Tests updated to construct columns via `setTypeName()`; two comparator tests that only made sense under class-based identity (`clone Type`, `overrideType`) collapsed into a single name-based equivalence test.
Column now stores a type name rather than a Type instance, which overlaps heavily with the per-connection TypeRegistry work. Resolved in favour of type names throughout, and dropped what that makes redundant: - Schema managers pass the type name straight to Column instead of resolving an instance first, so AbstractSchemaManager::getType() is now only needed where a real Type is required (PostgreSQL's JsonType check). - ColumnDiff compares type names. This supersedes the TODO on this branch about comparing by class: names distinguish json from json_object even though both map to JsonType, without depending on two schemas resolving from the same registry. - Table no longer needs a TypeRegistry, since addColumn() hands the name to Column without resolving it. Removed the constructor argument along with SchemaConfig::get/setTypeRegistry() and the propagation through Schema and AbstractSchemaManager, all added earlier on this branch. Also routed AbstractPlatform::getType(), added by the merged branch, through the connection's registry rather than the static Type::getType().
Covers Configuration::get/setTypeRegistry(), the fallback to the global singleton for connections that do not set one, and the deliberate isolation from Type::addType(). Also notes two things that are easy to trip over: new TypeRegistry() is now pre-populated with the built-in types, and mocking Configuration requires stubbing getTypeRegistry() because TypeRegistry is final.
|
I reworked the container injection following @stof's review.
$registry = new TypeRegistry($container, ['money' => 'app.dbal_type.money']);The The container is never queried during construction, nor by This does not add complexity on the bundle side. doctrine/DoctrineBundle#2221 is updated accordingly: the I also merged #7490 into this branch, to give a complete picture of where this is heading. Storing a type name on I recommend merging #7490 first, then rebasing this PR on top of it. |
Configuration now exposes get/setTypeProvider() typed against the new Doctrine\DBAL\Types\TypeProvider instead of the final TypeRegistry, so the type source can be extended or stubbed. The ORM testsuite previously had to instantiate a real registry and touch unrelated tests because the final class could not be doubled. The interface extends PSR-11 ContainerInterface and Traversable, since a type registry is a container of types that callers may also enumerate. get() is redeclared to narrow the return type to Type; without that, every call site would degrade to mixed. register() and override() stay off the interface, so Type::getTypeRegistry() keeps returning the concrete class for Type::addType(). Because interface inheritance is resolved eagerly, psr/container moves back to a hard requirement. getMap() is replaced by iteration: TypeRegistry implements IteratorAggregate with a generator that yields from each source in turn rather than merging them, so iteration allocates nothing extra and stopping early leaves the remaining types uninstantiated. Also from the review: - Drop the unset() in get()'s finally. It was redundant, since $instances is checked first and shadows the service ID, and being in finally it also ran on failure: a transient container error permanently dropped the type, so has() flipped to false and a retry reported an unknown type instead of retrying. - Deprecate TypeRegistry::lookupName() and Type::lookupName(). They cannot be removed yet because the deprecated Column::setType(), ColumnEditor::setType() and ORM's TypedExpression branch still need to derive a name from an instance. Both go in 5.0, along with the one-instance-one-name restriction. - Add a runtime deprecation to Column::getType(). It uses triggerIfCalledFromOutside because toArray() calls it internally when $skipType is false, and that path already triggers its own deprecation.
b66ae60 to
a158b60
Compare
8468a1d to
898befe
Compare
Extending PSR-11 made psr/container a hard requirement, because interface inheritance is resolved eagerly. That is not worth it yet: nothing in DBAL consumes a TypeProvider as a container, and the interface can still be widened later without breaking implementors. psr/container therefore returns to require-dev. It stays a soft dependency: TypeRegistry still accepts a container and catches ContainerExceptionInterface, but those are parameter and catch positions, which PHP only resolves when a container is actually passed. Verified by running the array-based path with an autoloader that fails on any Psr\Container\* lookup.
898befe to
783d53c
Compare
Requested in review on doctrine#7342: the method was only marked @deprecated in its docblock, so callers got no signal at runtime. Uses triggerIfCalledFromOutside rather than trigger, because toArray() still calls getType() internally when $skipType is false, and that path already triggers its own deprecation. Verified that toArray() as the first call in a process reports exactly one deprecation, its own. Also moves the upgrade note from the 4.4 section to 4.5, where this deprecation actually lands, next to the related Column mutator notes. Its @deprecated docblock no longer points at Configuration::getTypeRegistry(), which does not exist on 4.5.x.
It was inserted directly after the "Upgrade to 4.4" heading, so it documented a 4.5 deprecation under 4.4. Placed next to the related Column mutator notes. Same fix as on the doctrine#7490 branch, where the note originates.
The message told users to register the type with Type::addType(), which does not help a connection that has its own TypeProvider: such a connection does not see globally registered types, so following the advice led nowhere. Also normalises Foo#bar() to Foo::bar(); that notation appeared nowhere else in src/.
They all operate on the process-wide registry, which behaves unexpectedly once a connection has its own type provider: a type registered with Type::addType() is invisible to that connection, and Type::getType() resolves against the global registry rather than the connection's. Nothing signalled that, so the failure surfaced later as an unrelated UnknownColumnType. All seven now carry an @deprecated docblock and a runtime trigger. getTypeRegistry() and getType() use triggerIfCalledFromOutside, because Configuration::getTypeProvider(), AbstractPlatform and the deprecated Column::getType() call them internally; that keeps the supported default path silent and avoids reporting one user call twice. Verified that each static fires exactly once, that Column::getType() still reports once rather than twice, and that a plain connection stays silent through insert and schema introspection. UnknownColumnType no longer recommends Type::addType(), which this change deprecates and which would not have fixed the error for a connection with its own provider. DBAL 5 will have no static type provider.
|
Deprecated all the static They keep working, and the supported default path stays silent. DBAL 5 will have no static type provider. |
Continues #6705. The existing
TypeRegistryclass already supports scoped, instance-basedtype management; this PR wires it into the rest of DBAL so it is actually used.
Companion PRs: doctrine/orm#12421 and doctrine/DoctrineBundle#2221.
This is already being done in the same way for Doctrine MongoDB ODM: doctrine/mongodb-odm#2966
#7490 is merged into this branch so the whole direction is visible in one place. I recommend
merging #7490 first, then rebasing this PR on top of it.
Motivation
Two goals:
Dependency injection into type instances. Custom types sometimes need access to
services (e.g. a serializer, an encryption service). With the global static registry this is impossible
without static state. A per-connection type provider can hold fully-constructed type
instances.
Prevent global side-effects.
Type::addType()andType::overrideType()mutate aprocess-wide singleton, so a test or a bundle changing a type affects every connection.
A scoped provider isolates those changes.
What changed
TypeProviderNew interface, and the type
Configurationnow expresses:It extends
Traversable, since callers may also want to enumerate the available types.register()andoverride()are deliberately absent, soType::getTypeRegistry()keepsreturning the concrete
TypeRegistrythatType::addType()needs.Extending PSR-11
ContainerInterfacewas considered and left out for now, since it would makepsr/containera hard requirement. It can still be added later.TypeRegistrynew TypeRegistry()already contains all of them. They areread straight from a class constant rather than copied per instance, so construction is cheap.
Custom types passed to the constructor are layered on top and may override built-ins by name.
The container is never queried during construction, nor by
has():IteratorAggregatewith a generator, replacing the@internal getMap(). Stoppingearly no longer instantiates every remaining type.
lookupName()is deprecated. It cannot go yet, because the deprecatedColumn::setType(),ColumnEditor::setType()and ORM'sTypedExpressionstill need instance-to-name. It is removedin 5.0, together with the restriction that an instance may only be registered under one name.
ConfigurationgetTypeProvider(): TypeProviderreturns the provider for this connection, lazily defaultingto the global registry (
Type::getTypeRegistry()).setTypeProvider(TypeProvider $provider): selfinjects a provider scoped to this connection.Internal type resolution
All of the following now resolve types through
$configuration->getTypeProvider()insteadof the static
Type::*methods:ConnectionconvertToDatabaseValue(),convertToPHPValue(),getBindingInfo()StatementAbstractPlatforminitializeAllDoctrineTypeMappings(),registerDoctrineTypeMapping(),getType()*SchemaManager(×6)_getPortableTableColumnDefinition()*MetadataProvider(×6)AbstractPlatformreceives itsConfigurationvia a newsetConfiguration()methodcalled by
Connection::getDatabasePlatform()after platform creation.Because #7490 makes
Columnstore a type name rather than an instance, the schema managers handthe name straight to
Columnand no longer resolve aTypefirst.Table,SchemaandSchemaConfigtherefore need no provider at all.The static
Type::*methods are deprecatedThey all operate on the process-wide registry, which behaves unexpectedly as soon as a connection
has its own type provider: a type registered with
Type::addType()is invisible to thatconnection, and
Type::getType()resolves against the global registry rather than theconnection's. Nothing signalled that today, so the mistake surfaced later as an apparently
unrelated
UnknownColumnType.getTypeRegistry(),getType(),addType(),hasType(),overrideType(),getTypesMap()andlookupName()therefore carry an@deprecateddocblock and a runtimedeprecation. They keep working and still delegate to the global registry, so nothing breaks.
getTypeRegistry()andgetType()usetriggerIfCalledFromOutside, becauseConfiguration::getTypeProvider(),AbstractPlatformand the deprecatedColumn::getType()call them internally: the supported default path stays silent, and a single user call is not
reported twice.
DBAL 5 will have no static type provider.
Trade-offs
Iterating a provider resolves every type.
AbstractPlatform::initializeAllDoctrineTypeMappings()does exactly that, so schema introspection instantiates all types. Laziness holds for query paths.
Custom types registered via
Type::addType()are invisible to connections that use acustom provider. This is intentional: it is the isolation the feature provides.
Users who set a provider are responsible for registering all types they need in it.
The global singleton is preserved.
Type::getTypeRegistry()still returns theprocess-wide registry. Connections that do not call
setTypeProvider()continue to behaveexactly as before.