Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.7] Sync changes before firing updated event #25026

Closed
wants to merge 2 commits into from

Conversation

Dylan-DutchAndBold
Copy link
Contributor

! Contains breaking change

Problem

  1. Logic: Since the updated events suggests the model is now saved (which it is). We should be using the ->wasChanged function instead of the ->isDirty functions within this function.

  2. Unwanted behaviour: In my case I now check if a value is dirty and then save another. But it will create an infinite loop when you save the model, because the dirty values are still there when you update again. Instead of just that second new value.

Code example

This is my code example which creates an infinite loop, which it wouldn't when using ->wasChanged.

HasLocationObserver

public function updated(HasLocation $model)
    {
        if ($model->needsLocationUpdate()) {
            FetchLatLong::dispatchNow($model);
        }
    }

Model function ->needsLocationUpdate()

public function needsLocationUpdate(): bool
    {
        return $this->isDirty(['city', 'country_code']);
    }

The FetchLatLong job sets the spatial point value and saves again. But since the dirty values from the previous update still are set on the model, it will always return true on the ->needsLocationUpdate function

1. Logic: Since the updated events suggests the model is now saved (which it is). We should be using the `->wasChanged` function instead of the `->isDirty` functions within this function.
2. Unwanted behaviour: In my case I now check if a value is dirty and then save another. But it will create an infinite loop when you save the model, because the dirty values are still there when you update again. Instead of just that second new value.
@taylorotwell
Copy link
Member

Merged to 5.7 branch.

@taylorotwell
Copy link
Member

taylorotwell commented Aug 6, 2018

What exactly is the breaking change with this method? It seems more like a bug fix? Would you expect this to have a large impact on existing applications?

@Dylan-DutchAndBold
Copy link
Contributor Author

@taylorotwell If someone has been using isDirty function within the updated model event, it will no longer work. It should now be swapped with wasChanged

@andrewsuzuki
Copy link

@taylorotwell I think the upgrade guide should mention isDirty no longer working in updated model events. Of the two updated model events I have in my app, I was using isDirty in both of them (watching for changes to User->email and updating third-party services).

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.

3 participants