Breaking change to optimistic locking in 2.8.2 #6647

New Issue

admin · 2026-01-22T15:36:27+01:00

admin commented

2026-01-22 15:36:27 +01:00

Originally created by @acoulton on GitHub (Mar 9, 2021).

Version 2.8.2 introduced a breaking change to the way optimistic lock versions are compared.

This has been reported (and fixed) specifically for DateTime objects as #8499 but in reality it applies more broadly.

Prior to 2.8.2 the version was compared with a != operator. However among the 25k lines changed in this automated coding style fix commit is a change from that to a !== operator.

Previously, this would work:

// view.php
//... form stuff
<input type="hidden" name="version" value="<?=$entity->getVersion();?>">

// controller.php
$entity = $em->find('MyEntity', $id, LockMode::OPTIMISTIC, $_POST['version']);

However because the POST value is a string rather than an int it now consistently fails with an OptimisticLockException.

I appreciate that the documentation examples for this show the controller code explicitly casting the incoming value to (int) $version before passing it in. Also that the various Doctrine phpdocs have @param int|null $lockVersion - although this is technically incorrect, since it's also valid to pass a DateTimeInterface.

So there's an argument the old behaviour was wrong, and the caller should always have been forced to cast the value if required.

On the other hand, since there's only (incomplete/inaccurate) phpdoc rather than strict typehints, and it's always "just worked" it seems reasonable to assume a fair number of people will be just passing in the POST/GET value without explicitly casting.

And arguably it should actually be valid to pass the raw string here - AFAIK integer-versioned entities always start at version 1, so there's no case where you could get a false-positive-version match by passing in a string that casts to 0. If you pass a string, either it contains the value of the correct integer (and the lock is valid), or you haven't (and the lock fails).

Either way the release changes previously-working behaviour, so should at very least have a changelog / commit message highlighting that even if you consider this a side-effect of a bugfix rather than a semver-level breaking change.

More generally, I don't think it's ideal practice to include behavioural changes (comparison operators etc) alongside stylistic fixes in a single commit that is too big for consumers to realistically review. The diff between 2.8.1 and 2.8.2 has 1,199 changed files with 28,067 additions and 25,814 deletions and with no changelog beyond the 78 mostly-cursory commit messages it's very hard to see what's included.

Fortunately this issue was caught by our integration tests before shipping to production, but it does now make me nervous that there may be strict-equality related bugs that have yet to surface. Even if the equality operator fixes had been split to a separate commit it would be much easier to assess whether there are others that might affect us.

Originally created by @acoulton on GitHub (Mar 9, 2021). Version 2.8.2 introduced a breaking change to the way optimistic lock versions are compared. This has been reported (and fixed) specifically for DateTime objects as #8499 but in reality it applies more broadly. Prior to 2.8.2 the version was compared with a `!=` operator. However among the 25k lines changed in this [automated coding style fix commit](https://github.com/doctrine/orm/commit/68bc00b6c672349925c247951f13c7c90eb801e6#diff-55a900494fc8033ab498c53929716caf0aa39d6bdd7058e7d256787a24412ee4R2502) is a change from that to a `!==` operator. Previously, this would work: ```php // view.php //... form stuff <input type="hidden" name="version" value="<?=$entity->getVersion();?>"> // controller.php $entity = $em->find('MyEntity', $id, LockMode::OPTIMISTIC, $_POST['version']); ``` However because the POST value is a `string` rather than an `int` it now consistently fails with an OptimisticLockException. I appreciate that the documentation examples for this show the controller code explicitly casting the incoming value to `(int) $version` before passing it in. Also that the various Doctrine phpdocs have `@param int|null $lockVersion` - although this is technically incorrect, since it's also valid to pass a DateTimeInterface. So there's an argument the old behaviour was wrong, and the caller should always have been forced to cast the value if required. On the other hand, since there's only (incomplete/inaccurate) phpdoc rather than strict typehints, and it's always "just worked" it seems reasonable to assume a fair number of people will be just passing in the POST/GET value without explicitly casting. And arguably it should actually be valid to pass the raw string here - AFAIK integer-versioned entities always start at version 1, so there's no case where you could get a false-positive-version match by passing in a string that casts to `0`. If you pass a string, either it contains the value of the correct integer (and the lock is valid), or you haven't (and the lock fails). Either way the release changes previously-working behaviour, so should at very least have a changelog / commit message highlighting that even if you consider this a side-effect of a bugfix rather than a semver-level breaking change. More generally, I don't think it's ideal practice to include behavioural changes (comparison operators etc) alongside stylistic fixes in a single commit that is too big for consumers to realistically review. The [diff between 2.8.1 and 2.8.2](https://github.com/doctrine/orm/compare/242cf1a33df1b8bc5e1b86c3ebd01db07851c833...ebae57eb9637acd8252b398df3121b120688ed5c) has 1,199 changed files with 28,067 additions and 25,814 deletions and with no changelog beyond the 78 mostly-cursory commit messages it's very hard to see what's included. Fortunately this issue was caught by our integration tests before shipping to production, but it does now make me nervous that there may be `strict-equality` related bugs that have yet to surface. Even if the equality operator fixes had been split to a separate commit it would be much easier to assess whether there are others that might affect us.

admin closed this issue

2026-01-22 15:36:28 +01:00

admin commented

2026-01-22 15:36:29 +01:00

@greg0ire commented on GitHub (Mar 9, 2021):

although this is technically incorrect, since it's also valid to pass a DateTimeInterface.

I believe this has been spotted and fixed in https://github.com/doctrine/orm/pull/8508

Either way the release changes previously-working behaviour, so should at very least have a changelog / commit message highlighting that even if you consider this a side-effect of a bugfix rather than a semver-level breaking change.

Previously-working, yet undocumented (I'm not talking about the DatetimeInterface) here, so no, it should not have a the very least a changelog/commit messages highlight etc. , because:

It was obviously not intended
Even if it was, we consider phpdoc part of the contract. In the case of DateTime the contract was in contradiction with the documentation so we accepted the fix.

More generally, I don't think it's ideal practice to include behavioural changes (comparison operators etc) alongside stylistic fixes in a single commit that is too big for consumers to realistically review. The diff between 2.8.1 and 2.8.2 has 1,199 changed files with 28,067 additions and 25,814 deletions and with no changelog beyond the 78 mostly-cursory commit messages it's very hard to see what's included.
Fortunately this issue was caught by our integration tests before shipping to production, but it does now make me nervous that there may be strict-equality related bugs that have yet to surface. Even if the equality operator fixes had been split to a separate commit it would be much easier to assess whether there are others that might affect us.

You're right, I could have done a better job. The thing is, the codebase is huge, and it's hard to do a great job when there are that many errors to fix.

See this list of PRs if you want to witness how painstaking an endeavour it is. Anyway, I'm almost done so hopefully there won't be that many bugs.

Anyway, I appreciate the effort you put in writing this, but I don't consider this a breaking change, that would set a precedent for not trusting our documentation and not trusting our phpdoc, which is getting more and more accurate thanks to phpcs, Psalm and PHPStan. Both are very much contractual.

If it makes you feel any better, I think these phpdoc improvements are precisely moving Doctrine in a direction that leads to fewer bugs, since it will become easier to statically analyze.

If you want, you can contribute the feature back, but

I don't guarantee it will be accepted;
you will have to write a test for it;
you will have to document it.

it does now make me nervous that there may be strict-equality related bugs that have yet to surface

One way to catch those would be to use static analysis on your project I think, consider it.

@greg0ire commented on GitHub (Mar 9, 2021): > although this is technically incorrect, since it's also valid to pass a DateTimeInterface. I believe this has been spotted and fixed in https://github.com/doctrine/orm/pull/8508 > Either way the release changes previously-working behaviour, so should at very least have a changelog / commit message highlighting that even if you consider this a side-effect of a bugfix rather than a semver-level breaking change. Previously-working, yet undocumented (I'm not talking about the DatetimeInterface) here, so no, it should not have a the very least a changelog/commit messages highlight etc. , because: 1. It was obviously not intended 2. Even if it was, we consider phpdoc part of the contract. In the case of DateTime the contract was in contradiction with the documentation so we accepted the fix. > More generally, I don't think it's ideal practice to include behavioural changes (comparison operators etc) alongside stylistic fixes in a single commit that is too big for consumers to realistically review. The diff between 2.8.1 and 2.8.2 has 1,199 changed files with 28,067 additions and 25,814 deletions and with no changelog beyond the 78 mostly-cursory commit messages it's very hard to see what's included. > Fortunately this issue was caught by our integration tests before shipping to production, but it does now make me nervous that there may be strict-equality related bugs that have yet to surface. Even if the equality operator fixes had been split to a separate commit it would be much easier to assess whether there are others that might affect us. You're right, I could have done a better job. The thing is, the codebase is huge, and it's hard to do a great job when there are that many errors to fix. See [this list of PRs](https://github.com/doctrine/orm/pulls?q=is%3Apr+author%3Agreg0ire+is%3Aclosed+CS+batch) if you want to witness how painstaking an endeavour it is. Anyway, I'm almost done so hopefully there won't be that many bugs. Anyway, I appreciate the effort you put in writing this, but I don't consider this a breaking change, that would set a precedent for not trusting our documentation and not trusting our phpdoc, which is getting more and more accurate thanks to phpcs, Psalm and PHPStan. Both are very much contractual. If it makes you feel any better, I think these phpdoc improvements are precisely moving Doctrine in a direction that leads to fewer bugs, since it will become easier to statically analyze. If you want, you can contribute the feature back, but 1. I don't guarantee it will be accepted; 2. you will have to write a test for it; 2. you will have to document it. > it does now make me nervous that there may be strict-equality related bugs that have yet to surface One way to catch those would be to use static analysis on your project I think, consider it.

admin commented

2026-01-22 15:36:29 +01:00

@greg0ire commented on GitHub (Mar 10, 2021):

We discussed this internally, it will be reverted after all: https://github.com/doctrine/orm/pull/8531

Consider adding a test to make sure somebody else doesn't remove it again in the future.

@greg0ire commented on GitHub (Mar 10, 2021): We discussed this internally, it will be reverted after all: https://github.com/doctrine/orm/pull/8531 Consider adding a test to make sure somebody else doesn't remove it again in the future.

admin commented

2026-01-22 15:36:29 +01:00

@acoulton commented on GitHub (Mar 10, 2021):

@greg0ire thanks, sorry I didn't get a reply to you yesterday.

I didn't mean to come across too critical either, sorry if I did. Fully appreciate maintaining and modernising the codebase is a huge job, and absolutely agree it's moving in the right direction for newer projects and longer-term reliability and appreciate the work you and the other maintainers have been putting in on that.

Good point that adding static analysis and strict typing at our end would also help to protect against this sort of thing. We're heading that direction, though since we're working with several big and legacy codebases that's a fairly huge job in itself. In the meantime our end-to-end tests caught this before we updated, even though they couldn't point to the cause.

I'd really just wanted to flag that from a user perspective it would be really helpful to split out changes that could have functional impact (almost regardless of whether the behaviour is intended / contractual etc) from those that are just formatting and styling etc. Or at least to summarise in release notes the rules that have been applied / policies that have been changed (the release notes for 2.8.2 don't actually mention the CS fixes at all, they imply a much smaller changeset). Otherwise from the outside it's very hard to predict problems, or indeed to debug them if they come up.

I'll take a look at adding a test.

@acoulton commented on GitHub (Mar 10, 2021): @greg0ire thanks, sorry I didn't get a reply to you yesterday. I didn't mean to come across too critical either, sorry if I did. Fully appreciate maintaining and modernising the codebase is a huge job, and absolutely agree it's moving in the right direction for newer projects and longer-term reliability and appreciate the work you and the other maintainers have been putting in on that. Good point that adding static analysis and strict typing at our end would also help to protect against this sort of thing. We're heading that direction, though since we're working with several big and legacy codebases that's a fairly huge job in itself. In the meantime our end-to-end tests caught this before we updated, even though they couldn't point to the cause. I'd really just wanted to flag that from a user perspective it would be really helpful to split out changes that could have functional impact (almost regardless of whether the behaviour is intended / contractual etc) from those that are just formatting and styling etc. Or at least to summarise in release notes the rules that have been applied / policies that have been changed (the release notes for 2.8.2 don't actually mention the CS fixes at all, they imply a much smaller changeset). Otherwise from the outside it's very hard to predict problems, or indeed to debug them if they come up. I'll take a look at adding a test.

Sign in to join this conversation.

Branches Tags

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: doctrine/archived-orm#6647