Model events fired multiple times

[Fl0Cri]

Winter CMS Build

1.2

PHP Version

8.1

Database engine

MySQL/MariaDB

Plugins installed

No response

Issue description

Note: I will use afterSave as an example, but this issue applies to other events as well.

When an afterSave() method is defined in a model, each time the save action is run, afterSave() is bound to the model.afterSave event, and then model.afterSave event is fired.

Here is the code I am talking about: https://github.com/wintercms/storm/blob/54a0814cc44ac0fd2572e0da5f1dbab504979871/src/Database/Model.php#L227-L237

This means that if we call save() twice on the same model instance, afterSave() will be run once on the first time, which is expected, but twice on the second time (3 times in total).

I was about to do a PR replacing this bindEvent by bindEventOnce to solve the problem, but it may not be the best option.

Using bindEventOnce would fix this issue in most cases, but there's one I can think of where it would fail: if someone registers a handler for model.afterSave with more priority than the default, and this handler sometimes returns false.
In this case, let's imagine we save the model a first time: we bind the afterSave() method once; we fire the event; it gets cancelled by the other handler; so we don't call the afterSave() method.
Then we save a second time: we bind afterSave() again (we now have 2); we fire the event; for some reason the other handler does not return false; we call afterSave() twice.

Maybe a better approach would be to bind the afterSave() method to model.afterSave on every model instance on construction (i.e. taking it out of self::$eventMethod(...)), but it would require a bit more rewriting.
As is, it won't work because of the exit condition here. We would need to run bindEvent on every model instance, but self::$eventMethod only once per model class.

There would be one side-effect thoug (I guess it's an undefined behavior we can change, but I mention it just in case): the afterSave() method would be bound early in the object construction instead of at the time the save is made. If someone registers another model.afterSave handler with default priority, the order in which the handlers are called may change.

Let me know what you prefer. My initial guess is that using bindEventOnce requires less change and would mostly fix the issue without altering existing behavior; the other solution is less hacky but requires more changes and testing. Or maybe you have other ideas ?

Let me know if you would like a PR.

Steps to replicate

Using a model like this :

class TestModel extends Model
{
    protected $table = 'test';

    public function afterSave()
    {
        dump("afterSave");
    }
}

Demonstration using artisan tinker:

> $test = new TestModel();
>
> $test->save();
"afterSave"
>
> $test->save();
"afterSave"
"afterSave"
>
> $test->save();
"afterSave"
"afterSave"
"afterSave"
>

Workaround

Option 1: As only the afterSave model method is affected, TestModel can be adapted like this:

class TestModel extends Model
{
    protected $table = 'test';

    public function __construct(array $attributes = [])
    {
        parent::__construct($attributes);

        $this->bindEvent('model.afterSave', function () {
            dump("afterSave");
        });
    }
}

Result using artisan tinker:

> $test = new TestModel();
>
> $test->save();
"afterSave"
>
> $test->save();
"afterSave"
>
> $test->save();
"afterSave"
>

Option 2: fetch a new instance of the model between saves

> $test = new TestModel();
>
> $test->save();
"afterSave"
> $test = TestModel::findOrFail($test->id);
>
>$test->save();
"afterSave"
> $test = TestModel::findOrFail($test->id);
>

[mjauvin]

I was able to reproduce.