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

HandleModelNotFound() method uses displayName() method of the job as class name #54502

Open
JochemLettink opened this issue Feb 7, 2025 · 4 comments

Comments

@JochemLettink
Copy link

JochemLettink commented Feb 7, 2025

Laravel Version

11.41.3

PHP Version

8.4.3

Database Driver & Version

Mysql 8.0.40 - Debian 12

Description

When pushing a job to the database queue using the displayName() method to set a custom display name in the queue worker, and with the $deleteWhenMissingModels property set to true, a ModelNotFoundException is still logged and the job is going through the fail() lifecycle if the model is deleted in the meantime.

This happens because the handleModelNotFound() method in CallQueueHandler resolves the job name using $class = $job->resolveName();, which calls JobName::resolve($this->getName(), $this->payload()); This method returns the displayName, which is not a valid class name because the displayName is changed.

As a result, ReflectionClass fails to reflect the class, causing the system to incorrectly determine that shouldDelete is false, leading to running the fail() lifecycle of the job in stead of quietly discard the job.

Steps To Reproduce

  1. Create a database job with the $deleteWhenMissingModels property set to true
  2. Add a model to the constructor of the job
  3. Add an custom displayName() method to the job that returns an non existing class
  4. Create a test that creates the model, dispatches the job and delete the model before running the (DB) queue worker.
  5. Make sure to run the test expecting no errors to be logged.

Image

@JochemLettink JochemLettink changed the title Using the displayName() method returning a custom name in database queue jobs with $deleteWhenMissingModels = true breaks JobName::resolve. HandleModelNotFound() method uses displayName() method of the job as className Feb 7, 2025
@JochemLettink JochemLettink changed the title HandleModelNotFound() method uses displayName() method of the job as className HandleModelNotFound() method uses displayName() method of the job as class name Feb 7, 2025
Copy link

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

@achrafAa
Copy link

@JochemLettink I tried to reproduce the issue but I was unable to, can you please check the code bellow and provide more information

    public function test_job_fails_when_model_is_deleted()
    {
        Queue::fake();

        $post = PostFactory::new()->create([
            'title' => 'Test Post',
            'content' => 'Test Content',
        ]);

        ProcessPost::dispatch($post);

        $post->delete();

        $this->artisan('queue:work', ['--once' => true]);

        Queue::assertPushed(ProcessPost::class, function ($job) use ($post) {
            return $job->post->id === $post->id;
        });

        $this->expectException(ModelNotFoundException::class);
        $this->artisan('queue:work', ['--once' => true]);
    }

class ProcessPost implements ShouldQueue
{

    use Dispatchable, InteractsWithQueue, Queueable, SerializesModels;

    public bool $deleteWhenMissingModels = true;

    public $post;

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

    public function handle()
    {
        Log::info('Processing post: ' . $this->post->title);
    }

    public function displayName(): string
    {
        return 'NonExistentClass';
    }
}

@JochemLettink
Copy link
Author

@achrafAa Thanks for your response! I tested your code and noticed a few differences:

  1. I'm not using queue::fake() because I wanted to test the actual execution of the job.
  2. I'm monitoring the test using Log::shouldReceive('error')->once(); rather than checking whether an exception is thrown. The exception isn’t actually thrown because Laravel catches it internally and executes the handleModelNotFound() method but the job should still be quietly discarded and this is not the case.

The bug is that the exception is still logged because CallQueuedWorker::handleModelNotFound() doesn’t delete the job immediately. Instead, it marks the job as failed, as it cannot reflect the NonExistingClass. (I've attached some screenshots below to illustrate what the function does.).

Ultimately, while the specific ModelNotFoundException isn’t thrown, the job isn’t silently deleted either—it goes through the failure lifecycle. This should not happen because deleteWhenMissingModels should quietly discard the job without raising an exception.

From the logs, it appears that the job has thrown the exception, but this is not the case. I will update the ticket description to match this. Thanks for your input!

<?php

declare(strict_types=1);

namespace App\Jobs;

use App\Models\User;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Queue\Queueable;
use Illuminate\Support\Facades\Log;

class ProcessUser implements ShouldQueue
{

    use Queueable;

    public bool $deleteWhenMissingModels = true;

    public $user;

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

    public function handle()
    {
        Log::info('Processing user: ' . $this->user);
    }

    public function displayName(): string
    {
        return 'NonExistentClass';
    }

    public function fail(?\Exception $e = null) : void
    {
        Log::info($this->user->id);
    }
}

it('skips the job with a custom display name and a removed model', function () : void
{
    // The error is stilled logged...
    Log::shouldReceive('error')->once();

    $user = User::factory()->create();

    ProcessUser::dispatch($user);

    $user->delete();

    // Make sure the models are deleted...
    $this->assertModelMissing($user);

    // Run the whole queue so we can check that no errors are thrown...
    $this->artisan('queue:work', ['--once' => true]);
});

  Tests:    1 passed (2 assertions)

Image

Image

@cosmastech
Copy link
Contributor

This will be resolved in Laravel 12.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants