Skip to content
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

forceDelete() does not work if the model has observer #54733

Closed
exemSK opened this issue Feb 20, 2025 · 9 comments
Closed

forceDelete() does not work if the model has observer #54733

exemSK opened this issue Feb 20, 2025 · 9 comments

Comments

@exemSK
Copy link

exemSK commented Feb 20, 2025

Laravel Version

11.31

PHP Version

8.4.3

Database Driver & Version

No response

Description

The forceDelete() method does not delete a model when it has an observer. The method returns true and updates the model's created_at timestamp to the current date and time. You must use forceDeleteQuietly() to delete a model.

Steps To Reproduce

Model:

#[ ObservedBy( [ DemandObserver::class ] ) ]
class Demand extends Model
{
    use SoftDeletes;

    ......
}

Controller:


public function deletePermanently( Demand $demand )
    {
        if ( auth()->user()->can(  'forceDelete',  $demand  )  ){

            $deleted = $demand->forceDelete();

            return $deleted ? back()->with(  'success', 'The demand has been permanently removed.' ) : back()->withErrors( [ 'error' => 'The demand was not deleted.' ] );
        }

        return back()->withErrors( [ 'error' => 'You do not have permission to perform this operation.' ] );
    }

Firing the deletePermanently() function return 'success', but the model is not deleted and created_at timestamp IS UPDATED.

@crynobone
Copy link
Member

Hey there, thanks for reporting this issue.

We'll need more info and/or code to debug this further. Can you please create a repository with the command below, commit the code that reproduces the issue as one separate commit on the main/master branch and share the repository here?

Please make sure that you have the latest version of the Laravel installer in order to run this command. Please also make sure you have both Git & the GitHub CLI tool properly set up.

laravel new bug-report --github="--public"

Do not amend and create a separate commit with your custom changes. After you've posted the repository, we'll try to reproduce the issue.

Thanks!

@rodrigopedra
Copy link
Contributor

rodrigopedra commented Feb 21, 2025

I made a test app, and could not replicate it.

Can you share your observer's code?

More specifically, any forceDeleting and forceDeleted methods.

If your observer has a forceDeleting method that returns false, then the object won't be deleted, as any -ing events are fired before the action is actually performed, and if they return false then the action is not performed.

If you want to execute code on your observer after the action is actually performed, use the -ed methods.

That would explain why Model@forceDeleteQuietly() works, as it doesn't fire any events, and therefore none of the observer's methods are called, and thus checking for false on the forceDeleting event is skipped.

@exemSK
Copy link
Author

exemSK commented Feb 21, 2025

Here is my observer code:


namespace App\Observers;

use App\Models\Demand;

class DemandObserver
{
    /**
     * Handle the Demand "created" event.
     */
    public function created(Demand $demand): void
    {
        //
    }

    /**
     * Handle the Demand "updated" event.
     */
    public function updated(Demand $demand): void
    {
        //
    }

    /**
     * Handle the Demand "deleted" event.
     */
    public function deleted( Demand $demand ): void
    {
        $demand->deleted_by = auth()->user()->id;
        $demand->save();

        $demand->logs()->create([
            'details' => [
                "text" => 'The demand has been trashed by ' . auth()->user()->name . '.'
            ]
        ]);

    }

    /**
     * Handle the Demand "restored" event.
     */
    public function restored(Demand $demand): void
    {
        $demand->deleted_by = auth()->user()->id;
        $demand->save();

        $demand->logs()->create([
            'details' => [
                "text" => 'The demand has been restored by ' . auth()->user()->name . '.'
            ]
        ]);
    }
}

@rodrigopedra
Copy link
Contributor

@exemSK I made the public repo as @crynobone suggested, and still cannot replicate it:

https://github.com/rodrigopedra/issue-54733

Please review the code, and either fork and share the link, or send a PR to the repo above - with the changes needed to replicate - so we can review it.

You can also search your code base for any listeners of a forceDeleting event that might be registered somewhere else outside the observer.

@macropay-solutions
Copy link

macropay-solutions commented Feb 21, 2025

@exemSK This is not a bug but just another corner case of a happy flow from Laravel.

your code created it again after deletion

    /**
     * Handle the Demand "deleted" event.
     */
    public function deleted( Demand $demand ): void
    { // here $deleted is hydrated but does not exist in db. Try here dd(Demand::query()->find($demand->id)); It will print null
        $demand->deleted_by = auth()->user()->id;
        $demand->save(); // here you create it again. 

        $demand->logs()->create([
            'details' => [
                "text" => 'The demand has been trashed by ' . auth()->user()->name . '.'
            ]
        ]);

    }

fix

    /**
     * Handle the Demand "deleted" event.
     */
    public function deleting( Demand $demand ): void
    { 
        $demand->deleted_by = auth()->user()->id;
    }

    /**
     * Handle the Demand "deleted" event.
     */
    public function deleted( Demand $demand ): void
    { 
        $demand->logs()->create([
            'details' => [
                "text" => 'The demand has been trashed by ' . auth()->user()->name . '.'
            ]
        ]);
    }


    /**
     * Handle the Demand "deleted" event.
     */
    public function forceDeleted( Demand $demand ): void
    { 
        $demand->logs()->create([
            'details' => [
                "text" => 'The demand has been forceDeleted by ' . auth()->user()->name . '.'
            ]
        ]);
    }

@rodrigopedra
Copy link
Contributor

@macropay-solutions good catch!

Alternative fix if the Demand model uses the SoftDeleted trait, and you want to perform a more convoluted logic on the observer's deleted method:

public function deleted(Demand $demand): void
{
    // `Model@isForceDeleting()` is only available if the `Demand` model 
    // uses the `SoftDeletes` trait
    if (! $demand->isForceDeleting())
    {
        // additional logic

        $demand->deleted_by = auth()->user()->id;
        $demand->save();
    }

    // this might fail if the `logs` relation has a foreign key
    // as the force deleted record will no longer be in the database
    $demand->logs()->create([
        'details' => [
            "text" => 'The demand has been trashed by ' . auth()->user()->name . '.',
        ],
    ]);
}

@macropay-solutions
Copy link

macropay-solutions commented Feb 21, 2025

@rodrigopedra We updated the original response with a fix. Your proposal makes an extra save.

@exemSK
Copy link
Author

exemSK commented Feb 22, 2025

Ok, thanks for your time and response. My mistake, I mistakenly assumed that if the model uses the SoftDeletes trait, the deleting and deleted events will only be fired if softDelete() is called and when forceDelete() is called, only the forceDeleting and forceDeleted events will be fired

@macropay-solutions
Copy link

@exemSK you can close this issue.

@exemSK exemSK closed this as completed Feb 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants