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

Use LockMode::NONE as default lock mode #8341

Open
wants to merge 1 commit into
base: old-prototype-3.x
Choose a base branch
from

Conversation

BenMorel
Copy link
Contributor

As discussed in doctrine/dbal#4391, the $lockMode parameter should not be nullable anywhere; what should be used as a default is LockMode::NONE.

There was a long standing confusion in the DBAL that can be summarized to:

  • in all platforms but SQL Server / SQL Anywhere, null was equivalent to LockMode::NONE;
  • in SQL Server / SQL Anywhere, LockMode::NONE resulted in using WITH (NOLOCK), which changed the effective isolation level to READ UNCOMMITTED for the table; this is not the contract of LockMode::NONE, and was qualified as a bug.

This has been fixed in doctrine/dbal#4400, and now null is not a valid value anymore for lock modes.

This PR is the sequel to the above:

  • $lockMode defaults to LockMode::NONE everywhere, null is not allowed anymore
  • no transaction is required anymore when using LockMode::NONE, which didn't make any sense

Notes:

  • the transaction requirement for LockMode::NONE was highly inconsistent anyway: the Query required one, but EntityManager::find() didn't
  • there were many inconsistencies in the documented types for $lockMode as well, which did document in many places that only an int was acceptable, but did accept or even expect null at some point: 1, 2, 3

@BenMorel
Copy link
Contributor Author

Looks like composer lock file is broken; CI can't run.

@BenMorel
Copy link
Contributor Author

@beberlei Could you please take a look at this PR? This should be an easy pick.

@SenseException
Copy link
Member

You need to re-target your PR to a different branch, but I'm not sure if this is supposed to be a bugfix or an improvement for a newer branch. composer.lock isn't in the versioning in 2.8.x and 2.9.x anymore and shouldn't be a problem.

@BenMorel
Copy link
Contributor Author

@SenseException This is not a bugfix, rather an improvement. Should this go into 2.9.x? What's master currently, is it 3.x?

@SenseException
Copy link
Member

If master is the correct target branch, then it needs a new upmerge. It might be a current blocker.

@BenMorel
Copy link
Contributor Author

That's the point, I'm not sure if master is the correct branch; is it 3.x?

@SenseException
Copy link
Member

@doctrine/team-orm Is there a suggestion about the target branch for this PR?

Base automatically changed from master to old-prototype-3.x February 23, 2021 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants