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

Better explain the EntityManager::getReference() method #10797

Closed
mpdude opened this issue Jun 25, 2023 · 3 comments · Fixed by #10800
Closed

Better explain the EntityManager::getReference() method #10797

mpdude opened this issue Jun 25, 2023 · 3 comments · Fixed by #10800

Comments

@mpdude
Copy link
Contributor

mpdude commented Jun 25, 2023

As one takeaway from #3037 (comment) and #843, we should look into better explaining the EntityManager::getReference() method, it’s semantics, caveats and potential responsibilities placed on the user.

@mpdude
Copy link
Contributor Author

mpdude commented Jun 25, 2023

@flack what would have helped you, what would you like to have known before, what warning signs do you deem appropriate?

@flack
Copy link
Contributor

flack commented Jun 25, 2023

@mpdude I was about to open a ticket about that, too (was thinking something along the lines of "getReference() can corrupt UnitOfWork internal state").

A stern warning in the documentation can't hurt, but realistically, it won't prevent many errors either. I suppose there are no sanity checks in this method because of performance, but it's a bit absurd that I can have an entity with an autoincrement ID field, and then get a reference for id -1 and for id "some random string" (but ok, those totally invalid references won't cause collisions later on).

It would be nice it doctrine had some way of handling these invalid references gracefully. E.g. if addToIdentityMap detects a collision and sees the existing entry is a reference, it could try to load it and if that fails remove it and replace it with the new entity. But I don't know if that detection can be made 100% reliable.

@flack
Copy link
Contributor

flack commented Jun 25, 2023

How terrible would it be to make getReference actually do a DB query to see if the identifier exists? Maybe this could be optional so that you can skip it if you're sure the ID is valid, e.g. something like this:

public function getReference($entityName, $id, $skip_id_check = false);

mpdude added a commit to mpdude/doctrine2 that referenced this issue Jun 25, 2023
mpdude added a commit to mpdude/doctrine2 that referenced this issue Jun 26, 2023
As one takeaway from doctrine#3037 (comment) and doctrine#843, we should look into better explaining the `EntityManager::getReference()` method, it’s semantics, caveats and potential responsibilities placed on the user.

This PR tries to do that, so it fixes doctrine#10797.
SenseException pushed a commit that referenced this issue Jun 26, 2023
* Explain `EntityManager::getReference()` peculiarities

As one takeaway from #3037 (comment) and #843, we should look into better explaining the `EntityManager::getReference()` method, it’s semantics, caveats and potential responsibilities placed on the user.

This PR tries to do that, so it fixes #10797.

* Update docs/en/reference/advanced-configuration.rst

Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>

---------

Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants