-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
[5.6] Fix an issue where delayed jobs in L5.5 fail to run in L5.6 #23287
Conversation
…the new relations traits introduced in 230eeef
This is related to issue #23168 |
Hi @vicgonvt, Whilst this fixes the initial error - what occurs further down the path on the logical side? If the model actually had a relationship, we are now passing in an empty array, is there the potential for a logical error by the queue for jobs queued in 5.5 but run in 5.6? For example - if my 5.5 job model had a User->Post relationship. In 5.5 the system would check for the Post after it was loaded. In 5.6 you've given an empty array - will 5.6 know to go and check for that Post relationship (from a 5.5 job), or will it wrongly assume it does not exist? In which case, the queued 5.5 job will run - but the data is incorrect and it wont be obvious. I think this should be confirmed before merging the PR, because otherwise we are just hiding the issue and it wont be obvious. p.s. I can see this is your first PR - thanks and welcome to the community 👍 ping @browner12 |
hello @laurencei, This is a very valid point that I hadn't even thought of so after A LOT of setup of dummy projects in 5.5 and 5.6, I was able to test it and see for myself if in fact we would run into relationship problems further down the code. The Short FormYes, it works and the relations are still there in L5.6. The Long Form
$table->unsignedInteger('user_id');
$table->string('message');
public function posts()
{
return $this->hasMany(Post::class);
}
<?php
namespace App\Jobs;
use Illuminate\Bus\Queueable;
use Illuminate\Queue\SerializesModels;
use Illuminate\Queue\InteractsWithQueue;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Bus\Dispatchable;
class MyCoolJob implements ShouldQueue
{
use Dispatchable, InteractsWithQueue, Queueable, SerializesModels;
public $user;
/**
* Create a new job instance.
*
* @return void
*/
public function __construct($user)
{
$this->user = $user;
}
/**
* Execute the job.
*
* @return void
*/
public function handle()
{
\Log::info($this->user->posts->pluck('message'));
}
} This job will just accept a user and write the relationship's message to the log file. Simple but proves that the relations are still intact.
Route::get('/', function () {
dispatch(new \App\Jobs\MyCoolJob(\App\User::first()->with('posts')->first()));
});
{"displayName":"App\\Jobs\\MyCoolJob","job":"Illuminate\\Queue\\CallQueuedHandler@call","maxTries":1,"timeout":null,"timeoutAt":null,"data":{"commandName":"App\\Jobs\\MyCoolJob","command":"O:18:\"App\\Jobs\\MyCoolJob\":8:{s:4:\"user\";O:45:\"Illuminate\\Contracts\\Database\\ModelIdentifier\":3:{s:5:\"class\";s:8:\"App\\User\";s:2:\"id\";i:1;s:10:\"connection\";s:5:\"mysql\";}s:6:\"\u0000*\u0000job\";N;s:10:\"connection\";N;s:5:\"queue\";N;s:15:\"chainConnection\";N;s:10:\"chainQueue\";N;s:5:\"delay\";N;s:7:\"chained\";a:0:{}}"}}
Side note: Same job created in L5.6 looks like this...{"displayName":"App\\Jobs\\MyCoolJob","job":"Illuminate\\Queue\\CallQueuedHandler@call","maxTries":null,"timeout":null,"timeoutAt":null,"data":{"commandName":"App\\Jobs\\MyCoolJob","command":"O:18:\"App\\Jobs\\MyCoolJob\":8:{s:4:\"user\";O:45:\"Illuminate\\Contracts\\Database\\ModelIdentifier\":4:{s:5:\"class\";s:8:\"App\\User\";s:2:\"id\";a:1:{i:0;i:1;}s:9:\"relations\";a:1:{i:0;s:5:\"posts\";}s:10:\"connection\";s:5:\"mysql\";}s:6:\"\u0000*\u0000job\";N;s:10:\"connection\";N;s:5:\"queue\";N;s:15:\"chainConnection\";N;s:10:\"chainQueue\";N;s:5:\"delay\";N;s:7:\"chained\";a:0:{}}"}} We see that Let's continue...
Let's apply the PRApply the fix to the SerializesAndRestoresModelIdentifiers trait. public function restoreModel($value)
{
return $this->getQueryForModelRestoration(
(new $value->class)->setConnection($value->connection), $value->id
)->useWritePdo()->firstOrFail()->load($value->relations ?? []);
}
|
Thanks for the PR, @vicgonvt! It will be nice to have my temporary fix removed from my 5.6 installations! |
@aaronhuisinga me too! |
@vicgonvt - great work. awesome first PR to Laravel :) |
Due to the new relations method introduced in commit 230eeef when jobs got queued in L5.5, the
relations
didn't exist in the serialized payload of the queued job so when the queue tries to run those old L5.5 queued jobs in L5.6, it throws an exception ofparseWithRelations() must be of type array, boolean given
. This problem only affects jobs that were queued in L5.5 for a later date and are now trying to be dispatched in L5.6.In a production environment where jobs have been sitting in the queue waiting to be dispatched by the queue worker, it fails to run the jobs.
With this commit those jobs that have a false for the
relations
property, it will default to an empty array.