dimsav / laravel-translatable

[Deprecated] A Laravel package for multilingual models
MIT License
1.95k stars 320 forks source link

Model event "saving" and "updating" is not firing #456

Closed MehediDracula closed 6 years ago

MehediDracula commented 6 years ago

If a model uses Translatable trait then "saving" and "updating" event is not firing.

Gummibeer commented 6 years ago

https://github.com/dimsav/laravel-translatable/blob/27306d6/src/Translatable/Translatable.php#L231-L258

I would say that they are fired if your model is dirty. If not it's just throwing the ...ed events. In my opinion it's an abuse of these events cause the original model isn't touched. I would drop all events there cause laravel handles these events already. Depending on what is changed it will fire the events on the main and/or translation model.

MehediDracula commented 6 years ago

I've tried to update an existent product.

Product::findOrFail(1)->update($attributes);

shouldn't this fire "saving" and "updating" event? without Translatable trait it's actually firing those events.

Gummibeer commented 6 years ago

Is the original model dirty or do you just update translations? Take a look in the linked method - it just calls parent::save() if the original model does not exist or is dirty. Otherwise it just updates the translations.

MehediDracula commented 6 years ago

ok, I got it. I've actually tried to update translations that's why it's not firing those events. but IMO translation fields are part of the original model so it should also fire those events while I'm updating translations.

Gummibeer commented 6 years ago

@MehediDracula I got your point - haven't take a look in the default save method. Yes, saving should be fired in every case. But updating just if the model is dirty - this is still handled by the default save method. If your model just has dirty translations it won't fire updating but it will fire all events on the translation model.

Gummibeer commented 6 years ago

@MehediDracula this is now something philosophic.^^ There is a model that fires all the events - so I would recommend you to use these events.

MehediDracula commented 6 years ago

ok, I assume this behavior was intentional so closing it for now