From 2c2bb1b3a4669327897329009ed3c7c7a4d97c6b Mon Sep 17 00:00:00 2001 From: techmahedy Date: Fri, 30 Oct 2026 21:17:00 +0600 Subject: [PATCH] Fix save() leaving the model dirty after a quiet save --- README.md | 11 +- .../InteractsWithModelQueryProcessing.php | 12 +- tests/Model/EntityModelHookTest.php | 200 ++++++++++++++++++ tests/Model/WatcherTest.php | 16 ++ 4 files changed, 232 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 4a4fd8a9..8a43856f 100644 --- a/README.md +++ b/README.md @@ -10,6 +10,7 @@ Doppar isn't one package pretending to be a framework. It's a core plus a set of |---|---| | [Ai](https://github.com/doppar/ai) | AI integration, wired into the framework's own conventions. | | [Queue](https://github.com/doppar/queue) | Database-backed background jobs with process-isolated timeouts — a stuck job gets killed, not left to hang the worker. | +| [Ripple](https://github.com/doppar/ripple) | Build reactive interfaces in pure PHP. | | [Embeds](https://github.com/doppar/embeds) | Semantic search for your models. Locally-computed embeddings, no external API required. | | [Guard](https://github.com/doppar/guard) | Authorization — gates and policies without wiring your own. | | [Flarion](https://github.com/doppar/flarion) | API authentication. | @@ -26,10 +27,12 @@ Every package here is versioned and released alongside the core. Add what your a ## Get started -- **Documentation:** [doppar.com](https://doppar.com/versions/4.x/installation) -- **What's new in 4.x:** [doppar.com/versions/4.x/releases](https://doppar.com/versions/4.x/releases) -- **News and updates:** [blog.doppar.com](https://blog.doppar.com) -- **Doppar tour:** [tour.doppar.com](https://tour.doppar.com) +- **Documentation:** [Doppar](https://doppar.com/versions/4.x/installation) +- **What's new in 4.x:** [Releases](https://doppar.com/versions/4.x/releases) +- **AI:** [AI](https://ai.doppar.com) +- **Ripple:** [Ripple](https://ripple.doppar.com) +- **News and updates:** [Blog](https://blog.doppar.com) +- **Doppar tour:** [Tour](https://tour.doppar.com) ## Contributing diff --git a/src/Phaseolies/Database/Entity/Query/InteractsWithModelQueryProcessing.php b/src/Phaseolies/Database/Entity/Query/InteractsWithModelQueryProcessing.php index 8728110d..4fae29d1 100644 --- a/src/Phaseolies/Database/Entity/Query/InteractsWithModelQueryProcessing.php +++ b/src/Phaseolies/Database/Entity/Query/InteractsWithModelQueryProcessing.php @@ -181,10 +181,16 @@ public function save(): bool ->where($this->primaryKey, $this->attributes[$this->primaryKey]) ->update($dirtyAttributes); - if (! $this->hooksSuppressed && $response) { + if ($response) { $this->pruneNonColumnDirtyAttributes($this->getDirtyAttributes()); - $this->fireAfterHooks('updated'); - $this->firePropertyWatches($dirtyAttributes); + + if (! $this->hooksSuppressed) { + $this->fireAfterHooks('updated'); + $this->firePropertyWatches($dirtyAttributes); + } + + // Sync even when hooks are suppressed, otherwise the model + // stays dirty for a change that is already persisted. $this->originalAttributes = $this->attributes; } diff --git a/tests/Model/EntityModelHookTest.php b/tests/Model/EntityModelHookTest.php index 7d552dd0..60690ca4 100644 --- a/tests/Model/EntityModelHookTest.php +++ b/tests/Model/EntityModelHookTest.php @@ -410,4 +410,204 @@ public function testWithHooksAndWithoutHookTogetherSkipHooks(): void $this->assertSame('Quiet', MockHook::find(1)->name); $this->assertSame('Quiet', MockHook::find(3)->name); } + + /** + * Register an after_updated spy on MockHook that records how many times it + * ran and what getOriginal('name') was while it ran. + * + * @return \stdClass + */ + private function spyAfterUpdated(): \stdClass + { + $spy = new \stdClass(); + $spy->calls = 0; + $spy->originals = []; + + $this->tempHooks([ + 'after_updated' => function ($model) use ($spy) { + $spy->calls++; + $spy->originals[] = $model->getOriginal('name'); + }, + ]); + + return $spy; + } + + public function testQuietSaveLeavesTheModelClean(): void + { + $hook = MockHook::find(1); + $hook->name = 'Quiet'; + $hook->withoutHook()->save(); + + $this->assertSame([], $hook->getDirtyAttributes()); + $this->assertFalse($hook->isDirtyAttr('name')); + $this->assertSame('Quiet', $hook->getOriginal('name')); + } + + public function testQuietSaveStillPersistsTheChange(): void + { + $hook = MockHook::find(1); + $hook->name = 'Quiet'; + + $this->assertTrue($hook->withoutHook()->save()); + $this->assertSame('Quiet', MockHook::find(1)->name); + } + + public function testQuietSaveDoesNotFireAfterUpdated(): void + { + $spy = $this->spyAfterUpdated(); + + $hook = MockHook::find(1); + $hook->name = 'Quiet'; + $hook->withoutHook()->save(); + + $this->assertSame(0, $spy->calls); + } + + public function testPlainSaveAfterQuietSaveDoesNotRefireTheHook(): void + { + $spy = $this->spyAfterUpdated(); + + $hook = MockHook::find(1); + $hook->name = 'Quiet'; + $hook->withoutHook()->save(); + + $this->assertTrue($hook->save()); // nothing new changed + + $this->assertSame(0, $spy->calls, 'a change that is already persisted must not fire after_updated later'); + } + + public function testPlainSaveAfterQuietSaveDoesNotRewriteTheRow(): void + { + $hook = MockHook::find(1); + $hook->name = 'Quiet'; + $hook->withoutHook()->save(); + + // Change the row behind the model's back. A stale-dirty model would + // overwrite it again on the next save(). + $this->pdo->exec("UPDATE hooks SET name = 'Changed elsewhere' WHERE id = 1"); + + $hook->save(); + + $this->assertSame('Changed elsewhere', MockHook::find(1)->name); + } + + public function testChangeAfterQuietSaveIsTheOnlyDirtyAttribute(): void + { + $spy = $this->spyAfterUpdated(); + + $hook = MockHook::find(1); + $hook->name = 'Quiet'; + $hook->withoutHook()->save(); + + $hook->name = 'Loud'; + $this->assertSame(['name' => 'Loud'], $hook->getDirtyAttributes()); + + $hook->save(); + + $this->assertSame(1, $spy->calls); + $this->assertSame(['Quiet'], $spy->originals, 'the hook must see the quiet value as the original'); + $this->assertSame('Loud', MockHook::find(1)->name); + } + + public function testQuietSaveAppliesToOneWriteOnly(): void + { + $spy = $this->spyAfterUpdated(); + + $hook = MockHook::find(1); + $hook->name = 'Quiet'; + $hook->withoutHook()->save(); + + $hook->name = 'Loud'; + $hook->save(); + + $hook->name = 'Louder'; + $hook->save(); + + $this->assertSame(2, $spy->calls); + } + + public function testRepeatedQuietSavesStayClean(): void + { + $hook = MockHook::find(1); + + foreach (['One', 'Two', 'Three'] as $name) { + $hook->name = $name; + $hook->withoutHook()->save(); + + $this->assertSame([], $hook->getDirtyAttributes()); + } + + $this->assertSame('Three', MockHook::find(1)->name); + } + + public function testQuietSaveWithNothingDirtyDoesNotLeakSuppressionIntoTheNextSave(): void + { + $spy = $this->spyAfterUpdated(); + + $hook = MockHook::find(1); + $hook->withoutHook()->save(); // nothing dirty, returns early + + $hook->name = 'Loud'; + $hook->save(); + + $this->assertSame(1, $spy->calls); + } + + public function testNormalSaveStillExposesTheOldValueInsideAfterUpdated(): void + { + $spy = $this->spyAfterUpdated(); + + $hook = MockHook::find(1); + $hook->name = 'Loud'; + $hook->save(); + + $this->assertSame(1, $spy->calls); + $this->assertSame(['John Doe'], $spy->originals, 'originals must only be synced after the hook has run'); + $this->assertSame([], $hook->getDirtyAttributes()); + $this->assertSame('Loud', $hook->getOriginal('name')); + } + + public function testNormalSaveFollowedByAnotherSaveDoesNotRefireTheHook(): void + { + $spy = $this->spyAfterUpdated(); + + $hook = MockHook::find(1); + $hook->name = 'Loud'; + $hook->save(); + $hook->save(); + + $this->assertSame(1, $spy->calls); + } + + public function testQuietSaveOnOneInstanceDoesNotAffectAnotherInstanceOfTheSameRow(): void + { + $spy = $this->spyAfterUpdated(); + + $quiet = MockHook::find(1); + $other = MockHook::find(1); + + $quiet->name = 'Quiet'; + $quiet->withoutHook()->save(); + + $other->name = 'Loud'; + $other->save(); + + $this->assertSame(1, $spy->calls); + $this->assertSame([], $quiet->getDirtyAttributes()); + $this->assertSame([], $other->getDirtyAttributes()); + } + + public function testQuietUpdateAndQuietSaveBehaveTheSame(): void + { + $viaSave = MockHook::find(1); + $viaSave->name = 'Same'; + $viaSave->withoutHook()->save(); + + $viaUpdate = MockHook::find(2); + $viaUpdate->withoutHook()->update(['name' => 'Same']); + + $this->assertSame($viaUpdate->getDirtyAttributes(), $viaSave->getDirtyAttributes()); + $this->assertSame($viaUpdate->getOriginal('name'), $viaSave->getOriginal('name')); + } } diff --git a/tests/Model/WatcherTest.php b/tests/Model/WatcherTest.php index c4a6d2a8..7ba1e218 100644 --- a/tests/Model/WatcherTest.php +++ b/tests/Model/WatcherTest.php @@ -974,6 +974,22 @@ public function testWithoutHookDisablesWatchesOnSave(): void $this->assertFalse(SpyListener::$called, 'withoutHook() must suppress watches'); } + /** A quiet save() must not leave the change pending, or the next save() fires the watch late */ + public function testSaveAfterQuietSaveDoesNotFireTheWatchLater(): void + { + $id = $this->insertOrder('pending', 100, 'test'); + $order = MockWatchableOrder::find($id); + + $order->status = 'cancelled'; + $order->withoutHook()->save(); + $this->assertFalse(SpyListener::$called); + + $order->save(); // nothing new changed + + $this->assertFalse(SpyListener::$called, 'an already-persisted change must not fire a watch on a later save'); + $this->assertSame([], $order->getDirtyAttributes()); + } + /** withoutHook() prevents watches from firing on update() */ public function testWithoutHookDisablesWatchesOnUpdate(): void {