Skip to content

Loading…

DDC-1041: UnitOfWork tryGetById method is always called with the rootEntityName #1631

Closed
doctrinebot opened this Issue · 10 comments

2 participants

@doctrinebot

Jira issue originally created by user dreddy:

My problem : i have a class table inheritance, with only 3 class, one abstract and two concrete.
says AbstractClass, ConcretClassA and ConcreteClassB
All three are declared with @entity

if i already have in my identity map the ConcretClassA with id = 1
when i do an entityManager->find('ConcreteClassB', 1) the identityMap returns me the ConcretClassA with id = 1
that's not correct !
it should return a null value instead

That's because the entityRepository (and all the other doctrine class) call the UnitOfWork tryGetById with the rootEntityName, which in my case is AbstractClass.
See the first line of the entityRepository's find method :

$this->*em->getUnitOfWork()->tryGetById($id, $this->*class->rootEntityName)

if i change this line to :

$this->*em->getUnitOfWork()->tryGetById($id, $this->*class->name)

the find method return the expected null value.

So, why the UnitOfWork tryGetById method is always called with the rootEntityName ?

@doctrinebot

Comment created by @beberlei:

The identity map HAS to work by root entity name.

Otherwise think of an inheritance hierachy A -> B. Then both $em->find('A', 1); and $em->find('B', 1); should return the same instance.

If this does not hold you are screwed.

Your problem is more subtle, i am not sure what the expected behavior should be here.

@doctrinebot

Comment created by dreddy:

In my sense, the client code should exactly knows the inheritance hierachy, and exactly know what it wants too, so if it asks for an 'A' instance, you can't return a 'B' instance, although A inherit from B.

@doctrinebot

Comment created by @beberlei:

I agree with you except for the last sentence part.

If A->B and you query for B with Id 1 it should treturn an A if it exists. But if B->C and A->C then find(A) should only ever return As or Cs never, Bs.

@doctrinebot

Comment created by dreddy:

I agree.
Perhaps my problem is more specific to the class table inheritance strategy with an abstract class at the top of it.
Here's my identityMap's dump (with my first comment inheritance example) at the time the problem occurs :

'AbstractClass' =>
array
1 => string 'ConcretClassA'
2 => string 'ConcretClassA'
3 => string 'ConcretClassA'
367 => string 'ConcretClassB'
368 => string 'ConcretClassB'
369 => string 'ConcretClassB'

And, at this time, in my code, i would like to know if the id 1 is a ConcretClassB. If the id 1 is a ConcretClassA, i must throw an exception.
So, how could i test if the id 1 is a ConcretClassB or not, if the find('ConcretClassB', 1) method returns me the ConcretClassA 1 ?

thx for your help

@doctrinebot

Comment created by @beberlei:

Cant you just check instanceof?

$a = $em->find('ConcretClassA', $aId);
if ($a instanceof ConcretClassB) {
   throw new Exception();
}
@doctrinebot

Comment created by dreddy:

Of course i can.
But i thought Doctrine should already have to do this check internally for us. Because we must now be very careful with the find method's returns...

Nevermind, i don't have all the uses case Doctrine have to handle with inheritance in mind; so if you say it is to the client code to check, and it is a completely normal behavior, then it's okay for me.

@doctrinebot

Comment created by @beberlei:

i will change this behavior in the future, i am just making suggestions for you to fix it now. Using instanceof will also be forwards compatible since "null instanceof Class" is always false.

In any case the level of having to be careful is the same, using null instead of an object can get you into troubles aswell.

@doctrinebot

Comment created by dreddy:

Perhaps throw a \Doctrine\ORM\EntityNotFoundException would be a better option than return a null value ?

@doctrinebot

Comment created by @beberlei:

Fixed.

@doctrinebot

Issue was closed with resolution "Fixed"

@beberlei beberlei was assigned by doctrinebot
@doctrinebot doctrinebot added this to the 2.0.2 milestone
@doctrinebot doctrinebot closed this
@doctrinebot doctrinebot added the Bug label
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.