Possible bug when saving an existing model that has updating event

[DePalmo]

Before I file a bug report, wanted to discuss if I found it in the Laravel Framework.

It all started when I was using ActivityLog, which created a lot of "ghost" entries. I call those where the update is being done to the model and it's being logged, but nothing is changed. After I debugged a lot, this is what I have found.

I have a model and related model which uses updating and updated events. For this case, updating is important. When a related model is saved, the updating event causes some processing and it changes the model that's being saved. Basically, when the data comes in, it's different than the one already in the database but then the updating event changes it to match the one already saved in database (we have a business model that works like this). But this causes the isDirty to be true and it triggers the save event, which then updates updated_at and ActivityLog creates new entry.

Why do I think it's a bug? Because the updating event is triggered too late or when actually saving a model, the dirtiness is checked too late. This is the flow of the current code.

1. in Controller, I am calling a $model->relatedModel()->updateOrCreate(['id' => 1], $attributes) which is located in laravel\framework\src\Illuminate\Database\Eloquent\Relations\HasOneOrMany.php:L268
2. this calls $instance->save() on line 273, which is located in laravel\framework\Illuminate\Database\Eloquent\Model.php:L1113
3. this method then decides if the model already exists or it should create it. In my case, it already exists and then it checks if the model is dirty. At this point it is, since the data received is different than already stored in the database (or in original model, which was pulled from the database)
4. this then calls $this->performUpdate($query) on line 1131, which is then located in same file at line 1192
5. the performUpdate(Builder $query) then fires the updating event, updates the timestamps and checks if the model is dirty again. Because of updating event here, the model's attributes were set to the same as they were already in the database, but because of the timestamp check, model is now dirty and it saves it

The current performUpdate(Builder $query) looks like this:

    protected function performUpdate(Builder $query)
    {
        // If the updating event returns false, we will cancel the update operation so
        // developers can hook Validation systems into their models and cancel this
        // operation if the model does not pass validation. Otherwise, we update.
        if ($this->fireModelEvent('updating') === false) {
            return false;
        }

        // First we need to create a fresh query instance and touch the creation and
        // update timestamp on the model which are maintained by us for developer
        // convenience. Then we will just continue saving the model instances.
        if ($this->usesTimestamps()) {
            $this->updateTimestamps();
        }

        // Once we have run the update operation, we will fire the "updated" event for
        // this model instance. This will allow developers to hook into these after
        // models are updated, giving them a chance to do any special processing.
        $dirty = $this->getDirty();

        if (count($dirty) > 0) {
            $this->setKeysForSaveQuery($query)->update($dirty);

            $this->syncChanges();

            $this->fireModelEvent('updated', false);
        }

        return true;
    }

But I think it should look like this:

    protected function performUpdate(Builder $query)
    {
        // If the updating event returns false, we will cancel the update operation so
        // developers can hook Validation systems into their models and cancel this
        // operation if the model does not pass validation. Otherwise, we update.
        if ($this->fireModelEvent('updating') === false) {
            return false;
        }

        // Once we have run the update operation, we will fire the "updated" event for
        // this model instance. This will allow developers to hook into these after
        // models are updated, giving them a chance to do any special processing.
        $dirty = $this->getDirty();

        if (count($dirty) > 0) {
            // First we need to create a fresh query instance and touch the creation and
            // update timestamp on the model which are maintained by us for developer
            // convenience. Then we will just continue saving the model instances.
            if ($this->usesTimestamps()) {
                $this->updateTimestamps();
            }

           $this->setKeysForSaveQuery($query)->update($dirty);

            $this->syncChanges();

            $this->fireModelEvent('updated', false);
        }

        return true;
    }

The updated function sets timestamps ONLY if it finds dirty attributes and not before. This way the update happens only if the model, changed by updating event, is actually still dirty and needs to be updated in the database.

To be fair, I haven't tested this in clean installation, but I never change the vendor files, so I assume it will work the same there. I also assume to be sure, I do need to provide more code so that this potential bug can quickly be checked. But before I do that, what do you guys think about this? I understand why the updating event is triggered in performUpdate function and that it needs to stay there, but why create timestamps before checking the dirtiness of the model again? Isn't it logical that if we do so, that the model will always be dirty?

[ghost]

An update is still an update. If you call update it means you want to update. The fact that you are updating only your updated_at because the rest of the data is the same is a corner case which you could handle by not calling updateOrCreate. It could be a breaking change if others are depending on this.
You could create 2 macros with your code and solve the issue: updateIfNeededOrCreate, performUpdateIfNeeded

[DePalmo]

To make it clear, I'm not updating only updated_at as you said, I am trying to avoid it when there's no model change after updating event updated the data for it. Bottom line: I request the Laravel to update one field, but because it's dependent on another field, the updating method restores the first field to as it was, so nothing should be changed. But it is, the updated_at.

I am checking the documentation since I clearly recall that if no attributes have been changed, it won't update the updated_at column as well. But surprisingly, I can't find that part of the documentation anywhere in official docs, only on other site through Google. Some claim that if you do not update the model and call ->save() on it, nothing will happen. And if you want to update updated_at, you need to call ->touch() function.

Will re-create the scenarios when I have more time and get back to this one. I still think there's something not correct, but want to be sure if it's with my understanding of the framework or with my code.

[ghost]

Maybe you are referring to

    // If the model already exists in the database we can just update our record
    // that is already in this database using the current IDs in this "where"
    // clause to only update this model. Otherwise, we'll just insert them.
    if ($this->exists) {
        $saved = $this->isDirty() ?
                    $this->performUpdate($query) : true;
    }

from \Illuminate\Database\Eloquent\Model::save