Skip to content
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

Remove Proxy from EntityManagerInterface contract #9001

Merged
merged 1 commit into from
Sep 11, 2021

Conversation

derrabus
Copy link
Member

After merging #8922 by @simPod, I had a deeper look that the implementation to see if we can fix some of the newly baselined errors. I'd like to partially revert the change now. Sorry for the confusion.

#8922 added an intersection with the Proxy interface to the return type of EntityManagerInterface::getReference() and getPartialReference(). Here's why I now think that this is problematic:

  1. While our implementation EntityManager might return such an object, we cannot guarantee that it will. If you look at the following code snippet, you can see that the actual entity is returned instead of a reference, if the entity has already been registered with the UoW. In that case, the returned entity might not be a proxy object.

    $entity = $this->unitOfWork->tryGetById($sortedId, $class->rootEntityName);
    // Check identity map first, if its already in there just return it.
    if ($entity !== false) {
    return $entity instanceof $class->name ? $entity : null;
    }

    If we kept the Proxy interface in the return type, the correct type would be (T&Proxy)|T|null.

  2. The Proxy interface is somewhat an implementation detail of our EntityManager implementation that should not be leaked to the interface. I believe, the ORM should be able to move to a different proxy library without violating the contract of EntityManagerInterface.

@derrabus derrabus added this to the 2.9.6 milestone Sep 11, 2021
@greg0ire greg0ire added the Bug label Sep 11, 2021
@greg0ire greg0ire merged commit d1cd804 into doctrine:2.9.x Sep 11, 2021
@greg0ire
Copy link
Member

Thanks @derrabus !

@derrabus derrabus deleted the sa/em-get-reference branch September 11, 2021 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants