Queue support #1773
Queue support #1773
Conversation
|
@flarum/core I have been considering adding a Configuring event here as well, to immediately override the default queue. Instead of needing to work with a |
|
Looks good, I think |
|
I agree with @tobscure, the event shouldn't be necessary. |
src/Queue/QueueServiceProvider.php
Outdated
| }); | ||
|
|
||
| $this->app->singleton(Factory::class, function ($app) { | ||
| $manager = new QueueManager($app); |
There was a problem hiding this comment.
Do we really benefit from the manager here? I don't think we need to support multiple queue backends, and using the manager just means that we'll have to translate extension configuration (e.g. settings in a GUI) into Laravel config and let the manager turn that into a connector instance. Why not turn our settings into a driver directly?
There was a problem hiding this comment.
Makes sense, didn't even consider that. We don't even need the configuration, any listener that binds into the resolving of this binding can change what queue driver is being used.
There was a problem hiding this comment.
Okay so simplifying the binding doesn't work, because the Mailer instance expects a Factory, not an implementation..
There was a problem hiding this comment.
The Validator too. I've reverted my latest commits. Please review again @flarum/core .
luceos
force-pushed
the
dk/978-queue-implementation
branch
from
a9724ff
to
c47a3d7
Compare
|
Never mind, testing this for @Bokt and the commands aren't showing yet. |
…into dk/978-queue-implementation
[ci skip] [skip ci]
…into dk/978-queue-implementation
[ci skip] [skip ci]
|
This is starting to become very ugly. The queue worker needs a cache and does so by resolving protected function runWorker($connection, $queue)
{
$this->worker->setCache($this->laravel['cache']->driver());
return $this->worker->{$this->option('once') ? 'runNextJob' : 'daemon'}(
$connection, $queue, $this->gatherWorkerOptions()
);
}I'll try to get an implementation done and then review how to improve all this. |
|
@franzliedke subtle request to give this PR a review 🤞 |
|
Oh wow, this part of Laravel is indeed a bit too highly coupled. Thankfully, change is on the horizon. As we can't wait for that, I will experiment with alternatives in the meantime. |
The `Worker` class doesn't really need it. It only needs a connection factory and one additional method (for detecting whether the app is in maintenance mode), but unfortunately type-hints the `QueueManager` class. Laravel 6.0 will improve this, so let's hack our way around it for now by building a minimal subclass of `QueueManager` that only supports those two methods. Once we upgrade to Laravel 6.0, the hack can simply be removed.
Again, Laravel wants the whole manager even though it only needs a repository instance. Let's build a minimal workaround again until this situation is resolved. (I submitted a PR to Laravel.)
That concept doesn't apply here - extensions will have to take care of running migrations if needed.
[ci skip] [skip ci]
|
I pushed a few commits that let us avoid the managers. Let me know what you think. |
Implementation of clean queue handling, by default sync is used
Implementation of clean queue handling, by default sync is used
Relates to #978
TO DO
Changes proposed in this pull request:
This PR adds queue ability, it sets the driver to sync by default. It will also enable the commands if the driver is something else than the sync or null driver.
Reviewers should focus on:
Does this implementation comply to our conventions and do you foresee issues in extensibility.
Confirmed
php vendor/bin/phpunit).Required changes: