New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[L5] The disable_event options doesn't disable model events #2897

Closed
Anahkiasen opened this Issue Mar 12, 2016 · 5 comments

Comments

Projects
None yet
2 participants
@Anahkiasen

Anahkiasen commented Mar 12, 2016

The Laravel5 module has a disable_events which does this:

$this->app->instance('events', $mock);

However that doesn't disable model events, as those are fired through the dispatcher that is statically set on the Illuminate\Database\Eloquent\Model class. The following works:

$this->app->instance('events', $mock);
Model::setEventDispatcher($mock);

@Anahkiasen Anahkiasen changed the title from The disable_event options doesn't disable event options to [L5] The disable_event options doesn't disable model events Mar 12, 2016

@janhenkgerritsen janhenkgerritsen self-assigned this Mar 12, 2016

@janhenkgerritsen

This comment has been minimized.

Show comment
Hide comment
@janhenkgerritsen

janhenkgerritsen Mar 12, 2016

Contributor

I will look into this next week, thanks for reporting!

Contributor

janhenkgerritsen commented Mar 12, 2016

I will look into this next week, thanks for reporting!

@janhenkgerritsen

This comment has been minimized.

Show comment
Hide comment
@janhenkgerritsen

janhenkgerritsen Jul 4, 2016

Contributor

The code you provide will indeed ensure that model events are disabled when the disable_events configuration option is set to true. However, I am not sure if that is the right thing to do.

My thinking is that you use normal events for things like sending email notifications after registering a new user, but that you use model events for things like data validation and updating related models. In my opinion model events are conceptually at a lower level. So that's why I am not so sure that disable_events should also disable model events.

A solution would be to also add an option disable_model_events, that way users have more control over which events are disabled.

What are your thoughts on this?

Contributor

janhenkgerritsen commented Jul 4, 2016

The code you provide will indeed ensure that model events are disabled when the disable_events configuration option is set to true. However, I am not sure if that is the right thing to do.

My thinking is that you use normal events for things like sending email notifications after registering a new user, but that you use model events for things like data validation and updating related models. In my opinion model events are conceptually at a lower level. So that's why I am not so sure that disable_events should also disable model events.

A solution would be to also add an option disable_model_events, that way users have more control over which events are disabled.

What are your thoughts on this?

@Anahkiasen

This comment has been minimized.

Show comment
Hide comment
@Anahkiasen

Anahkiasen Jul 4, 2016

I'm ok with that, what would be its default though? Would it be the value of disable_events or just false?

Anahkiasen commented Jul 4, 2016

I'm ok with that, what would be its default though? Would it be the value of disable_events or just false?

@janhenkgerritsen

This comment has been minimized.

Show comment
Hide comment
@janhenkgerritsen

janhenkgerritsen Jul 4, 2016

Contributor

I am leaning to just false, that way you have to explicitly disable the model events. Of course the documentation of the disable_events configuration option should make clear that it does not disable model events and that you need to use the disable_model_events option for that.

Contributor

janhenkgerritsen commented Jul 4, 2016

I am leaning to just false, that way you have to explicitly disable the model events. Of course the documentation of the disable_events configuration option should make clear that it does not disable model events and that you need to use the disable_model_events option for that.

@Anahkiasen

This comment has been minimized.

Show comment
Hide comment
@Anahkiasen

Anahkiasen Jul 4, 2016

Ok! Sounds good to me

Anahkiasen commented Jul 4, 2016

Ok! Sounds good to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment