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

Eloquent Model delete/deleting events are not triggered on a where(...)->delete() #2536

Closed
maximerassi opened this Issue Oct 22, 2013 · 12 comments

Comments

Projects
None yet
@maximerassi

maximerassi commented Oct 22, 2013

Scenario A
Analytic::where('id', 2)->delete();

  • this deletes the entry in the database
  • this does not trigger the deleting/deleted events on the model

Scenario B
Analytic::where('id', 2)->first()->delete();

  • this deletes the entry in the database
  • this does trigger the deleting/deleted events on the model

Is this expected behavior?

public static function boot() {
    static::deleting(function($obj) {
    });
    static::deleted(function($obj) {
    });
}

referring doc: http://four.laravel.com/docs/eloquent

@taylorotwell

This comment has been minimized.

Show comment
Hide comment
@taylorotwell

taylorotwell Oct 22, 2013

Member

Yes, this is basically expected. In order to fire the events we would have to pull the models first and then call the events with the model instances. Then call the delete query.

Member

taylorotwell commented Oct 22, 2013

Yes, this is basically expected. In order to fire the events we would have to pull the models first and then call the events with the model instances. Then call the delete query.

orrd pushed a commit to orrd/framework that referenced this issue Jan 17, 2014

orrd
Allows routes to be specified as "http".
Issue #2536 fixed an issue where URL::to()  couldn't be used to create
http URLs from an https page.  This is a fix to a similar but different
issue where URL::route() also can't be used to create routes that are
http if the current page is https.

In order to properly fix this issue with consistency for Laravel route
design, it was necessary to add an "http" route option (an "https"
option already existed, but there was no "http" option).

So with this commit, it is now possible to specify 'http' in a route
declaration, which behaves exactly as the 'https' option does, forcing
generated route URLs to use http, and only responding to http requests
for the route.

orrd pushed a commit to orrd/framework that referenced this issue Jan 17, 2014

orrd
Allows routes to be specified as 'http'.
Issue #2536 fixed an issue where URL::to() couldn't be used to create
http URLs from an https page.  This is a fix to a similar but different
issue where URL::route() can't be used to create routes to URLs that are
http if the current page is https.

In order to properly fix this issue with consistency for the current
route design, it was necessary to add an 'http' option to the route
actions (an 'https' option already existed).

So with this commit it is now possible to specify 'http' in a route
declaration, which behaves exactly as the 'https' option does, forcing
generated route URLs to use http, and only responding to http requests
for that route.
@amahrt

This comment has been minimized.

Show comment
Hide comment
@amahrt

amahrt Feb 16, 2015

Is there a workaround?

amahrt commented Feb 16, 2015

Is there a workaround?

@orrd

This comment has been minimized.

Show comment
Hide comment
@orrd

orrd Feb 16, 2015

Just to be clear, I wouldn't say you need a "workaround" since this isn't a bug, it really does make sense when you understand what's going on. The solution you need to use instead depends on what you're trying to do.

First, let's talk about their first example:

Analytic::where('id', 2)->delete();

All this actually does is execute SQL, probably something like "DELETE FROM analytic WHERE id=2". That's it. It doesn't load the models into memory, etc. For these kinds of functions, Laravel is just passing control to an underlying Query Builder query that performs the actions in SQL. That's just something that Eloquent lets you do, it lets you perform Query actions directly on its underlying query object.

In order to trigger events when you delete something, the Eloquent model would have to be loaded into memory, and then delete() has to be called on each model individually. This is an entirely different kind of operation. The question gives you an example of how to do that for one object with the ID "2":

Analytic::where('id', 2)->first()->delete();

In this case "first()" loads the model into memory, and then the delete() function is called on it. If you have more than one row/object that you want to delete at a time, you can do something like this (be careful with the where() statements, you don't want to delete things you don't want to!):

$analytics = Analytic::where('id', '>', 100)->get();
foreach ($analytics as $analytic) $analytic->delete();

Here's another way to do the same thing using Laravel's each() function that it gives you for working on Collections...

Analytic::where('id', '>', 100)->get()->each(function($analytic) {
    $analytic->delete();
});

orrd commented Feb 16, 2015

Just to be clear, I wouldn't say you need a "workaround" since this isn't a bug, it really does make sense when you understand what's going on. The solution you need to use instead depends on what you're trying to do.

First, let's talk about their first example:

Analytic::where('id', 2)->delete();

All this actually does is execute SQL, probably something like "DELETE FROM analytic WHERE id=2". That's it. It doesn't load the models into memory, etc. For these kinds of functions, Laravel is just passing control to an underlying Query Builder query that performs the actions in SQL. That's just something that Eloquent lets you do, it lets you perform Query actions directly on its underlying query object.

