Breaking change to optimistic locking in 2.8.2 #6646

New Issue

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

admin commented

2026-01-22 15:36:25 +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 referenced this issue

2026-01-22 16:06:22 +01:00

[PR #6646] add test for corrupted entry in the cache #10082

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#6646