laravel / nova-issues

554 stars 34 forks source link

Unable to add batching job within Action's `handle` method #4760

Closed crynobone closed 2 years ago

crynobone commented 2 years ago

I've spent a few hours today, further digging in to this now.

Like mentioned earlier, $batchId is actually never set on the Action. Laravel does set it on the \Laravel\Nova\Actions\CallQueuedAction, as it's the Job that Nova queues up, which is then wrapping the Action itself. So, currently, using the Batchable trait on the Action won't do anything as the $batchId is never set, and therefore none of the methods are currently usable, as they require the $batchId to actually return/handle the Batch it belongs to. A side note on this: if we search through Nova's entire code base, there's no reference to $batchId (or ->batchId, withBatchId, etc) for the Action, which should be needed since \Laravel\Nova\Actions\CallQueuedAction wraps it.

Upon further granular testing, I was able to confirm my theory in an earlier message in this discussion

On a second thought, not 'altogether'. I think the Action would still need to implement ShouldQueue. If it doesn't, Laravel won't queue it, and therefore you might miss the $batchId property. But.. for those Actions that does implement ShouldQueue, it looks like it could work.

This is also perfectly fine as this is how Laravel is designed. Besides - we need to dispatch the Batch in order to get an ID in the first place.

I also tested my code snippet in an earlier message in this discussion, albeit slightly modified.

// `\Laravel\Nova\Actions\CallQueuedAction::handle()` (line 48-56 in Nova 4.13)
$this->callAction(function ($action) {
    if (\in_array(Batchable::class, \class_uses_recursive($action))) {
        $action->withBatchId($this->batchId);
    }

    return $action->withActionBatchId($this->actionBatchId)->{$this->method}($this->fields, $this->models);
});

This makes it work, and I haven't seen any side effects so far. I'm able to simply

// TestQueuedAction

public function handle()
{
    if ($this->batching()) {
        $this->batch()->add(new TestBatchJob);
    }
}

TestQueuedAction::handle() will be run when the queue works on the Job, and the TestBatchJob will be queued up at that moment. Just like Laravel's documentation shows around using Jobs to queue other Jobs as part of a Batch.

I've commited the repo I used to test things. In the repo root there's also a directory called 'issue-details', which should be fairly straight forward with before/after various steps. Each directory contains log output and screenshots from the database I used. To quickly find the actual output location in the code, you can look for TODO: Check output, which is right above each call to the log. Since the 'fix' needs to be applied to Nova itself, you'll need to apply my code snippet at the location mentioned earlier in this message.

I'm not sure if I should keep this message here, start a feature request, or file a bug report. But the documentation does mention

In addition, the action should use the Illuminate\Bus\Batchable trait.

Originally posted by @juse-less in https://github.com/laravel/nova-issues/discussions/4509#discussioncomment-3494569

github-actions[bot] commented 2 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.