Skip to content

Add "trashed" event to $observables in SoftDeletes Trait #54987

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

Closed
wants to merge 2 commits into from

Conversation

duemti
Copy link
Contributor

@duemti duemti commented Mar 12, 2025

Added "trashed" event to $observables array so that subscribed Observers can catch the event as Expected!

Added "trashed" event to $observables array so that subscribed Observers can catch the event.
Copy link
Contributor Author

@duemti duemti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should work

@duemti duemti changed the title Add $observables to SoftDeletes Trait Add "trashed" event to $observables in SoftDeletes Trait Mar 12, 2025
@taylorotwell
Copy link
Member

I am closing this pull request because it lacks sufficient explanation, tests, or both. It is difficult for us to merge pull requests without these things because the change may introduce breaking changes to the framework.

Feel free to re-submit your change with a thorough explanation of the feature and tests - integration tests are preferred over unit tests. Please include it's benefit to end users; the reasons it does not break any existing features; how it makes building web applications easier, etc.

Thanks!

@duemti
Copy link
Contributor Author

duemti commented Mar 12, 2025

Sorry, you are right!
It is a fix for #54980

I don't think it creates any breaking changes because it uses addObservableEvents() which just populates the $observables array property. The property is described as "User exposed observable events. These are extra user-defined events observers may subscribe to." so we not even reset it but just adding this new event to it (even though it is probably empty).

The reason to add "trashed" event to it in SoftDeletes Trait initialization function is because on soft deletion of a model the "trashed" event is dispatched BUT the user will not be able to catch it with an observer because it is not registered.

Here is part of the code responsible for registering observable events:

/**
     * Get the observable event names.
     *
     * @return array
     */
    public function getObservableEvents()
    {
        return array_merge(
            [
                'retrieved', 'creating', 'created', 'updating', 'updated',
                'saving', 'saved', 'restoring', 'restored', 'replicating',
                'deleting', 'deleted', 'forceDeleting', 'forceDeleted',
            ],
            $this->observables
        );
    }

So only this observable events will be available to be registered with an observer. No "trashed" event which is available by SoftDeletes Trait when used.

The reason for adding this event is because users might want to track activity log of a model with an observer and log actions like soft deleted, restored, created etc. Both forceDelete() and delete() fire "deleted" event and their respective events of "force deleted" and "trashed" but "trashed can't be listened to in an observer which i think is not intuitive or the desired feature. So it will be common sense for it to be available in an observer too, like someone would draw the conclusion from the docs.

Eloquent models dispatch several events, allowing you to hook into the following moments in a model's lifecycle: retrieved, creating, created, updating, updated, saving, saved, deleting, deleted, trashed, forceDeleting, forceDeleted, restoring, restored, and replicating.

Do i still need to create a test for this?!

@laravel laravel deleted a comment from duemti Mar 12, 2025
duemti added a commit to duemti/framework that referenced this pull request Mar 13, 2025
Fixes laravel#54980

Previous pull laravel#54987 but this one is more clear and inline.
@duemti duemti deleted the patch-1 branch March 13, 2025 08:18
taylorotwell pushed a commit that referenced this pull request Mar 13, 2025
Fixes #54980

Previous pull #54987 but this one is more clear and inline.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants