Route queued jobs via a class-keyed registry instead of a shared static

imorland · imorland · commit 0dae3e404f0b · 2026-07-29T11:00:33.000+01:00
AbstractJob::$onQueue was a static property on the base class. A subclass
that does not redeclare a static shares its parent's storage, so every job
extending AbstractJob without its own $onQueue shared a single slot — routing
two job classes made the last assignment win for both, and the same class
could resolve to different queues in different processes. This silently
mis-routed jobs onto whichever queue was assigned last.

Route jobs by a container-side map keyed by class name instead, setting the
queue on the instance (Laravel's $queue via onQueue()) rather than a shared
static:

- Remove AbstractJob::$onQueue; the constructor resolves the job's queue from
  the new flarum.queue.routes registry.
- Bind flarum.queue.routes (job class =&gt; queue) in QueueServiceProvider.
- Add Extend\Queue-&gt;route(JobClass::class, 'queue') to populate it.

Nothing is stored on the job class, so routing is collision-proof and
deterministic across processes. Breaking: code setting $onQueue must move to
Extend\Queue::route().
diff --git a/framework/core/src/Extend/Queue.php b/framework/core/src/Extend/Queue.php
@@ -0,0 +1,56 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace Flarum\Extend;
+
+use Flarum\Extension\Extension;
+use Illuminate\Contracts\Container\Container;
+
+class Queue implements ExtenderInterface
+{
+    /**
+     * @var array<class-string, string>
+     */
+    private array $routes = [];
+
+    /**
+     * Route all instances of a job class onto a named queue.
+     *
+     * The job's queue is set on the instance (Laravel's `$queue`, via
+     * `onQueue()`) when it is constructed, so dispatch sites do not need to
+     * change. Routing is stored in a `[job class => queue]` map rather than on
+     * the job class itself — a per-class static would be shared by any subclass
+     * that does not redeclare it, so routing two sibling jobs would silently
+     * collide.
+     *
+     * Routing a base/abstract job class does not automatically cover its
+     * subclasses (resolution is by exact class name); route each concrete class
+     * you dispatch, or the base if you dispatch it directly.
+     *
+     * @param class-string $jobClass  A job extending Flarum\Queue\AbstractJob.
+     * @param string       $queue     The queue name to dispatch its instances on.
+     */
+    public function route(string $jobClass, string $queue): self
+    {
+        $this->routes[$jobClass] = $queue;
+
+        return $this;
+    }
+
+    public function extend(Container $container, ?Extension $extension = null): void
+    {
+        if (empty($this->routes)) {
+            return;
+        }
+
+        $container->extend('flarum.queue.routes', function (array $routes) {
+            return array_merge($routes, $this->routes);
+        });
+    }
+}
diff --git a/framework/core/src/Queue/AbstractJob.php b/framework/core/src/Queue/AbstractJob.php
@@ -10,6 +10,7 @@
 namespace Flarum\Queue;
 
 use Illuminate\Bus\Queueable;
+use Illuminate\Container\Container;
 use Illuminate\Contracts\Queue\ShouldQueue;
 use Illuminate\Queue\InteractsWithQueue;
 use Illuminate\Queue\SerializesModels;
@@ -32,20 +33,41 @@ class AbstractJob implements ShouldQueue
      */
     public bool $deleteWhenMissingModels = true;
 
+    public function __construct()
+    {
+        $this->applyQueueRoute();
+    }
+
     /**
-     * Optional queue name onto which jobs of this class should be routed.
+     * Route this job onto a dedicated queue if one has been registered for its
+     * class, by setting the instance's queue (Laravel's normal `$queue`
+     * property, via `onQueue()`).
+     *
+     * Routing is looked up in the `flarum.queue.routes` container binding — a
+     * `[job class => queue name]` map that operators and extensions populate
+     * (see `Flarum\Extend\Queue::route()`). This keeps the routing decision off
+     * the job class itself: a per-class static would be shared by every
+     * subclass that does not redeclare it, so routing two sibling jobs would
+     * silently collide. A container-side map keyed by class name cannot.
      *
-     * Operators (or extensions) may set this on a subclass to dispatch all of
-     * its instances onto a dedicated queue without modifying the dispatch
-     * sites. Subclasses overriding `__construct` must call `parent::__construct()`
-     * to opt in.
+     * Subclasses overriding `__construct` must call `parent::__construct()` to
+     * keep their routing.
      */
-    public static ?string $onQueue = null;
-
-    public function __construct()
+    protected function applyQueueRoute(): void
     {
-        if (static::$onQueue !== null) {
-            $this->onQueue(static::$onQueue);
+        // Jobs are instantiated directly (`new SomeJob(...)`) at dispatch time,
+        // so pull the registry from the current container rather than requiring
+        // it to be injected.
+        $container = Container::getInstance();
+
+        if ($container === null || ! $container->bound('flarum.queue.routes')) {
+            return;
+        }
+
+        $queue = $container->make('flarum.queue.routes')[static::class] ?? null;
+
+        if ($queue !== null) {
+            $this->onQueue($queue);
         }
     }
 }
diff --git a/framework/core/src/Queue/QueueServiceProvider.php b/framework/core/src/Queue/QueueServiceProvider.php
@@ -65,12 +65,21 @@ public function register(): void
         });
 
         // The queue names known to this installation. Extensions that route
