Do not require an active transaction for LockMode::NONE in Query::setLockMode()

[ruudk]

LockMode::NONE means "no lock", so it does not need an active transaction. EntityManager::checkLockRequirements() already only enforces a transaction for the pessimistic lock modes, but Query::setLockMode() also rejected NONE outside a transaction, making the two inconsistent.

Fixes #9499

[ruudk]

@greg0ire @derrabus Curious to hear your thoughts on this. The error is unrelated to this PR.

[greg0ire]

I find it weird that LockMode::NONE is deliberately present here. It's not like somebody forgot to exclude it. Looking at the history, I see it was added in #932

If you have time to dig into why, please do.

[ruudk]

You're right that it was deliberate, not a forgotten exclusion. The reason is just long gone.

It was added in #932 (a6c8ab8, 2014). Back then LockMode::NONE made SQL Server emit WITH (NOLOCK), which effectively means READ UNCOMMITTED at table level. That PR introduced the null vs NONE distinction, and because NONE now produced a real isolation-affecting hint, it grouped it with the pessimistic modes to require a transaction, in both EntityManager::find() and Query::setLockMode(). At that point they were consistent.

Two things killed that rationale since:

1. 1dbacec (2015) removed NONE from the transaction requirement on the EntityManager side. That's where the two drifted apart. Query::setLockMode() just never got updated to match.
2. In DBAL, 3ed11aa (LockMode::NONE should not set WITH (NOLOCK) dbal#4400, fixing Clarify LockMode::NONE dbal#4391, released in 2.12.1) made appendLockHint() stop emitting WITH (NOLOCK) for NONE, with the explicit reasoning that NOLOCK "is not the contract of LockMode::NONE." So NONE now emits zero lock SQL on every platform.

Current SQLServer appendLockHint returns the from-clause unchanged for NONE: SQLServerPlatform.php.

So Query::setLockMode() is the last leftover of the old design. This change doesn't drop a safeguard, it finishes the alignment that's been half-done since 2015.

[ruudk]

Maybe @BenMorel wants to chip in 😊 Thanks 🙏

[ruudk]

I think that

orm/src/EntityManager.php

Line 338 in ed636b7

case $lockMode === LockMode::NONE:

should also be removed. Why would we force a refresh when LockMode::None is used?

[greg0ire]

Maybe yes. Try removing it, and if nothing break then it's probably the right thing to do.

[BenMorel]

@ruudk Thank you for pinging me.

I agree with this change: my DBAL PR, plus this one on the ORM (which was closed due to inactivity but I believe is still relevant today), were heading in the same direction.