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

[2.6] Fix for #7068: EntityManager::find() with pessimistic lock should check for transaction #7291

Merged
merged 1 commit into from
Jul 3, 2018

Conversation

Majkl578
Copy link
Contributor

@Majkl578 Majkl578 commented Jul 3, 2018

Rebased and adjusted #7075 for 2.6.
Fixes #7068.

@Majkl578 Majkl578 added the Bug label Jul 3, 2018
@Majkl578 Majkl578 added this to the 2.6.2 milestone Jul 3, 2018
@Majkl578 Majkl578 requested review from Ocramius and lcobucci July 3, 2018 01:01
@Majkl578 Majkl578 changed the title Fix for #7068: EntityManager::find() with pessimistic lock should check for transaction [2.6] Fix for #7068: EntityManager::find() with pessimistic lock should check for transaction Jul 3, 2018
* @throws OptimisticLockException
* @throws TransactionRequiredException
*/
private function checkLockRequirements(int $lockMode, ClassMetadata $class): void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: we use a space between ) and :. Not important though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, leftover from original PR.

@Ocramius Ocramius self-assigned this Jul 3, 2018
@Ocramius Ocramius merged commit 3cfcd6a into doctrine:2.6 Jul 3, 2018
@Ocramius
Copy link
Member

Ocramius commented Jul 3, 2018

Thanks @Majkl578!

@timdev
Copy link
Contributor

timdev commented Aug 22, 2018

Why the intentional fallthrough here? It causes a BC break when using optimistic locking outside of a tx. I'm probably missing something, but it just bit me.

@Majkl578
Copy link
Contributor Author

Majkl578 commented Aug 22, 2018

I think you are right, the transaction shouldn't be required for an optimistic lock (documentation examples agree), at least not in 2.x. Do you want to submit a PR? 👍

@timdev
Copy link
Contributor

timdev commented Aug 22, 2018

My pleasure: #7367

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.

3 participants