-        // jobs onto named queues (e.g. via AbstractJob::$onQueue) should
-        // append theirs so admin tooling can offer per-queue controls.
+        // jobs onto named queues should append theirs so admin tooling can
+        // offer per-queue controls.
         $this->container->singleton('flarum.queue.queues', function () {
             return ['default'];
         });
 
+        // Job-class => queue-name routing. AbstractJob consults this map in its
+        // constructor to place instances onto a dedicated queue without the
+        // dispatch site (or the job class) having to know. Populated via
+        // Flarum\Extend\Queue::route(); extensions (and FoF Horizon's built-in
+        // routing) add entries here.
+        $this->container->singleton('flarum.queue.routes', function () {
+            return [];
+        });
+
         $this->container->singleton(Factory::class, function (Container $container) {
             return new QueueFactory(
                 function () use ($container) {
diff --git a/framework/core/tests/integration/extenders/QueueTest.php b/framework/core/tests/integration/extenders/QueueTest.php
@@ -0,0 +1,87 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace Flarum\Tests\integration\extenders;
+
+use Flarum\Extend;
+use Flarum\Queue\AbstractJob;
+use Flarum\Testing\integration\TestCase;
+use PHPUnit\Framework\Attributes\Test;
+
+class QueueTest extends TestCase
+{
+    #[Test]
+    public function jobs_are_not_routed_by_default(): void
+    {
+        $this->app();
+
+        $this->assertNull((new QueueTestJobA())->queue);
+    }
+
+    #[Test]
+    public function route_sends_a_job_class_to_its_queue(): void
+    {
+        $this->extend(
+            (new Extend\Queue())->route(QueueTestJobA::class, 'priority')
+        );
+
+        $this->app();
+
+        $this->assertSame('priority', (new QueueTestJobA())->queue);
+    }
+
+    /**
+     * Regression: routing several job classes must not collide. Each class
+     * keeps its own queue when they are all dispatched in the same process.
+     */
+    #[Test]
+    public function sibling_job_classes_route_independently(): void
+    {
+        $this->extend(
+            (new Extend\Queue())
+                ->route(QueueTestJobA::class, 'realtime')
+                ->route(QueueTestJobB::class, 'gdpr')
+        );
+
+        $this->app();
+
+        $this->assertSame('realtime', (new QueueTestJobA())->queue);
+        $this->assertSame('gdpr', (new QueueTestJobB())->queue);
+        // An unrouted sibling is unaffected.
+        $this->assertNull((new QueueTestJobC())->queue);
+    }
+
+    #[Test]
+    public function multiple_extenders_compose(): void
+    {
+        $this->extend(
+            (new Extend\Queue())->route(QueueTestJobA::class, 'a')
+        );
+        $this->extend(
+            (new Extend\Queue())->route(QueueTestJobB::class, 'b')
+        );
+
+        $this->app();
+
+        $this->assertSame('a', (new QueueTestJobA())->queue);
+        $this->assertSame('b', (new QueueTestJobB())->queue);
+    }
+}
+
+class QueueTestJobA extends AbstractJob
+{
+}
+
+class QueueTestJobB extends AbstractJob
+{
+}
+
+class QueueTestJobC extends AbstractJob
+{
+}
diff --git a/framework/core/tests/unit/Queue/AbstractJobTest.php b/framework/core/tests/unit/Queue/AbstractJobTest.php
@@ -11,53 +11,101 @@
 
 use Flarum\Queue\AbstractJob;
 use Flarum\Testing\unit\TestCase;
+use Illuminate\Container\Container;
 use PHPUnit\Framework\Attributes\Test;
 
 class AbstractJobTest extends TestCase
 {
     protected function tearDown(): void
     {
-        AbstractJobTestStub::$onQueue = null;
-        AbstractJobTestSubclassStub::$onQueue = null;
+        Container::setInstance(null);
 
         parent::tearDown();
     }
 
+    /**
+     * @param array<class-string, string> $routes
+     */
+    private function containerWithRoutes(array $routes = []): void
+    {
+        $container = new Container();
+        $container->instance('flarum.queue.routes', $routes);
+        Container::setInstance($container);
+    }
+
     #[Test]
     public function defaults_to_no_queue_routing(): void
     {
-        $job = new AbstractJobTestStub();
+        $this->containerWithRoutes();
+
+        $this->assertNull((new AbstractJobTestStub())->queue);
+    }
+
+    #[Test]
+    public function no_container_is_harmless(): void
+    {
+        Container::setInstance(null);
+
+        $this->assertNull((new AbstractJobTestStub())->queue);
+    }
+
+    #[Test]
+    public function routes_onto_the_queue_registered_for_its_class(): void
+    {
+        $this->containerWithRoutes([AbstractJobTestStub::class => 'priority']);
 
-        $this->assertNull($job->queue);
+        $this->assertSame('priority', (new AbstractJobTestStub())->queue);
     }
 
     #[Test]
-    public function routes_onto_queue_named_by_static_property(): void
+    public function an_unrouted_class_is_unaffected_by_other_routes(): void
     {
-        AbstractJobTestStub::$onQueue = 'priority';
+        $this->containerWithRoutes([AbstractJobTestOtherStub::class => 'other']);
+
+        $this->assertNull((new AbstractJobTestStub())->queue);
+    }
 
-        $job = new AbstractJobTestStub();
+    /**
+     * The regression test for the shared-static bug: routing several sibling
+     * job classes must not collide. A previous implementation stored the queue
+     * in a static on AbstractJob, which siblings that did not redeclare it
+     * shared — so routing one overwrote the others and, across processes, the
+     * same class could even land on different queues. A class-keyed registry
+     * gives each class independent routing.
+     */
+    #[Test]
+    public function sibling_classes_route_independently_in_the_same_process(): void
+    {
+        $this->containerWithRoutes([
+            AbstractJobTestStub::class => 'realtime',
+            AbstractJobTestOtherStub::class => 'gdpr',
+            AbstractJobTestSubclassStub::class => 'exports',
+        ]);
 
-        $this->assertSame('priority', $job->queue);
+        $this->assertSame('realtime', (new AbstractJobTestStub())->queue);
+        $this->assertSame('gdpr', (new AbstractJobTestOtherStub())->queue);
+        $this->assertSame('exports', (new AbstractJobTestSubclassStub())->queue);
     }
 
     #[Test]
-    public function static_property_is_resolved_via_late_static_binding(): void
+    public function resolution_is_by_exact_class_not_inherited_from_parent(): void
     {
-        AbstractJobTestStub::$onQueue = 'parent-queue';
-        AbstractJobTestSubclassStub::$onQueue = 'child-queue';
+        // Routing only the parent does not implicitly route the subclass.
+        $this->containerWithRoutes([AbstractJobTestStub::class => 'parent-queue']);
 
         $this->assertSame('parent-queue', (new AbstractJobTestStub())->queue);
-        $this->assertSame('child-queue', (new AbstractJobTestSubclassStub())->queue);
+        $this->assertNull((new AbstractJobTestSubclassStub())->queue);
     }
 }
 
 class AbstractJobTestStub extends AbstractJob
 {
-    public static ?string $onQueue = null;
+}
+
+class AbstractJobTestOtherStub extends AbstractJob
+{
 }
 
 class AbstractJobTestSubclassStub extends AbstractJobTestStub
 {
-    public static ?string $onQueue = null;
 }
