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

[11.x] Notification middleware #51174

Closed
wants to merge 1 commit into from
Closed

Conversation

arondeparon
Copy link
Contributor

@arondeparon arondeparon commented Apr 22, 2024

This PR implements middleware support on queued notifications.

Why?

Very commonly, I run into rate limit issues with notifications, and ideally, I'd have the rate limiting logic bound with my notification. Notifications already support queued delivery through the use of the ShouldQueue interface and the Queable trait, so we are already leaning into jobs.

Use case: rate limiting

Rate limiting a notification requires the InteractsWithQueue trait to be imported, which allows the job to release itself onto the queue. In addition to that, a middleware() method should be added to the notification.

class MyNotification extends Notification implements ShouldQueue
{
    use InteractsWithQueue;
    use Queuable;

    public function middleware(): array
    {
        return [
            new Ratelimited('my-ratelimiter'),
        ];
    }
}

Other use cases

Personally, I have not had other use cases for notification middleware other than rate limiting, which is why I did consider building a notification-specific implementation of rate limiting. However, I feel like sticking to the existing syntax is better, especially considering the fact that queued notifications are already leaning on jobs.

Even so, middleware might unlock some new additional capabilities, such as content manipulation or conditional sending.

@taylorotwell
Copy link
Member

This seems to be passing the notification instance through the middleware... wouldn't job middleware expect a job instance?

@taylorotwell taylorotwell marked this pull request as draft April 22, 2024 17:24
@arondeparon
Copy link
Contributor Author

Wow, while digging deeper it actually seems like middleware functionality already exists; it's just not documented yet: #44767

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