Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Use of getOriginal() on model in observer doesn't return original value, when using transactions and ShouldHandleEventsAfterCommit.#52552

Unanswered
emil-nasso asked this question inQ&A
Discussion options

Laravel Version

11.20

PHP Version

8.3

Database Driver & Version

sqlite

Description

When using and observer to subscribe toupdated events on models, you can access the original value in a few different ways, including thegetOriginal(...) method.

If the observer implements theShouldHandleEventsAfterCommit and the changes to the model is wrapped in a databas transaction, the events should be processed after the transaction has commited. This works as expected.

However. When using transactions andShouldHandleEventsAfterCommit thegetOriginal(...) method no longer returns the original value.

I did not expect this, and can't find anything about it in the documentation. Is this a bug?

Steps To Reproduce

Below are test cases for this. It consists of:

  • A simple model.
  • An observer that observes the model and, onupdated, asserts that the value of$model->test isupdated and the value of$model->getOriginal('test') isoriginal
  • Two test-cases, one with transactions and one without. The cases both create a model with the fieldtest set tooriginal and then updates that field toupdated.

I would expect both of these test cases to pass, but the test case that uses transactions fails.

<?phpnamespaceTests;useIlluminate\Database\Eloquent\Model;class Testextends Model{protected$connection ='sqlite';protected$table ='test';public$timestamps =false;protected$fillable = ['test'];}
<?phpnamespaceTests;useIlluminate\Contracts\Events\ShouldHandleEventsAfterCommit;usePHPUnit\Framework\Assert;class Observerimplements ShouldHandleEventsAfterCommit{publicfunctionupdated(Test$model)    {        Assert::assertEquals('updated',$model->test);        Assert::assertEquals('original',$model->getOriginal('test'));    }}
<?phpnamespaceTests;useIlluminate\Database\Schema\Blueprint;useIlluminate\Foundation\Testing\RefreshDatabase;useIlluminate\Support\Facades\DB;useIlluminate\Support\Facades\Schema;class EventsInTransactionsTestextends TestCase{use RefreshDatabase;protectedfunctionconfig():void    {config()->set('database.connections.sqlite', ['driver' =>'sqlite','database' =>':memory:','prefix' =>'',        ]);        Schema::connection('sqlite')->create('test',function (Blueprint$table) {$table->id();$table->string('test');        });        Test::observe(Observer::class);    }publicfunctiontest_without_transaction():void    {$this->config();$model =newTest();$model->test ='original';$model->save();$model->test ='updated';$model->save();    }publicfunctiontest_with_transaction():void    {$this->config();DB::connection('sqlite')->transaction(function () {$model =newTest();$model->test ='original';$model->save();$model->test ='updated';$model->save();        });    }}
You must be logged in to vote

Replies: 5 comments 29 replies

Comment options

It seems that you're dealing with a situation where the model object being tracked within a transaction is modified, which affects the behavior of the callbacks assigned to that transaction. This can lead to issues, especially when working with models in the context of a transaction, particularly in conjunction with events like updated.

In short, when a transaction is registered, Laravel creates callbacks that will be executed after the transaction is committed. If the model is modified during the transaction and Laravel uses the same object for the registered callbacks, this can lead to a scenario where the callbacks operate on outdated or inconsistent data.

In PHP, objects are passed by reference, meaning that if you modify an object in one place, those changes will be reflected everywhere that object is used unless you specifically clone the object before making changes.

You must be logged in to vote
0 replies
Comment options

This is a big issue when building a logging mechanism for registering model changes, imagine this pretty common example:

Model to watch

