laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.23k stars 10.92k forks source link

Intended behaviors of Model::getOriginal() and Model::getChanges()? #28711

Closed manierim closed 5 years ago

manierim commented 5 years ago

In my codebase I'm using a model observer to collect attribute changed during updates. In the observer updated() method I pass the model instance to a notification that then uses Model::getChanges() and Model::getOriginal() to report both the new values as well as the previous ones.

The code works flawlessly in normal app execution (changes triggered by an incoming http request) as well as in Feature tests using the post() method. Not only for simple attributes but also for attributes being foreign keys for relations updated via the relation::associate() method.

Trying to put the notification class under Unit test I'm seeing weird results from Model::getOriginal(). While getChanges() correctly shows the attributes changed before a model::save() call after changing attributes' values, getOriginal() seems to give back the updated attributes' values both in the TestCase code as well as in the notification under test code that produces wrong results.

I've also tried reproducing the behavior in tinker and the getOriginal() method keeps returning the current values after a ::save() call.

My impression is that while getChanges() correctly keep tracks of the changes during the entire event cycle of the model update, getOriginal() only gives back the actual original values before the end of the update event chain, surely after the updated event.

Is this intended behavior? Shouldn't getOriginal() track the original attributes' values until a sync() is explicitly called on the instance?

staudenmeir commented 5 years ago

Can you provide some sample code that shows these behaviors?

manierim commented 5 years ago

Quick test with Sqlite:

Migration:

        Schema::create('anythings', function (Blueprint $table) {
            $table->bigIncrements('id');
            $table->text('text');
            $table->unsignedSmallInteger('qty');
            $table->timestamps();
        });

Observer:

<?php

namespace App\Observers;

use App\Anything;

class AnythingObserver
{
    public function updated(Anything $anything)
    {
        dump([
            'observer.updated' => [
                'getChanges'    => $anything->getChanges(),
                'getOriginal'   => $anything->getOriginal(),
            ]
        ]);
    }
}

In Tinker:

$instance = new App\Anything()
$instance->text = 'original qty 1';
$instance->qty = 1;
$instance->save();

$instance->text = 'changed qty 2';
$instance->qty = 2;
$instance->save();

dump from the observer:

array:1 [                                  
  "observer.updated" => array:2 [          
    "getChanges" => array:3 [              
      "text" => "changed qty 2"            
      "qty" => 2                           
      "updated_at" => "2019-06-03 22:48:20"
    ]                                      
    "getOriginal" => array:5 [             
      "text" => "original qty 1"           
      "qty" => 1                           
      "updated_at" => "2019-06-03 22:48:04"
      "created_at" => "2019-06-03 22:48:04"
      "id" => 78                           
    ]                                      
  ]                                        
]                                          

Then again in tinker:

>>> $instance->getChanges();               
=> [                                       
     "text" => "changed qty 2",            
     "qty" => 2,                           
     "updated_at" => "2019-06-03 22:48:20",
   ]                                       
>>> $instance->getOriginal()               
=> [                                       
     "text" => "changed qty 2",            
     "qty" => 2,                           
     "updated_at" => "2019-06-03 22:48:20",
     "created_at" => "2019-06-03 22:48:04",
     "id" => 78,                           
   ]                                       
>>>                                        
driesvints commented 5 years ago

Hi there,

Thanks for reporting but it looks like this is a question which can be asked on a support channel. Please only use this issue tracker for reporting bugs with the library itself. If you have a question on how to use functionality provided by this repo you can try one of the following channels:

However, this issue will not be locked and everyone is still free to discuss solutions to your problem!

Thanks.

manierim commented 5 years ago

Well if the reported behavior is not intended it is a bug...

The two methods are not documented so I guessed that community support could not reliably answer on this..

manierim commented 5 years ago

by the way the behavior is caused by the model::finishSave() method:

    /**
     * Perform any actions that are necessary after the model is saved.
     *
     * @param  array  $options
     * @return void
     */
    protected function finishSave(array $options)
    {
        $this->fireModelEvent('saved', false);

        if ($this->isDirty() && ($options['touch'] ?? true)) {
            $this->touchOwners();
        }

        $this->syncOriginal();
    }

The call to syncOriginal():

    /**
     * Sync the original attributes with the current.
     *
     * @return $this
     */
    public function syncOriginal()
    {
        $this->original = $this->attributes;

        return $this;
    }
staudenmeir commented 5 years ago

Do you mean that $original should still contain "original qty 1" etc. after the update?

manierim commented 5 years ago

Since getChanges() still shows the changed attributes after the update, my guess was that also getOriginal() - and therefore this->original - should have kept them.

But I see why it is not the case. $this->original is also used by getDirty to check if a save() needs to actually write to the DB.

I'm going to find another solution to unit test my Notification. Thanks all!

mhfereydouni commented 1 year ago

I also think it is a bug and syncing original data should be called before firing model events that end with '-ed' like ModelUpdated.

matthiaseigner commented 10 months ago

We also ran into this problem. Is there a plan to fix this in the near future or is this intended behaviour?

n8rowley commented 7 months ago

It looks like this is the "expected" behavior. See this comment from Taylor on it's use #39943. Basically getOriginal is used before changes have been persisted. I was able to get around this issue by tapping into the Model::updating instead of updated. Here getOriginal works as expected. I then had to use getDirty instead of getChanges to see what was being modified.