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

[12.x] Enable listening to multiple Eloquent events #53562

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

istiak-tridip
Copy link
Contributor

@istiak-tridip istiak-tridip commented Nov 18, 2024

Description

This PR introduces a new Model::listen method, allowing developers to listen to multiple Eloquent events at once.

This enhancement improves the developer experience and aligns with the existing Event::listen behavior. Additionally, it introduces support for wildcard event listening for Eloquent models.

Currently, performing the same action for multiple events looks like this:

Categories::created(function () {
    Cache::forget('categories');
});

Categories::deleted(function () {
    Cache::forget('categories');
});

With This PR:

Categories::listen(['created', 'deleted'], function () {
    Cache::forget('categories');
});

// Listen to all Eloquent events for the model
Categories::listen(function () {
    // Do something
});

Targeting the master branch based on @taylorotwell's suggestion.

@Diddyy
Copy link

Diddyy commented Nov 18, 2024

This looks like it'll clean up alot of my code base!

@chr15k
Copy link

chr15k commented Nov 19, 2024

hey @istiak-tridip is there a use case for listening on all events?

@istiak-tridip
Copy link
Contributor Author

@chr15k One of the goals of this PR was to align with Event::listen, which supports wildcard event listening, so I included it here as well. While I personally don’t have a specific use case for wildcard listeners, here are a few possible scenarios:

  • To listen to all events except specific ones (e.g., exclude certain events).
  • Perform actions when a model is booting or booted.
  • Track model activity or usage through comprehensive event logging.

Happy to hear your thoughts!

@chr15k
Copy link

chr15k commented Nov 19, 2024

@chr15k One of the goals of this PR was to align with Event::listen, which supports wildcard event listening, so I included it here as well. While I personally don’t have a specific use case for wildcard listeners, here are a few possible scenarios:

  • To listen to all events except specific ones (e.g., exclude certain events).
  • Perform actions when a model is booting or booted.
  • Track model activity or usage through comprehensive event logging.

Happy to hear your thoughts!

@istiak-tridip thanks, just to be more specific; my understanding is in order to achieve this with Event::listen you'd need to explicitly pass the wildcard, which shows intent:

  Event::listen('*', function () {
      Cache::forget('categories');
  });

I don't believe that you can pass a callback as first param with Event::listen without declaring the specific Event class as the closure param:

for example, this will not work:

Event::listen(function () {
    Cache::forget('categories');
});

But this will:

Event::listen(function (CategoryDeletedEvent $event) {
    Cache::forget('categories');
});

However, the following does work, which IMO is unexpected if going by the above behaviour, and the underlying '*' wildcard that's in effect, is hidden from the dev:

Categories::listen(function () {
   Cache::forget('categories');
});

Let me know your thoughts.

Cheers!

@istiak-tridip
Copy link
Contributor Author

@chr15k Huh, I can't believe I missed that. Thanks for pointing it out! 🙌 What syntax would you suggest?

Based on my current implementation, developers can still use * to listen to wildcard events. Since model events are string and not class, I don't think we can replicate the Event::listen closure behavior (maybe I'm wrong 🤔?).

With that in mind, I propose keeping Model::listen(fn () => dump('...')) as syntax sugar for the wildcard listener to enhance the developer experience. However, I'm open to changes and would love to hear feedback from @taylorotwell or others.

@chr15k
Copy link

chr15k commented Nov 20, 2024

@istiak-tridip yeh I'd like to hear others' opinions on this for sure, as I can see this as something I'd use in my projects and agree it would be quite useful to add a catch-all for events on a model. I suppose all I'm wondering is if the wildcard should be set explicitly or not, for the purpose of clearer intent. If so, a straightforward approach could be to disallow the first parameter from accepting a closure.

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.

3 participants