class Orderextends Model{protected$fillable = ['state_id',// ...   ];}

Model to log changes

#[ObservedBy(OrderObserver::class)]class OrderStatesextends Model{protected$fillable = ['order_id','old_state_id','new_state_id',// ...   ];publicfunctionorder():BelongsTo    {return$this->belongsTo(Order::class,'order_id','id');    }}

Observer code

class OrderObserverimplements ShouldHandleEventsAfterCommit{publicfunctionupdated(Order$model)// tried also updating with no luck    {$new_state_id =$model->state_id;$previous_state_id =$model->getOriginal('state_id'));// here I would like to log the change in a OrderStates entry// but real previous state_id is lost when wrapping the change in a transaction...    }}

Example code

DB::beginTransaction;$order = Order::find(1);$order->update(['state_id' =>2]);// other relevant code...DB::commit();

Is there any other clean way to do this?

You must be logged in to vote
0 replies
Comment options

Solution

  1. Put this behavior in documentation (if not done already). This seems to be an issue converted into a discussion. Sad...
  2. Stop usingShouldHandleEventsAfterCommit and use queue with delay. In that job check if the model has the new state and if so use the old state (including timestamp) sent in the job to notify or log things@yurik94 . Do not use model id as job payload. Use model to array.

Thank you for this. We had thoughts of using thisShouldHandleEventsAfterCommit but we never used it. Lucky us.

You must be logged in to vote
1 reply
@macropay-solutions
Comment options

Better solution#52552 (comment)

Comment options

I have the same issue. Are there any updates?

You must be logged in to vote
0 replies
Comment options

I believe I've found a working solution for this! Avoid using ShouldHandleEventsAfterCommit and instead wrap any observer actions within manual DB::afterCommit() callbacks.

Using your example, modify the provided Observer like the following;

<?phpnamespaceTests;useIlluminate\Support\Facades\DB;usePHPUnit\Framework\Assert;class Observer{publicfunctionupdated(Test$model)    {$original =$model->getOriginal('test');DB::afterCommit(function ()use ($model,$original) {            Assert::assertEquals('updated',$model->test);            Assert::assertEquals('original',$original);        });    }}

This maintains proper event firing order (saving --> updating --> SQL executed --> updated) for both transactions & non-transactions (these will be executed immediately) and retains original model values, all while ensuring that any external actions still only happen after the SQL is committed to the database.

I was building something similar to what@yurik94 had suggested, an outbox-style logging system that was triggered from both transactions and non-transactions within my codebase. Interestingly (or frustratingly depending on how you look at it), assuming your observer is firing from a transaction AND you're only making DB changes, since the observer is ran on the same PHP thread the transaction rollbackwill properly undo any db changes. This flow retains atomicity, however if the observer can also be fired from outside of a transaction and the changes fail to commit to your database you will introduce ghost actions that shouldn't have occurred, breaking the atomicity.

By not using ShouldHandleEventsAfterCommit and instead using manual DB::afterCommit we are able to have the best of both worlds, atomicity on the save and atomicity on the executed observer actions.

Here's a real world example;

Situation: When a user's address is updated I want to send an email to them indicating changes were made while including the original address in the email.

<?phpnamespaceApp\Observers;useApp\Models\User;useIlluminate\Support\Facades\DB;useIlluminate\Support\Facades\Mail;useApp\Mail\AddressChangedEmail;class Observer{publicfunctionupdated(User$user)    {if ($user->wasChanged('address')) {$originalAddress =$user->getOriginal('address');DB::afterCommit(function()use ($user,$originalAddress) {                Mail::to([$user->email])->send(newAddressChangedEmail($user,$originalAddress));            });        }    }}

This solved the problem for me!

You must be logged in to vote
28 replies
@marius-ciclistu
Comment options

Bloating your code base with N events when you can handle all in an observer is definitelly not my first choice.

@macropay-solutions
Comment options

@JaredMezz2 You gave us another idea to optimize the observers.
image

@JaredMezz2
Comment options

@macropay-solutions That's interesting! For the base case of the default Observer event methods that is probably a little quicker, especially if you're not utilizing every available event.

Could this change potentially be less-efficient if I have several helper methods within my observer though? Ie if I had normalizeFoo(), buildPayload(), etc. etc, would these now be getting caught by the get_class_methods, loaded into memory, and result in a slightly more expensive comparison? With the original code these helper methods would never be checked.

Perhaps they'd be excluded with proper private scoping, I'm not sure whereabouts this registerObserver actuallylives and what it can see.

@macropay-solutions
Comment options

The array intersect eliminates your concern. That is better than calling method exists N times for each observer at boot time.

We are looking into event:cache now so that it can autodiscover also observers so that Model::observe does not need to be called.

@macropay-solutions
Comment options

@JaredMezz2 have a lookhttps://github.com/macropay-solutions/maravel-framework/pull/26/files
This will eliminate the execution of

publicstaticfunctionobserve($classes)    {$instance =newstatic();foreach (Arr::wrap($classes)as$class) {$instance->registerObserver($class);        }    }protectedfunctionregisterObserver($class)    {$className =$this->resolveObserverClassName($class);// When registering a model observer, we will spin through the possible events// and determine if this observer has that method. If it does, we will hook// it into the model's event system, making it convenient to watch these.foreach (\array_intersect($this->getObservableEvents(),\get_class_methods($class))as$event) {// only the below will be executed on each request.static::registerModelEvent($event,$className .'@' .$event);        }    }
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Category
Q&A
Labels
None yet
7 participants
@emil-nasso@JakubSzczesniak@yurik94@JaredMezz2@AHMED-GAMAL-AG@macropay-solutions@marius-ciclistu
Converted from issue

This discussion was converted from issue #52531 on August 22, 2024 03:40.


[8]ページ先頭

©2009-2025 Movatter.jp