WithoutEvents static method on class will overwrite listeners defined in the boot method of a model.

scorgn commented 4 years ago

Laravel Version: Lumen 5.8.35 (also with non-lumen versions)
PHP Version: 7.3.15
Database Driver & Version: sqlite

Description:

In one of the services I am building, all of the models use a trait that use the boot() method to add a UUID onto the model on creating(). Without this UUID, the model would not be able to be inserted into the database. In my seeders, I have disabled event listeners. As expecting, this creating() observer did not fire, but in seeders it was fine because I just added it in there.

The issue came up after the seeder was ran. It looked like if I use withoutEvents(...) in the seeder, then even model observer events outside of the callable argument do not become triggered.

I dug deep and wide to figure out why this was happening, and I finally figured it out.

The first thing the withoutEvents(...) method will do is copy the dispatcher on the model. Since this is ran before the model class is booted, the created model event listener which is generated in the boot method of the model is not in the copied dispatcher. Inside the callable, it is booting the method and attempting to observe the created event. After all is said and done, the dispatcher (which doesn't have the created observer) is put back onto the model. Subsequent usages of the model do not trigger the boot method and the model will now never have the created observer defined there.

Steps To Reproduce:

Create model
Create trait with a boot() method that sets a model observer (self::creating(function ($model) { ... })
Add trait to model
Write seeder that creates model inside a Model::withoutEvents(function() { ... }) callable
Write test that first calls that seeder, then tries to create a model
Observe that the observer in the boot method of the model is not triggered.

Suggestion for fixing

I added static::boot() in the beginning of the withoutEvents() method before copying the dispatcher, and everything worked. I recommend this as a fix.

taylorotwell commented 4 years ago

Do you want to submit a PR for review?

nmokkenstorm commented 4 years ago

If @ShawnCorrigan does not want to submit a PR I'm happy to give it a shot

derekmd commented 4 years ago

I added static::boot() in the beginning of the withoutEvents() method before copying the dispatcher, and everything worked. I recommend this as a fix.

The full Model boot lifecycle, including events 'booting' and 'booted' would have to be run.

v6.x branch

https://github.com/laravel/framework/blob/1b0bdb43062a2792befe6fd754140124a8e4dc35/src/Illuminate/Database/Eloquent/Model.php#L180-L191

v7.x branch

Laravel 7.x added new booting() and booted() event hooks.

https://github.com/laravel/framework/blob/2d62466ed9cfaebd852ba363721c1cf5e35aba26/src/Illuminate/Database/Eloquent/Model.php#L182-L195

Unable to `Model::boot()` ahead of time in `withoutEvents()`

The static Model::withoutEvents() method doesn't have an instance of the model however the bootIfNotBooted() method requires one to dispatch all boot events.
Model::withoutEvents() temporarily removes the event dispatcher for all Eloquent Model-extending classes.

withoutEvents() would need to instantiate and pre-boot every Eloquent Model referenced in that Closure. e.g.,

Comment::creating(function ($comment) {
   $comment->uuid = Str::uuid()->toString();
});

$commenter = User::withoutEvents(function () {
   $user = factory(User::class)->create();
   factory(Comment::class, 4)->create(['user_id' => $user->id]);
   factory(Avatar::class)->create(['user_id' => $user->id]);

   return $user;
});

User, Comment, and Avatar would all need the boot lifecycle. withoutEvents() can't know ahead of time that those models will be referenced in the Closure.

The workaround I can see is introducing a new Model property (similar to Model::$booted) that tracks which classes were booted without the event dispatcher.

protected static $bootedWithoutEvents = [];

// ....

protected function bootIfNotBooted() 
{ 
    if (! isset(static::$booted[static::class])) {
        // ...

        if (! isset(static::$dispatcher))  {
            static::$bootedWithoutEvents[static::class] = true;
        }
    }
}

Then withoutEvents() could "unboot" models once it completes. ... but the problem with that approach is userland can register boot behavior that has absolutely nothing to do with event dispatchers. So this solution would allow boot hooks to be called twice: once without an event dispatcher, another with the dispatcher.

Delay `Model::registerModelEvent()` calls until after `withoutEvents()`

Another approach could be to cache Model::registerModelEvent() callbacks when registered within withoutEvents(). They could be restored at the end of withoutEvents() after static::setEventDispatcher($dispatcher) is called.

EDIT: Here is an example implementation where Model@registerModelEvent() is intercepted. It requires introducing two new properties to Illuminate\Database\Eloquent\Model, which isn't ideal.

Clarify behavior in documentation

The most straight-forward solution might be to add a callout alert in the docs that note to keep custom event listeners you must new up model classes before calling withoutEvents(). https://laravel.com/docs/7.x currently doesn't have any documentation about how the withoutEvents() method behaves.

nmokkenstorm commented 4 years ago

The workaround I can see is introducing a new Model property (similar to Model::$booted) that tracks which classes were booted without the event dispatcher.

Love this approach personally, but as you mention it does add more API surface to a class that's already often criticized for being big.

The most straight-forward solution might be to add a callout alert in the docs that note to keep custom event listeners you must new up model classes before calling withoutEvents(). https://laravel.com/docs/7.x currently doesn't have any documentation about how the withoutEvents() method behaves.

Documenting the current behaviour is probably a good thing but it might also invite more people to use this functionality, and withoutEvents is something a developer should hesitate to reach for in my opinion.

E: thanks for the clarifying research by the way!

derekmd commented 4 years ago

Implementation attempt 2: https://github.com/laravel/framework/compare/6.x...derekmd:without-events-registers-listeners

This is lightweight on the Eloquent Model class and no object properties are added. A null pattern is used on the dispatcher to do nothing when a fired event is attempted. By proxying the concrete Illuminate\Events\Dispatcher, listener registrations can still be done when inside Model::withoutEvents().

The NullDispatcher code is kind of gnarly because PHP interfaces don't allow falling back to a proxy __call(). You must explicitly define each method in the contract.

driesvints commented 4 years ago

@derekmd looks good to me. Can you send that in? Thanks!

laravel / framework