Queued notification model serialization problem

[lucadegasperi]

• Laravel Version: 5.3.18
• PHP Version: 7.0.5

  Description:

When a Notification is set to queue (it implements ShouldQueue, Queueable, SerializesModels) the via($notifiable) method doesn't accept a call to a model passed into the notification constructor by returning a Undefined property: Illuminate\Contracts\Database\ModelIdentifier::$property

Steps To Reproduce:

create a notification

class MyNotification extends Notification implements ShouldQueue
{
    use Queueable;
    use SerializesModels;

    public $model;

    public function __construct($model)
    {
        $this->model = $model;
    }

    public function via($notifiable)
    {
        if($this->model->property == 1) {
            return ['mail'];
        } 
        return [];
    }
}

call a notification via

Notification::send($users, new MyNotification($model));

get a Undefined property: Illuminate\Contracts\Database\ModelIdentifier::$property error

[themsaid]

I'm unable to reproduce the issue on a fresh 5.3 installation, model serialization works just fine.

Also you don't need to use SerializesModels.

[lucadegasperi]

@themsaid The problem is weird indeed, because on other similar Notifications I don't seem to encounter it. The best solution I've found so far is to clone the notification object when it's dispatched onto the bus on the channel manager.

from

(new SendQueuedNotifications($this->formatNotifiables($notifiable), $notification, [$channel]))

to

(new SendQueuedNotifications($this->formatNotifiables($notifiable), clone $notification, [$channel]))

I saw the clone method is used on the Notification when it's set to "send now" instead of "queued" shouldn't it be used in both cases anyways?

[lucadegasperi]

When sending the notification to multiple users (and getting the error above) what I saw is that the first time the via() method gets called it's an instance of the model, the second time it's an instance of Illuminate\Contracts\Database\ModelIdentifier this led me to believe there was a problem with the serialization / deserialization that happens when a notification is dispatched onto the bus.

[themsaid]

So the problem occurs only when there are multiple users to send to?

[lucadegasperi]

@themsaid apparently, yes.

[osteel]

Same sort of issue here, happening even when the notification isn't queued. In my case, the problem occurs when there's more than one channel defined for the notification.

E.g. I've got a notification setting an Eloquent model in its constructor, and the notification's via method returns two channels: database and mail. The Eloquent model is properly unserialized in the toDatabase method, but is not in the toMail one. If I remove the database channel, it is properly unserialized in the toMail method.

There must be something going on in the channel manager (sendNow).

[lucadegasperi]

I'm still having this problem...

[themsaid]

@lucadegasperi can you help me replicate? on a fresh installation can you replicate the issue?

[lucadegasperi]

You're right, @themsaid I'll try as soon as I have time to put a demo together 😉

[hasandz]

I have the same issue. This started after I put multiple channels to the via method.

PHP 5.6.28 laravel/framework v5.3.24

[hasandz]

Run phpunit to see error in test case: https://github.com/hasandz/laravel-framework-issue-15866-test-case/commit/fd10c87663b68d54cd124dea244ec9259bfb4311#diff-f8da5a4c1ff306533286796b0e772207

[hasandz]

@lucadegasperi have you figured out what's the problem?

[lucadegasperi]

@hasandz looks like a problem with the serialization / deserialization of the notification object. when cloning the notification before it's queued the problem seems to go away.

[lucadegasperi]

I'm stil facing this issue but can't easily replicate it on a fresh installation. What @hasandz is what I'm getting as well. Simply adding that clone method I reported above seems to fix the issue. It seems like a no-brainer to add it.

[hasandz]

I found the problem, you need to define it as protected in the notification class, like this:

class PaymentCreated extends Notification implements ShouldQueue
{
    use Queueable;

    protected $payment;

