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

[9.x] Fix queueable notification's ID overwritten #42581

Merged
merged 1 commit into from
Jun 1, 2022

Conversation

jay-youngn
Copy link
Contributor

@jay-youngn jay-youngn commented May 31, 2022

When the notification ID has been assigned, It should not be overwritten. Even in the queue.

The case for database notification:
I've defined a special unique ID, but it's overwritten here.

If this option is not desirable, the Str::uuid() should also be replace to Str::orderedUuid() for keep order of database index.

When the notification ID has been assigned, It should not be overwritten. Even in the queue.
@jay-youngn jay-youngn changed the title Fix queueable notification's ID overwritten [9.x] Fix queueable notification's ID overwritten May 31, 2022
@taylorotwell
Copy link
Member

How are you defining a special unique ID that is being overwritten?

@jay-youngn
Copy link
Contributor Author

jay-youngn commented May 31, 2022

How are you defining a special unique ID that is being overwritten?

@taylorotwell Thanks for your attention

My cases like this:

Part 1:
I have a function to generate unique IDs in ascending order.

<?php

namespace App\Supports;

use Str;

class UniqueID
{
    /**
     * Generates a string UID.
     */
    public static function generateString(): string
    {
        return str_replace('-', '', (string) Str::orderedUuid());
    }
}

Part 2:
And I assigned notification ID when an instance created.
(Because I want the primary key to be ordered in table notifications)

<?php

namespace App\Notifications;

use App\Enums\EntityAlias;
use App\Enums\InteractType;
use App\Models\User;
use App\Supports\UniqueID;
use Illuminate\Bus\Queueable;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Notifications\Messages\MailMessage;
use Illuminate\Notifications\Notification;

class UserInteractsNotification extends Notification
{
    use Queueable;

    private EntityAlias $topic = EntityAlias::User;

    /**
     * Create a new notification instance.
     *
     * @return void
     */
    public function __construct(
        private InteractType $type,
        private User $fromUser,
    ) {
        $this->id = UniqueID::generateString();
    }

    /**
     * Get the notification's delivery channels.
     *
     * @param  mixed  $notifiable
     * @return array
     */
    public function via($notifiable)
    {
        return ['database'];
    }

    // ...
}

Part 3:
If the UserInteractsNotification implements ShouldQueue, the earlier code in Illuminate\Notification\NotificationSender will always overwrite notification ID.

The important thing is... When queue driver is sync, overwrite will not happen, this is because there is such a piece of code in method NotificationSender::sendToNotifiable():

protected function sendToNotifiable($notifiable, $id, $notification, $channel)
{
if (! $notification->id) {
$notification->id = $id;
}
if (! $this->shouldSendNotification($notifiable, $notification, $channel)) {
return;
}

When notification should queue, the method NotificationSender::queueNotification() will be called, and it does not have a determination of whether the ID exists or not.

So I added same code to line 194

foreach ($notifiables as $notifiable) {
$notificationId = Str::uuid()->toString();
foreach ((array) $original->via($notifiable) as $channel) {
$notification = clone $original;
if (! $notification->id) {
$notification->id = $notificationId;
}

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