issue 18 #26
issue 18 #26
Conversation
Resolves #18 — declaring `public string $queue` on a Task causes a PHP fatal error because Laravel's Queueable trait declares $queue without a type hint. The new #[OnQueue('queue-name')] attribute sets the queue via reflection, avoiding the property conflict. When applied to a Process, the attribute sets the process's own queue and propagates it to child tasks that don't define their own #[OnQueue].
📝 WalkthroughWalkthroughThis pull request introduces the OnQueue attribute feature, enabling Tasks and Processes to specify their execution queue via PHP 8 annotation. The attribute is resolved through reflection during instantiation, and Processes propagate queue preferences to chained tasks. Queue metadata is tracked through the Brain map system. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Task as Task/Process
participant Reflect as Reflection System
participant Attr as OnQueue Attribute
participant Queue as Queue System
App->>Task: Instantiate with OnQueue annotation
Task->>Reflect: Read class attributes
Reflect->>Attr: Locate OnQueue attribute
Attr-->>Reflect: Return queue name
Reflect-->>Task: Queue name resolved
Task->>Queue: Route to specified queue
Queue-->>App: Task dispatched to queue
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Process.php`:
- Around line 155-157: The current condition calls resolveQueue() three times
redundantly; change it to call resolveQueue() once (assign to $queue and then
check it) or even simpler use the existing property set by the constructor:
check $this->queue once and if not empty call $chain->onQueue($this->queue);
update the conditional around $chain->onQueue to use the single $queue variable
or $this->queue to avoid repeated reflection/attribute instantiation and
unnecessary reassignments.
- Around line 211-219: Hoist the call to resolveQueue() out of the per-task
loop: compute $processQueue once before the foreach that iterates over tasks
(instead of calling $this->resolveQueue() inside the loop), then inside the loop
use that cached $processQueue when checking $instance->queue and calling
$instance->onQueue($processQueue); alternatively you may use the already-set
$this->queue from the constructor instead of repeatedly calling resolveQueue();
keep the existing logic around ShouldQueue::class, $instance creation, and
dispatch($instance).
🧹 Nitpick comments (3)
src/Task.php (1)
62-67: DuplicateReflectionClassinstantiation per dispatch.
dispatch()(line 110) already creates aReflectionClass(static::class), and here the constructor creates another one. Consider extracting the reflection instance to avoid the double cost — e.g., pass it through or cache it in a static/lazy property.src/Process.php (2)
72-78: Constructor duplicatesresolveQueue()logic.The attribute-resolution logic here is identical to
resolveQueue()(lines 270–279). Reuse the helper to keep a single source of truth, and also reuse theReflectionClassalready created on line 69:♻️ Proposed fix
- Context::add('process', [$this->name, $this->uuid]); - - $onQueue = (new ReflectionClass(static::class)) - ->getAttributes(OnQueue::class); - - if ($onQueue !== []) { - $this->onQueue($onQueue[0]->newInstance()->queue); - } + Context::add('process', [$this->name, $this->uuid]); + + $queue = $this->resolveQueue(); + if ($queue !== null) { + $this->onQueue($queue); + }
267-280: Consider caching the resolved queue or relying on$this->queue.
resolveQueue()creates a newReflectionClasson every call, yet the result is constant for the class. Since the constructor already stores the value via$this->onQueue(...), all downstream call sites (runInChain,run) could simply read$this->queueinstead. This would makeresolveQueue()needed only once — in the constructor — and the helper could be removed entirely.
| if (($queue = $this->resolveQueue()) !== null && ($queue = $this->resolveQueue()) !== '' && ($queue = $this->resolveQueue()) !== '0') { | ||
| $chain->onQueue($queue); | ||
| } |
There was a problem hiding this comment.
resolveQueue() is invoked three times (reflection + attribute instantiation each time).
Each ($queue = $this->resolveQueue()) reassigns $queue, so the first two calls are pure waste — only the last return value survives the && chain. Assign once, then guard:
🐛 Proposed fix
- if (($queue = $this->resolveQueue()) !== null && ($queue = $this->resolveQueue()) !== '' && ($queue = $this->resolveQueue()) !== '0') {
+ $queue = $this->resolveQueue();
+
+ if ($queue !== null && $queue !== '' && $queue !== '0') {
$chain->onQueue($queue);
}Additionally, since the constructor already calls $this->onQueue(...) (setting $this->queue), you could simplify the entire block to just read $this->queue instead of re-resolving via reflection:
if (!empty($this->queue)) {
$chain->onQueue($this->queue);
}🤖 Prompt for AI Agents
In `@src/Process.php` around lines 155 - 157, The current condition calls
resolveQueue() three times redundantly; change it to call resolveQueue() once
(assign to $queue and then check it) or even simpler use the existing property
set by the constructor: check $this->queue once and if not empty call
$chain->onQueue($this->queue); update the conditional around $chain->onQueue to
use the single $queue variable or $this->queue to avoid repeated
reflection/attribute instantiation and unnecessary reassignments.
| if ($reflectionClass->implementsInterface(ShouldQueue::class)) { | ||
| $task::dispatch($payload); | ||
| $processQueue = $this->resolveQueue(); | ||
| $instance = new $task($payload); | ||
|
|
||
| if ($instance->queue === null && $processQueue !== null) { | ||
| $instance->onQueue($processQueue); | ||
| } | ||
|
|
||
| dispatch($instance); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
resolveQueue() called per iteration inside the foreach loop — hoist it above the loop.
resolveQueue() performs reflection on every call, and the result is constant for the lifetime of the process. Move it before the loop, or simply use $this->queue which was already set in the constructor.
♻️ Proposed fix (minimal — hoist before the loop)
Add before line 185 (foreach):
$processQueue = $this->resolveQueue();Then on line 212, remove the per-iteration call:
- $processQueue = $this->resolveQueue();
$instance = new $task($payload);🤖 Prompt for AI Agents
In `@src/Process.php` around lines 211 - 219, Hoist the call to resolveQueue() out
of the per-task loop: compute $processQueue once before the foreach that
iterates over tasks (instead of calling $this->resolveQueue() inside the loop),
then inside the loop use that cached $processQueue when checking
$instance->queue and calling $instance->onQueue($processQueue); alternatively
you may use the already-set $this->queue from the constructor instead of
repeatedly calling resolveQueue(); keep the existing logic around
ShouldQueue::class, $instance creation, and dispatch($instance).
1 participant
What:
Description:
Related:
Summary by CodeRabbit
New Features
Documentation