    public function __construct(Payment $payment)
    {
        $this->payment = $payment;
    }

I hope this helps.

[lucadegasperi]

@hasandz that doesn't work for me. setting the properties as private yields the same problem.

[lucadegasperi]

This is the stack trace from the failed job

Illuminate\Database\Eloquent\ModelNotFoundException: No query results for model [App\TimeManagement\Availabilities\Models\Availability] 25053 in /vendor/laravel/framework/src/Illuminate/Database/Eloquent/Builder.php:324
Stack trace:
#0 /vendor/laravel/framework/src/Illuminate/Queue/SerializesAndRestoresModelIdentifiers.php(46): Illuminate\Database\Eloquent\Builder->findOrFail(25053)
#1 /vendor/laravel/framework/src/Illuminate/Queue/SerializesModels.php(41): Illuminate\Notifications\Notification->getRestoredPropertyValue(Object(Illuminate\Contracts\Database\ModelIdentifier))
#2 [internal function]: Illuminate\Notifications\Notification->__wakeup()
#3 /vendor/laravel/framework/src/Illuminate/Queue/CallQueuedHandler.php(38): unserialize('O:48:"Illuminat...')
#4 /vendor/laravel/framework/src/Illuminate/Queue/Jobs/Job.php(69): Illuminate\Queue\CallQueuedHandler->call(Object(Illuminate\Queue\Jobs\BeanstalkdJob), Array)
#5 /vendor/laravel/framework/src/Illuminate/Queue/Worker.php(291): Illuminate\Queue\Jobs\Job->fire()
#6 /vendor/laravel/framework/src/Illuminate/Queue/Worker.php(258): Illuminate\Queue\Worker->process('beanstalkd', Object(Illuminate\Queue\Jobs\BeanstalkdJob), Object(Illuminate\Queue\WorkerOptions))
#7 /vendor/laravel/framework/src/Illuminate/Queue/Worker.php(110): Illuminate\Queue\Worker->runJob(Object(Illuminate\Queue\Jobs\BeanstalkdJob), 'beanstalkd', Object(Illuminate\Queue\WorkerOptions))
#8 /vendor/laravel/framework/src/Illuminate/Queue/Console/WorkCommand.php(100): Illuminate\Queue\Worker->daemon('beanstalkd', 'production', Object(Illuminate\Queue\WorkerOptions))
#9 /vendor/laravel/framework/src/Illuminate/Queue/Console/WorkCommand.php(83): Illuminate\Queue\Console\WorkCommand->runWorker('beanstalkd', 'production')
#10 [internal function]: Illuminate\Queue\Console\WorkCommand->fire()
#11 /vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php(28): call_user_func_array(Array, Array)
#12 /vendor/laravel/framework/src/Illuminate/Support/helpers.php(912): Illuminate\Container\BoundMethod::Illuminate\Container\{closure}()
#13 /vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php(86): value(Object(Closure))
#14 /vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php(30): Illuminate\Container\BoundMethod::callBoundMethod(Object(Illuminate\Foundation\Application), Array, Object(Closure))
#15 /vendor/laravel/framework/src/Illuminate/Container/Container.php(524): Illuminate\Container\BoundMethod::call(Object(Illuminate\Foundation\Application), Array, Array, NULL)
#16 /vendor/laravel/framework/src/Illuminate/Console/Command.php(182): Illuminate\Container\Container->call(Array)
#17 /vendor/symfony/console/Command/Command.php(262): Illuminate\Console\Command->execute(Object(Symfony\Component\Console\Input\ArgvInput), Object(Illuminate\Console\OutputStyle))
#18 /vendor/laravel/framework/src/Illuminate/Console/Command.php(167): Symfony\Component\Console\Command\Command->run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Illuminate\Console\OutputStyle))
#19 /vendor/symfony/console/Application.php(826): Illuminate\Console\Command->run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#20 /vendor/symfony/console/Application.php(189): Symfony\Component\Console\Application->doRunCommand(Object(Illuminate\Queue\Console\WorkCommand), Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#21 /vendor/symfony/console/Application.php(120): Symfony\Component\Console\Application->doRun(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#22 /vendor/laravel/framework/src/Illuminate/Foundation/Console/Kernel.php(123): Symfony\Component\Console\Application->run(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#23 /artisan(35): Illuminate\Foundation\Console\Kernel->handle(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#24 {main}

[lucadegasperi]

I'm not sure where that 25053 comes from in the findOrFail call since no id like that can be found in my DB.

[desh901]

This issue was ever solved?

[desh901]

I was able to detect where the issue: the shouldSendNotification in Illuminate\Notifications\ChannelManager method that actually dispatches the NotificationSending event. The event dispatcher Illuminate\Events\Dispatcher will call serialize on line 417 over the just cloned notification and unserialize is never called back to restored serialized models.

[desh901]

I was able to solve definitively by modifying the shouldSendNotification method in Illuminate\Notifications\ChannelManager to

/**
     * Determines if the notification can be sent.
     *
     * @param  mixed  $notifiable
     * @param  mixed  $notification
     * @param  string  $channel
     * @return bool
     */
    protected function shouldSendNotification($notifiable, $notification, $channel)
    {
        $dispatchable = clone $notification;
        return $this->app->make('events')->until(
            new Events\NotificationSending($notifiable, $dispatchable, $channel)
        ) !== false;
    }

[akalongman]

Is this fixed?

[mehdirochdi]

i have same issue, and i resolved, i juste remove SerializesModels in my classJob and the queue work fine

[jwhulette]

I had a similar issue. When queuing users to notify, they notifications were being sent but to the same wrong user. My user collection was the result of a join and in the resulting user object had a role_id, which was used as the user id when serializing the data. My workaround was to just return the user id from the query.

[Kreshnik]

I am still encountering this issue when sending multiple emails to different users.

[MovingGifts]

I had the same issue when I had an event that fired 2 listeners. Each listener did not have any queue, instead, each listener fired a notification class, and each of the 2 notifications had a queue.

Once the first notification had implements ShouldQueue, the collection (with a join) I had passed to the listeners from the event gave the Undefined property: Illuminate\Contracts\Database\ModelIdentifier::$property and so the second queue would never run. Removing the implements ShouldQueue makes it work.

But since we need both notifications on the queue, what fixed it was removing SerializesModels from the event file. Now everything works with the queue as expected. Really not sure if this is a bug or what, since it works without the shouldQueue and only breaks with it!

[driesvints]

I just experienced this problem myself. The problem was because my queue processor was still running the old code after deploying. You specifically need to terminate and restart your queue after your new code is released. If you're using horizon f.e. you need to run horizon:purge & horizon:terminate right after activating a new release. That should be enough for your jobs to work properly with the SerializesModels trait.

[akalongman]

Why run horizon:purge?

[driesvints]

@akalongman it prunes orphaned Horizon processes which aren't managed anymore.

[matbeard]

Not sure why this was closed, as it's not fixed.

I've just spent hours tracking this issue down. After adding implements ShouldQueue to my notification, the model attributes are lost in the toMail and toArray methods:

I'm passing a model to the constructor and setting a public (or private/protected... makes no differernce) property. If I dump the model from the constructor, all attributes are there as expected, but if I dump the model from the toMail method, attributes are missing.

Obviously removing implements ShouldQueue fixes the problem, but slows the system. Is there a fix that doesn't involve changes to Laravel source?

[MovingGifts]

@matbeard I believe this is by design. This question/answer helped me a lot understand and fix the issue. Hopefully it can help you and others as well!

[silverdonkey]

Hi,

have you checked the mass assignable attributes on the model, which is getting passed to the Notification?

I had the same issue and fixed it by adding the 'id' into the list of the attributes that are mass assignable:

protected $fillable = [
    'id',
    'another_attribute'
];

Cheers!

[rohan-deshpande]

Having this same issue on 5.4.* with php71 this issue should not be closed especially without any changes to the docs. I don't think this has anything to do with sending to multiple users.

I fixed it by adding protected properties to the notification class as @hasandz stated.

[acacha]

I'm not sure it's 110 related but in my case I avoid ModelIdentifier problems being careful of order of Listeners executed related to same event in EventServiceProvider:

For example:

This thown an error:

 // Incidents
        IncidentStored::class => [
            LogIncidentStored::class,
            SendIncidentCreatedEmail::class,
        ],

But not this:

 // Incidents
        IncidentStored::class => [
            SendIncidentCreatedEmail::class,
            LogIncidentStored::class,
        ],

But i'm not sure why. Also I confirmed that using only one Listener (SendIncidentCreatedEmail) it works.

[acacha]

See also https://github.com/laravel/framework/issues/26791 for more info about my issue how to reproduce and how to solve it.