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

[5.6] Fix an issue where delayed jobs in L5.5 fail to run in L5.6 #23287

Merged
merged 1 commit into from
Feb 25, 2018
Merged

[5.6] Fix an issue where delayed jobs in L5.5 fail to run in L5.6 #23287

merged 1 commit into from
Feb 25, 2018

Conversation

vicgonvt
Copy link
Contributor

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 of parseWithRelations() 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.

@vicgonvt
Copy link
Contributor Author

This is related to issue #23168

@tillkruss tillkruss changed the title fixes an issue where delayed jobs in L5.5 fail to run in L5.6 [5.6] Fix an issue where delayed jobs in L5.5 fail to run in L5.6 Feb 25, 2018
@laurencei
Copy link
Contributor

laurencei commented Feb 25, 2018

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

@vicgonvt
Copy link
Contributor Author

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 Form

Yes, it works and the relations are still there in L5.6.

The Long Form

  • Create new L5.5 Project
  • Change Environment QUEUE_DRIVER=database
  • Change Environment Database credentials to properly work in your test environment
  • Create new Posts table with a user_id field and string field for message
$table->unsignedInteger('user_id');
$table->string('message');
  • Add relations users table
    public function posts()
    {
        return $this->hasMany(Post::class);
    }
  • Create new job
<?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.

  • Run php artisan migrate

  • Manually add a dummy user and create a couple of dummy posts with the user's id in the user_id column

  • I chose to fire off the job in my routes file but of course anywhere would work the same

Route::get('/', function () {
	
	dispatch(new \App\Jobs\MyCoolJob(\App\User::first()->with('posts')->first()));

});
  • Now let's inspect the job in the database
{"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:{}}"}}
  • As noted before notice the lack of that serialized relations entry.

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 9:\"relations\";a:1:{i:0;s:5:\"posts\";} Posts relationship in the serialized payload.

Let's continue...

  • Upgrade project and dependencies to L5.6.
  • We should still have that job in the database as we haven't attempted to run it just yet,
  • In the command line, run php artisan queue:work
  • Job fails... a quick inspection of the log file reveals the following message
[2018-02-25 14:49:50] local.ERROR: Type error: Argument 1 passed to Illuminate\Database\Eloquent\Builder::parseWithRelations() must be of the type array, null given, called in /Users/vicgonvt/Documents/code/pr-testing-56/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Builder.php on line 1038 {"exception":"[object] (Symfony\\Component\\Debug\\Exception\\FatalThrowableError(code: 0): Type error: Argument 1 passed to Illuminate\\Database\\Eloquent\\Builder::parseWithRelations() must be of the type array, null given, called in /Users/vicgonvt/Documents/code/pr-testing-56/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Builder.php on line 1038 at /Users/vicgonvt/Documents/code/pr-testing-56/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Builder.php:1079)

Let's apply the PR

Apply 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 ?? []);
    }
  • Re-queue job php artisan queue:retry all
  • Start up the php artisan queue:work again.
  • Job runs
  • Laravel log files results
[2018-02-25 14:52:25] local.INFO: ["My Message Here 1","My message 2 HERE"]  
  • Thus, proving that we are able to get that posts relationship from any L5.5 jobs without problems.

@taylorotwell taylorotwell merged commit 2d1364d into laravel:5.6 Feb 25, 2018
@aaronhuisinga
Copy link

Thanks for the PR, @vicgonvt! It will be nice to have my temporary fix removed from my 5.6 installations!

@vicgonvt
Copy link
Contributor Author

@aaronhuisinga me too!

@laurencei
Copy link
Contributor

@vicgonvt - great work. awesome first PR to Laravel :)

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.

None yet

4 participants