Make LockMode::NONE a no-op - #12548
Conversation
`LockMode::NONE` means "no lock", yet two code paths still treated it like a pessimistic lock mode: - `EntityManager::find()` refreshed an entity that was already in the identity map, silently discarding in-memory changes. A caller asking for "no lock" has no reason to expect a reload. - `UnitOfWork::lock()` demanded an active transaction and then executed `SELECT 1 FROM <table> WHERE id = ?` with no lock hint at all — a query that locks nothing. Both are leftovers from doctrine#932 (2014), when `LockMode::NONE` made SQL Server emit `WITH (NOLOCK)` and therefore did affect isolation. DBAL removed that hint in doctrine/dbal#4400 because it "is not the contract of `LockMode::NONE`", so nothing is left to guard. `EntityManager::refresh()`, `checkLockRequirements()` and (since doctrine#12516) `Query::setLockMode()` already ignore `NONE`. This aligns the last two. Follow-up to doctrine#12516, and picks up the remainder of the closed doctrine#8341.
|
Yeah good one. Should I do that here or in a new Pr |
| Both now do nothing, which is consistent with `EntityManager::refresh()` and | ||
| `Query::setLockMode()`. If you relied on `find()` reloading the entity, call | ||
| `EntityManager::refresh()` explicitly. | ||
|
|
There was a problem hiding this comment.
Technically, we're changing the behavior hiere, although that behavior is undocumented. How likely do we think it is that existing applications rely on this "hidden feature"?
There was a problem hiding this comment.
Hmmm, I find it a bit scary to be honest.
So if you would use EntityManager::find(132, LockMode::NONE) it would actually refresh from database while if you did EntityManager::find(123) it would not, if it already exists in the UOW.
What to do?
There was a problem hiding this comment.
Hmmm, I find it a bit scary to be honest.
Me too! 🫣
What to do?
I don't know. I don't see a better way forward, tbh. Maybe, this change is acceptable as is, given that the old behavior was unintentional and undocumented.
Either way: If we feel like we need to document this change of behavior in the upgrade document, we should not target a bugfix release. Let's target 3.7.x.
There was a problem hiding this comment.
I agree, let's do it in 3.7.x then!
3 participants
LockMode::NONEmeans "no lock", yet two code paths still treated it like a pessimistic lock mode:EntityManager::find()refreshed an entity that was already in the identity map, silently discarding in-memory changes. A caller asking for "no lock" has no reason to expect a reload.UnitOfWork::lock()demanded an active transaction and then executedSELECT 1 FROM <table> WHERE id = ?with no lock hint at all — a query that locks nothing.Both are leftovers from #932 (2014), when
LockMode::NONEmade SQL Server emitWITH (NOLOCK)and therefore did affect isolation. DBAL removed that hint in doctrine/dbal#4400 because it "is not the contract ofLockMode::NONE", so nothing is left to guard.EntityManager::refresh(),checkLockRequirements()and (since #12516)Query::setLockMode()already ignoreNONE. This aligns the last two.Follow-up to #12516, and picks up the remainder of the closed #8341.