In order to trigger events when you delete something, the Eloquent model would have to be loaded into memory, and then delete() has to be called on each model individually. This is an entirely different kind of operation. The question gives you an example of how to do that for one object with the ID "2":

Analytic::where('id', 2)->first()->delete();

In this case "first()" loads the model into memory, and then the delete() function is called on it. If you have more than one row/object that you want to delete at a time, you can do something like this (be careful with the where() statements, you don't want to delete things you don't want to!):

$analytics = Analytic::where('id', '>', 100)->get();
foreach ($analytics as $analytic) $analytic->delete();

Here's another way to do the same thing using Laravel's each() function that it gives you for working on Collections...

Analytic::where('id', '>', 100)->get()->each(function($analytic) {
    $analytic->delete();
});
@silverdr

This comment has been minimized.

Show comment
Hide comment
@silverdr

silverdr May 12, 2015

I have something similar and still consider it somewhat confusing to say the least.

I have a typical event hook for soft deleting children:

public static function boot()
{
    parent::boot();
    static::deleted(function($model1)
    {
        $model1->hasmanyrelation()->delete();
    });
}

and

public function hasmanyrelation()
{
    return $this->hasMany('Model2');
}

Now when I use:

$model0->model1->each(function($model1){$model1->delete();});

things work as expected and model2 children of model 1 get (soft) deleted. But when I use:

$model0->model1()->delete();

then all related model1 records get deleted but model2 and all its records remain untouched.

silverdr commented May 12, 2015

I have something similar and still consider it somewhat confusing to say the least.

I have a typical event hook for soft deleting children:

public static function boot()
{
    parent::boot();
    static::deleted(function($model1)
    {
        $model1->hasmanyrelation()->delete();
    });
}

and

public function hasmanyrelation()
{
    return $this->hasMany('Model2');
}

Now when I use:

$model0->model1->each(function($model1){$model1->delete();});

things work as expected and model2 children of model 1 get (soft) deleted. But when I use:

$model0->model1()->delete();

then all related model1 records get deleted but model2 and all its records remain untouched.

@alihossein

This comment has been minimized.

Show comment
Hide comment
@alihossein

alihossein Aug 4, 2015

thank u my problem is fix

alihossein commented Aug 4, 2015

thank u my problem is fix

@rahilwazir

This comment has been minimized.

Show comment
Hide comment
@rahilwazir

rahilwazir Sep 15, 2015

@orrd Thanks for explanation!

rahilwazir commented Sep 15, 2015

@orrd Thanks for explanation!

@orrd

This comment has been minimized.

Show comment
Hide comment
@orrd

orrd Sep 15, 2015

@silverdr, yes that is the expected behavior. When you call $model0->model1()->delete(); you are only executing SQL, so your delete events never get triggered. The way Laravel relations work, $model0->model1() pretty much just gets you an Query Builder class, so you can use any of the Query Builder functions on it. For example $model0->model1()->where('foo', '=', 'bar')->get().

That's different than $model0->model1 (without parenthesis on model1). That does something completely different. That fetches your model1 objects from the database and gives them to you in a Collection. That's why when you call delete() on each one, your delete events get triggered.

