Fix no-op for initializeObject() and isUninitializedObject() when object is not a mapped entity - #12617
Conversation
73d117d to
ea48587
Compare
| $reflection->initializeLazyObject($obj); | ||
| } catch (PersistenceMappingException) { | ||
| // No-op for non-Doctrine entities according to the ObjectManager::initializeObject() interface documentation. | ||
| } |
There was a problem hiding this comment.
A try with an empty catch is an anti pattern I think. Have you considered using AbstractClassMetadataFactory::isTransient() instead?
There was a problem hiding this comment.
Good point, thanks. Switched both methods to an isTransient() guard and dropped the try/catch (5c03f84). The MappingDriverChain test cases stay in GH12172Test, since that driver returns true from isTransient() for a class outside every registered namespace — which is the Symfony case that triggered this.
One note: AbstractClassMetadataFactory::isTransient() always goes to the driver, so for an already-loaded entity class this is a ReflectionClass + attribute read per call, on the computeAssociationChanges() path. Probably fine, but if you'd rather avoid it I can add a hasMetadataFor() fast path — or that could be a small follow-up in doctrine/persistence (check loadedMetadata before asking the driver).
There was a problem hiding this comment.
I think a follow-up in doctrine/persistence would make sense. Actually, you could send it right now, it's orthogonal to this PR, isn't it?
There was a problem hiding this comment.
I tried the bare isTransient() first and it broke ValueObjectsTest::testPartialDqlOnEmbeddedObjectsField: an embeddable loaded through a partial query is a lazy ghost, but isTransient() reports it as transient, since by contract it is only false for entities and mapped superclasses. So isUninitializedObject() returned false for an uninitialized embeddable ghost.
865fbf2 keeps isTransient() but checks hasMetadataFor() first. If this manager has already loaded metadata for the class, that covers embeddables (the ghost could not exist otherwise) and makes the common case an array lookup, which also takes care of the reflection cost. isTransient() remains as the fallback for an entity class this manager has not loaded yet.
I also drafted the doctrine/persistence change I floated earlier and then dropped it. Short-circuiting isTransient() on loadedMetadata would make it return false for embeddables (and ODM embedded documents) once loaded, which contradicts its documented contract and makes the answer depend on load order. The abstract factory cannot tell a mapped superclass from an embeddable via isEntity(), so I do not see a safe generic version. Sorry for the detour.
ea48587 to
5c03f84
Compare
…t is not a mapped entity
5c03f84 to
865fbf2
Compare
|
Thanks @SherinBloemendaal ! |
Summary
Supersedes #12173, which was auto-closed as stale and can no longer be reopened because its base branch
3.6.xhas been deleted. Same change, rebased onto3.7.x.Fix
initializeObject()andisUninitializedObject()to be a no-op for non-managed objects when native lazy objects are enabled.With
isNativeLazyObjectsEnabled(), both methods callgetClassMetadata($obj::class)unconditionally, so passing a non-entity (a DTO,stdClass, …) throws aMappingExceptioninstead of the no-op theObjectManagerinterface documents. This breaks e.g. Symfony'sUniqueEntityValidatoron DTOs withentityClass, which calls$em->initializeObject()on association field values.This fix wraps the
getClassMetadata()calls in try/catch blocks to handle non-entity objects gracefully while preserving functionality for valid Doctrine entities (including detached ones afterclear()).I went with the try/catch approach since
isInIdentityMap()also internally callsgetClassMetadata(), but I'm open to better solutions if there are any.Changes:
UnitOfWork::initializeObject(): Add try/catch around metadata accessUnitOfWork::isUninitializedObject(): Add try/catch around metadata accessGH12172Test: functional test covering both methods for non-entities and for real lazy entities