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

Add support for notification class overrides #1518

Merged

Conversation

lukemorcom
Copy link
Contributor

Back in July I raised issue #1475, because my team wanted to be able to customise the notification that was sent when a long wait occurs. I was told to attempt a PR and this is certainly... an attempt. Please be nice, this is my first ever PR into an open source project.

Usage

// Pass a map of $event => $notification into the function
Horizon::overrideNotifications([
    LongWaitDetected::class => MyCustomNotification::class
]);

Considerations

  • Currently Horizon has only one notification class (LongWaitDetected), but this implementation is designed with future extensibility in mind. Using a map provides flexibility for additional notifications if they are introduced, instead of being limited to a single notification override.
  • Any future Horizon events would also need to include similar logic to LongWaitDetected::toNotification() to determine whether to construct a custom notification. I considered alternative, more abstract approaches, but they would involve more significant changes to the codebase.
  • For compatibility, custom notification classes must extend the original notification associated with the event.

I am quite expecting this PR to be declined, but if that is so, I would greatly appreciate any feedback - as mentioned above this is my first contribution to an open source project so your feedback/advice is genuinely valuable to me. Thanks.

@taylorotwell taylorotwell merged commit 4c30f9c into laravel:5.x Dec 2, 2024
11 checks passed
@taylorotwell
Copy link
Member

@lukemorcom thanks for the PR! I modified this a bit to just use the container and a new interface that you can override with your own binding.

@siarheipashkevich
Copy link

@lukemorcom what about the documentation for this feature?

@jhm-ciberman
Copy link

@siarheipashkevich If you see the PR, Taylor did change it so it uses the container instead of a custom method. So I don't think you need extra documentation besides the existing docs for the IoC container. If someone is looking for something this specific they will see that in the source code Horizon uses the container and they will know that they can rebind the class.

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.

4 participants