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

[martijnb92]

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]

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

@martijnb92 did you find a solution?

@pxpm @tabacitu any clue on this?

[dividy]

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\TraitsCreate)

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]

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

[dividy]

PHP VERSION:

PHP 7.2.19 (cli) (built: May 29 2019 13:58:59) ( ZTS MSVC15 (Visual C++ 2017) x64 )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.2.0, Copyright (c) 1998-2018 Zend Technologies

LARAVEL VERSION:

v7.19.0@67d6d9688122dcf6c449b74ddc873fc9934e81e6

BACKPACK VERSION:

4.1.13@9beb79f0e80f9a525e8d4202affd56bfacfce8ed

[tabacitu]

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]

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]

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]

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

[martijnb92]

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]

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

[tabacitu]

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!