Understanding the difference between $model0->model1() and $model0->model1 is the key to understanding Laravel relations (personally I didn't even notice the difference for the first year or so of using Laravel, but everything made a lot more sense once I understood that).

orrd commented Sep 15, 2015

@silverdr, yes that is the expected behavior. When you call $model0->model1()->delete(); you are only executing SQL, so your delete events never get triggered. The way Laravel relations work, $model0->model1() pretty much just gets you an Query Builder class, so you can use any of the Query Builder functions on it. For example $model0->model1()->where('foo', '=', 'bar')->get().

That's different than $model0->model1 (without parenthesis on model1). That does something completely different. That fetches your model1 objects from the database and gives them to you in a Collection. That's why when you call delete() on each one, your delete events get triggered.

Understanding the difference between $model0->model1() and $model0->model1 is the key to understanding Laravel relations (personally I didn't even notice the difference for the first year or so of using Laravel, but everything made a lot more sense once I understood that).

@ameoba32

This comment has been minimized.

Show comment
Hide comment
@ameoba32

ameoba32 Oct 1, 2015

The Law of Leaky Abstractions

ameoba32 commented Oct 1, 2015

The Law of Leaky Abstractions

@lukepolo

This comment has been minimized.

Show comment
Hide comment
@lukepolo

lukepolo Feb 18, 2016

Contributor

@really i don't see why we can fire events from the builder, since we have all the necessary functionality. For now I have extended the builder class, added this function

    protected function fireModelEvent($event, $halt = true)
    {
        if(empty(!$this->getModel())) {
              $dispatcher = $this->query->getConnection()->getEventDispatcher();

              if (empty($dispatcher)) {
                  return true;
              }

              // We will append the names of the class to the event to distinguish it from
              // other model events that are fired, allowing us to listen on each model
              // event set individually instead of catching event for all the models.
              // event set individually instead of catching event for all the models.
              $event = "eloquent.{$event}: ".get_class($this->getModel());

              $method = $halt ? 'until' : 'fire';

              return $dispatcher->$method($event, $this);
        }

    }

Then add the proper $this->fireModelEvent('deleted', false) , and $this->fireModelEvent('save', false)

Let me know what you think

Contributor

lukepolo commented Feb 18, 2016

@really i don't see why we can fire events from the builder, since we have all the necessary functionality. For now I have extended the builder class, added this function

    protected function fireModelEvent($event, $halt = true)
    {
        if(empty(!$this->getModel())) {
              $dispatcher = $this->query->getConnection()->getEventDispatcher();

              if (empty($dispatcher)) {
                  return true;
              }

              // We will append the names of the class to the event to distinguish it from
              // other model events that are fired, allowing us to listen on each model
              // event set individually instead of catching event for all the models.
              // event set individually instead of catching event for all the models.
              $event = "eloquent.{$event}: ".get_class($this->getModel());

              $method = $halt ? 'until' : 'fire';

              return $dispatcher->$method($event, $this);
        }

    }

Then add the proper $this->fireModelEvent('deleted', false) , and $this->fireModelEvent('save', false)

Let me know what you think

@antonkomarev

This comment has been minimized.

Show comment
Hide comment
@antonkomarev

antonkomarev Apr 8, 2016

Contributor

+1 for solving this issue.

Details where I found this useful.

Why it isn't working is logical, but not obvious. At least this should be documented in Query builder section.

Contributor

antonkomarev commented Apr 8, 2016

+1 for solving this issue.

Details where I found this useful.

Why it isn't working is logical, but not obvious. At least this should be documented in Query builder section.

@shivekkhurana

This comment has been minimized.

Show comment
Hide comment
@shivekkhurana

shivekkhurana Apr 11, 2016

I understand that models shall be pulled before deleting and implemented this solution but ran into another problem. My course management schema is like :

Course -< Modules -< Chapters
Chapters -< Media
Chapter -< Comments
Comments -< Votes

-< sybolizes Has Many Relation

Now I have static::deleting methods registered in Course, Module and Chapter Model, which pulls related models and calls delete method on them individually, but nginx starts to hiccup when I send a delete request to delete course.

I get the following error :

2016/04/11 11:16:34 [error] 1438#0: *104 upstream sent too big header while reading response header from upstream, client: 127.0.0.1,

There was no such error when I was deleted using Course::where('id', '=', 1)->delete() but of course the event didn't cascade, which is the expected behavior.

Why is this problem occurring and how do I solve it ??

shivekkhurana commented Apr 11, 2016

I understand that models shall be pulled before deleting and implemented this solution but ran into another problem. My course management schema is like :

Course -< Modules -< Chapters
Chapters -< Media
Chapter -< Comments
Comments -< Votes

-< sybolizes Has Many Relation

Now I have static::deleting methods registered in Course, Module and Chapter Model, which pulls related models and calls delete method on them individually, but nginx starts to hiccup when I send a delete request to delete course.

I get the following error :

2016/04/11 11:16:34 [error] 1438#0: *104 upstream sent too big header while reading response header from upstream, client: 127.0.0.1,

There was no such error when I was deleted using Course::where('id', '=', 1)->delete() but of course the event didn't cascade, which is the expected behavior.

Why is this problem occurring and how do I solve it ??

@rico-ocepek

This comment has been minimized.

Show comment
Hide comment
@rico-ocepek

rico-ocepek Sep 6, 2018

Btw in 2018 you can replace
Analytic::where('id', '>', 100)->get()->each(function($analytic) { $analytic->delete(); });

with
Analytic::where('id', '>', 100)->get()->each->delete();

rico-ocepek commented Sep 6, 2018

Btw in 2018 you can replace
Analytic::where('id', '>', 100)->get()->each(function($analytic) { $analytic->delete(); });

with
Analytic::where('id', '>', 100)->get()->each->delete();

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