martijnb92 commented 4 years ago

What happened

My model contains a belongsTo relationship, when saving/updating an entry it fires the saving/saved observer for every belongsTo relationship. Is this intended behaviour?

This code is causing the additional save events:

Backpack\CRUD\app\Library\CrudPanel\Traits; private function createRelationsForItem($item, $formattedData)

   if ($relation instanceof BelongsTo) {
            $modelInstance = $model::find($relationData['values'])->first();
            if ($modelInstance != null) {
                $relation->associate($modelInstance)->save();
            } else {
                $relation->dissociate()->save();
            }

What I expected to happen

The saved/saving events to be firing only once per new entry.

Backpack, Laravel, PHP, DB version

Laravel 7.4 PHP 7.4.1 Backpack 4.0.58

Might be related to:

235

dividy commented 3 years ago

I'm having the same problem. Saving event firing 5 times.

@martijnb92 did you find a solution?

@pxpm @tabacitu any clue on this?

dividy commented 3 years ago

The only solution I found is using the $model::withoutEvents closure around the save() : https://laravel.com/docs/8.x/eloquent#muting-events

This code works fine and triggers the Saving event only once : (in Backpack\CRUD\app\Library\CrudPanel\Traits\Create)

if ($relation instanceof BelongsTo) {
                $modelInstance = $model::find($relationData['values'])->first();
                if ($modelInstance != null) {
                    $model::withoutEvents(function () use ($relation, $modelInstance) {
                        $relation->associate($modelInstance)->save();
                    });
                } else {
                    $model::withoutEvents(function () use ($relation) {
                        $relation->dissociate()->save();
                    });
                }
            } elseif ($relation instanceof HasOne) {
                if ($item->{$relationMethod} != null) {
                    $item->{$relationMethod}->update($relationData['values']);
                    $modelInstance = $item->{$relationMethod};
                } else {
                    $modelInstance = new $model($relationData['values']);
                    $model::withoutEvents(function () use ($relation, $modelInstance) {
                        $relation->save($modelInstance);
                    });
                }
            }

...or maybe, since Laravel 8, using $model->saveQuietly() could solve this..

tabacitu commented 3 years ago

@dividy what Backpack version are you using? What's the output of your php artisan backpack:version?

dividy commented 3 years ago

PHP VERSION:

LARAVEL VERSION:

v7.19.0@67d6d9688122dcf6c449b74ddc873fc9934e81e6

BACKPACK VERSION:

4.1.13@9beb79f0e80f9a525e8d4202affd56bfacfce8ed

tabacitu commented 3 years ago

Thanks again @dividy . I've gotten to investigate this a little bit.

A little background history: This is a change that has happened in Laravel itself, at their version 5.8. Until that point, events where NOT triggered for the main model, when modifying a relationship to it using attach() / detach() or sync(). Since then, they are. The 5.8 upgrade guide mentions it briefly. It's debatable if that's a good move or not on Laravel's part. But, whether we like it or not, that's the behaviour, in Laravel 5.8, 6, 7 and 8.

In this case though... it's a little tricky:

on the one hand, the operation will trigger the saving() and saved() events anyway, when modifying the main item; so why trigger them multiple times? at least at first glance;
on the other hand, what if someone uses the saving() or saved() events specifically to do stuff if the main item has relations, or depending on what relations are attached/detached/synced? In that case, us saving relationships without triggering the events would be super-unintuitive. They wouldn't understand why their code isn't working, since they'd expect the events to be triggered;

I don't know which way to go with this... Usually I believe we should stick to how Laravel triggers events as much as possible. Because that's what's intuitive, for things to happen the way Laravel normally does them. So I'm leaning towards keeping the events triggered, even multiple times...

What's your view on the dilemma above @dividy ? @martijnb92 ? @pxpm ? What's the "lesser evil" in your eyes?

(A) for the Create operation to trigger the events multiple times, once for the main item, then once for each successfully saved relationship;
(B) for the Create operation to NOT trigger the events for the relationships, and only trigger it once (which incidentally is before all relationships get saved, so at the time of the event no relationships are present, so you can't do stuff with the relationships inside the event)

Note: I guess there would be a third option here, option (C), where we save the main item quietly too (no events), and then trigger the saved() event at the end... but then what happens with the saving() event? We trigger it before we do anything? At this point the events and order would make sense from our point of view (trigger saving, save attributes, save relationships, trigger saved) but... it would be completely different from how they're triggered anywhere else where the entry is edited (inside the main app), and different from the way Laravel users expect them to be triggered. Right?! Idk...

Tricky...

dividy commented 3 years ago

Sorry I didn't mention it has been implemented in 5.8... And yes, that's exactly what I was explaining to one of my colleague yesterday.., It's kinda tricky.

You're right, it's better to stick to the way Laravel is working.

I will be looking for a solution later this day.

tabacitu commented 3 years ago

Thanks for the fast reply @dividy . Out of curiosity - how is this an inconvenience/problem to you? The fact that the events are being fired multiple times.

dividy commented 3 years ago

I'm inserting a record in another table. (and ofc, the record is inserted multiple times)

martijnb92 commented 3 years ago

I'm inserting a record in another table. (and ofc, the record is inserted multiple times)

I have the same problem, inserting a record in another table. Which should be done only once. I solved this problem by storing a variable on the model. The observer method checks whether the variable already exists before it calls the function. Quick and easy solution and no need to overwrite default Backpack functionality.

dividy commented 3 years ago

Yes, a flag is the way to go I guess... 👍

tabacitu commented 3 years ago

Oh yeah... I understand now. Thanks @martijnb92 & @dividy . This only strengthens my earlier opinion, that we should stick to the way Laravel does stuff and keep the events triggered multiple times.

Because, if you didn't see that the events get triggered multiple times by Backpack, you might not have noticed that Laravel 5.8+ triggers the events upon relationship change. And you'd still have the problem, but only when the Model is created from the front-end (not from the admin panel). And then you'd still have to use this solution - just that it wouldn't be needed by the admin panel.

Ok, great! 😄 Let's table the issue then. The Model should work the same whether used from a CrudController or from a regular Controller or ServiceProvider. So we keep triggering the saving and saved events when relationships are created/updated, just like our lord and saviour Laravel intended. Great!

Looks like we have consensus so I won't go further. If anybody else reads this and disagrees please feel free to reply with arguments why this isn't a good idea. We're open to reopening this.

Cheers!

Laravel-Backpack / CRUD

[4.1.x] Model events (saving and saved) are triggered multiple times, once when the main item is saved and once for each time a relationships is attached/detached/synced #2656

What happened

What I expected to happen

Backpack, Laravel, PHP, DB version

235

PHP VERSION:

LARAVEL VERSION:

BACKPACK VERSION: