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

[8.x] Allow custom names for queued notifications #34620

Merged
merged 1 commit into from
Oct 2, 2020
Merged

[8.x] Allow custom names for queued notifications #34620

merged 1 commit into from
Oct 2, 2020

Conversation

miken32
Copy link
Contributor

@miken32 miken32 commented Oct 1, 2020

Since this value shows up in the output for the queue worker, it would be nice to be able to customise it. Previous behaviour remains intact if no displayName() method is set on the Notification instance.

I wasn't able to find any tests for the existing function. Please point me in the right direction if I've missed something, but this seems like very low-level stuff not really needing a test.

Since this value shows up in the output for the queue worker, it would be nice to be able to customise it.
@GrahamCampbell GrahamCampbell changed the title Allow custom names for queued notifications [8.x] Allow custom names for queued notifications Oct 1, 2020
@taylorotwell
Copy link
Member

Why isn't the class name enough?

@miken32
Copy link
Contributor Author

miken32 commented Oct 1, 2020

The class name is enough for me, as the application programmer, but for people who don't live in the code it's nice to have the option to put something more human readable in the logs. This is already possible with queued jobs, so I expected to be able to do the same with queued notifications.

@taylorotwell taylorotwell merged commit 31075b7 into laravel:8.x Oct 2, 2020
taylorotwell added a commit that referenced this pull request Oct 2, 2020
taylorotwell added a commit that referenced this pull request Oct 2, 2020
@driesvints
Copy link
Member

We're not adding labels for hacktoberfest after the influx of spam recently.

@miken32
Copy link
Contributor Author

miken32 commented Jan 20, 2022

@taylorotwell just went through figuring this out again and then remembered that I'd done a PR for it already. Is there a reason it was merged and then immediately reverted?

To expand on my previous comment, it's also helpful when finding a problem with hundreds of notifications per minute to be able to add some kind of differentiation to show up in the logs, such as the recipient address for example.

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