mirror of
https://github.com/doctrine/orm.git
synced 2026-03-23 22:42:18 +01:00
Conversion of object query parameters to their identifiers is inconsistent #5371
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @stof on GitHub (Jan 3, 2017).
The detection of whether the object is an entity uses
$this->_em->getMetadataFactory()->hasMetadataFor(ClassUtils::getClass($value)). ButhasMetadataForchecks whether the factory has loaded metadata for the class. So the behavior depends on what happened previously in the request (and can change between a dev environment and the prod as the prod is more likely to use a cache for the DQL parsing logic, which is one of the place which could trigger the loading of metadata when being executed).I suggest switching to using
! isTransient()instead (potentially keeping the check for loaded metadata first for performance reasons asisTransientdoes not check the loaded metadata to short-circuit drivers).what do you think about it ?
@Ocramius commented on GitHub (Jan 3, 2017):
I think we should modify
hasMetadataFor()so that it actually does an internal lookup, and fails silently if the requested class is not mapped. The fact thathasMetadataFor()always checks for current in-memory state is a big annoyance, and it caused a lot of bugs in the past as well.@stof commented on GitHub (Jan 3, 2017):
you mean making
hasMetadataForthe equivalent of! isTransientfor the returned value ? This would be fine with me, but it could be considered a BC break@stof commented on GitHub (Jan 3, 2017):
Btw, this would require changing the interface in doctrine/common too, because this is the documented behavior: https://github.com/doctrine/common/blob/v2.7.1/lib/Doctrine/Common/Persistence/Mapping/ClassMetadataFactory.php#L54
so I'd rather not change it now
@Ocramius commented on GitHub (Jan 3, 2017):
@stof I can find bugs for basically every usage of
hasMetadataFor, so I think making it an alias (and keeping a map of the transient classes internally, for performance) would be a massive improvement.About BC breaks, it would need a lookup (on github) of who uses that method, and for which purposes.
@Ocramius commented on GitHub (Jan 3, 2017):
@stof yeah, the interface change would indeed be a breakage, nice spotting :-\
@InputOutput commented on GitHub (Oct 23, 2018):
I think I just encountered this bug in the wild, using an Entity as parameter in a where condition in DQL without specifying the specific property to use (in this case ID). Worked fine in dev mode, broke after cache warmup in production.