Skip to content

Refresh entity original values after saving in RepositoryCore - #12

Open
john-builder-nm wants to merge 1 commit into
avoutic:mainfrom
john-builder-nm:fix/repository-save-refresh-original-values
Open

Refresh entity original values after saving in RepositoryCore#12
john-builder-nm wants to merge 1 commit into
avoutic:mainfrom
john-builder-nm:fix/repository-save-refresh-original-values

Conversation

@john-builder-nm

@john-builder-nm john-builder-nm commented Aug 17, 2026

Copy link
Copy Markdown

The bug

RepositoryCore::save() builds its UPDATE set from getChangedFields(), which diffs the current entity fields against $entity->getOriginalValues(). Those original values are only ever filled in instantiateEntityFromData(), at hydration time. They are never refreshed after a successful UPDATE, and they are never set at all on the create branch of save() (there create($values) re-finds a fresh object, but the caller's $entity keeps originalValues = []).

That gives two problems on a single entity instance:

  1. Silent data loss on update. Set field A to a new value, 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.
  2. Every field rewritten forever after create. After save() on a new entity the baseline stays [], so every later save() 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 onhand back to its hydrated value while crediting banked, and the amount ended up duplicated because the onhand write was dropped.

Minimal reproduction, one instance, no concurrency:

$user = $repository->find(42);   // username is 'original'
$user->setUsername('temporary');
$repository->save($user);        // UPDATE users SET username = 'temporary'
$user->setUsername('original');
$repository->save($user);        // nothing happens, database keeps 'temporary'

The fix

src/Repository/RepositoryCore.php:

  • After a successful UPDATE, the fields that were just written become the new baseline: setOriginalValues() is called with the same field array the diff was computed from. On that path save() now retrieves the entity fields once and uses them for both the diff and the new baseline.
  • On the create branch, after the id and the additional id fields have been copied onto the entity, the baseline is set to the entity fields as they are then. That is one extra 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 private diffWithOriginalValues() so save() can pass in already retrieved fields.
  • The early "nothing to update" return leaves the baseline alone, which is correct, nothing was written.

The refreshed baseline keeps the id key, because instantiateEntityFromData() records it too, so getOriginalValues() 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, because array_diff_assoc() compares as strings, but it is visible to anyone reading getOriginalValues() 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:

  • A property the entity never set, such as the guid of Entity\VerificationCode (in baseFields and in additionalIdFields, declared without a default), is uninitialized at insert time, so it is not in the INSERT and, being skipped by getEntityFields(), 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 later save() leaves it alone.
  • An initialized property is always in the INSERT set list, so a column DEFAULT never applies to it. What can still make the stored row differ is a trigger, a generated column, or a database side coercion. In that case the entity and the baseline both keep the PHP value, so a later save() does not touch that column unless the caller changes it. Today, with the empty baseline, every later save() 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 next save() will now see only the fields that actually changed. That is the intended semantics of the change tracking, and it matches what a save() 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 over src/, tests/ and docs/ finds it in Entity\Entity and Entity\EntityCore (the accessors), in RepositoryCore::getChangedFields(), and in the two instantiateEntityFromData() 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, using Tests\Support\TestRepository and a Database stub 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 through save(), a second save() without changes issues no query at all.
  • testSaveAfterCreateOnlyUpdatesChangedField: after a create through save(), 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 only name changed.

Gates

  • vendor/bin/codecept run: OK, 658 tests, 1410 assertions.
  • vendor/bin/phpstan analyse: 1 error, actions/Tester.php:108 argument.invalidConstant. That error is present on unmodified main as well (verified by stashing this change), so it is pre-existing and unrelated.
  • php-cs-fixer: run with a locally installed 3.95 against .php-cs-fixer.dist.php. The changed files produce no findings from this change. That version does report repository wide differences (it wants declare(strict_types=1) and : void on 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 Unreleased bug fixes entry to MIGRATE.md, the same heading this file used before the v10.0 section got its number. Rename it to whatever version you cut this in.

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
john-builder-nm force-pushed the fix/repository-save-refresh-original-values branch from c1d149e to 4e39d3c Compare August 17, 2026 20:49
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.

1 participant