Refresh entity original values after saving in RepositoryCore - #12
Open
john-builder-nm wants to merge 1 commit into
Open
Refresh entity original values after saving in RepositoryCore#12john-builder-nm wants to merge 1 commit into
john-builder-nm wants to merge 1 commit into
Conversation
RepositoryCore::save() computed its UPDATE set by diffing the current entity fields against the original values recorded at hydration time, but never refreshed those original values afterwards. Changing a field, saving, and then changing it back to the value it was loaded with left an empty diff, so the second save silently did nothing and the database kept the intermediate value. The create branch of save() had the same problem in reverse: the caller entity kept an empty baseline, so every later save rewrote every field, which can clobber columns that a trigger or a database side coercion filled in differently. Both branches now take the values the entity holds after a successful write as the new baseline.
john-builder-nm
force-pushed
the
fix/repository-save-refresh-original-values
branch
from
August 17, 2026 20:49
c1d149e to
4e39d3c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
RepositoryCore::save()builds its UPDATE set fromgetChangedFields(), which diffs the current entity fields against$entity->getOriginalValues(). Those original values are only ever filled ininstantiateEntityFromData(), at hydration time. They are never refreshed after a successful UPDATE, and they are never set at all on the create branch ofsave()(therecreate($values)re-finds a fresh object, but the caller's$entitykeepsoriginalValues = []).That gives two problems on a single entity instance:
save()(persists), set A back to the value it was loaded with,save()again: the diff against the stale hydration baseline is empty, no UPDATE is issued, and the database keeps the intermediate value. The caller has no way to notice,save()returns void.save()on a new entity the baseline stays[], so every latersave()on that same instance writes every initialized field, including fields the caller never touched.The first one is not theoretical. It was reproduced in production in a downstream application built on WebFramework. A rebuild flow flipped a boolean flag off, saved, flipped it back on, saved, and the row stayed off. A currency transfer set
onhandback to its hydrated value while creditingbanked, and the amount ended up duplicated because theonhandwrite was dropped.Minimal reproduction, one instance, no concurrency:
The fix
src/Repository/RepositoryCore.php:setOriginalValues()is called with the same field array the diff was computed from. On that pathsave()now retrieves the entity fields once and uses them for both the diff and the new baseline.getEntityFields()pass on the create path, after the copy, because the additional id fields only get their value at that point.getChangedFields()keeps its signature and behavior, it now delegates to a privatediffWithOriginalValues()sosave()can pass in already retrieved fields.The refreshed baseline keeps the
idkey, becauseinstantiateEntityFromData()records it too, sogetOriginalValues()has the same keys for a hydrated entity and for a saved one. The values are not identical in type: a hydrated baseline holds the raw row values as the database driver returned them (mostly strings), a refreshed baseline holds the PHP typed property values. That makes no difference to the change tracking, becausearray_diff_assoc()compares as strings, but it is visible to anyone readinggetOriginalValues()directly.Why the baseline comes from the entity and not from the re-found row
create()deliberately re-fetches the row, because the inserted data misses database defaults and trigger filled columns. Copying that re-found object's values onto the baseline would be wrong though. The entity itself is not updated from that object, only the additional id fields are copied back, so a database truthful baseline on a stale entity would make the very next diff see a difference and write the PHP value back over what the database put there. That is exactly the clobbering this PR removes, so the baseline has to describe the entity, not the row.The baseline is therefore taken from the entity itself, after the additional id fields have been copied over. Note that
getEntityFields()skips uninitialized properties, so:guidofEntity\VerificationCode(inbaseFieldsand inadditionalIdFields, declared without a default), is uninitialized at insert time, so it is not in the INSERT and, being skipped bygetEntityFields(), it is not in the baseline and not in any later diff either. After the additional id copy it holds the value the database generated, and the baseline is taken after that copy, so it records the real value and a latersave()leaves it alone.save()does not touch that column unless the caller changes it. Today, with the empty baseline, every latersave()rewrites that column with the PHP value and clobbers what the database put there. So on that axis the new behavior is strictly better than the current one, never worse.Behavior change to be aware of
Code that relies on
save()rewriting every field of a just created entity on the nextsave()will now see only the fields that actually changed. That is the intended semantics of the change tracking, and it matches what asave()on a hydrated entity already does, but it is a visible difference for an application that used the create path as a full row refresh.Nothing else in the framework reads
getOriginalValues(). A grep oversrc/,tests/anddocs/finds it inEntity\EntityandEntity\EntityCore(the accessors), inRepositoryCore::getChangedFields(), and in the twoinstantiateEntityFromData()unit tests. There is no audit trail or "value at load time" consumer that this would affect.Tests
Added to
tests/Unit/RepositoryCoreTest.php, in the existing style, usingTests\Support\TestRepositoryand aDatabasestub that records the queries it receives:testSaveRevertedValueAfterSaveIsPersisted: update then revert on one instance issues a second UPDATE, with the reverted value in the params.testSaveUnchangedEntityAfterUpdateDoesNotQueryAgain: saving twice without a change in between issues only one UPDATE.testSaveAfterCreateDoesNotUpdateWhenUnchanged: after a create throughsave(), a secondsave()without changes issues no query at all.testSaveAfterCreateOnlyUpdatesChangedField: after a create throughsave(), changing one field issues an UPDATE with only that field.All four fail on the current code and pass with the fix. Before the fix, the failures are exactly the bug: 1 query instead of 2 for the revert case, 2 instead of 1 for the unchanged case, and
UPDATE test_entities SET name = ?, email = ?, age = ?, active = ?, secret_field = ?, created_at = ?where onlynamechanged.Gates
vendor/bin/codecept run: OK, 658 tests, 1410 assertions.vendor/bin/phpstan analyse: 1 error,actions/Tester.php:108argument.invalidConstant. That error is present on unmodifiedmainas well (verified by stashing this change), so it is pre-existing and unrelated..php-cs-fixer.dist.php. The changed files produce no findings from this change. That version does report repository wide differences (it wantsdeclare(strict_types=1)and: voidon test methods everywhere, on files this PR does not touch), which looks like drift between that release and the one your CI picked up, so I did not apply any of it.Documentation
Added an
Unreleasedbug fixes entry toMIGRATE.md, the same heading this file used before the v10.0 section got its number. Rename it to whatever version you cut this in.