wip queue implementation #1096
wip queue implementation #1096
Conversation
…aps the dispatcher is not the best way of doing this
…f the sync connector at this point only
| use Symfony\Component\Translation\TranslatorInterface; | ||
|
|
||
| class EmailConfirmationMailer | ||
| { | ||
| use DispatchJobsTrait; |
There was a problem hiding this comment.
Honestly, I don't think we need this trait, if all it does is shortening $this->queue->dispatch() to $this->dispatch(), at the cost of one more step to find out where this method is coming from, and why we need to inject the seemingly-unused $queue attribute. :)
There was a problem hiding this comment.
You might want to create an issue then to remove the need of a DispatchesEventTrait as well, it does exactly the same useless shortcut :) It might make more sense if the trait would have loaded the bound Dispatcher from my perspective.
There was a problem hiding this comment.
To be fair, that one at least encapsulates some logic ;)
| $this->registerSyncConnector($manager); | ||
|
|
||
| return $manager; | ||
| }); |
There was a problem hiding this comment.
I don't think we need the QueueManager at all,. Just configure the correct �queue.connection binding here, and we are good. Once we make this configurable (to switch between the default "sync" and other - real - drivers), this would just be a different instance, but it would be configured here,
There was a problem hiding this comment.
@franzliedke so you want me to only set up the default connection and then call the QueueServiceProvider of Illuminate?
There was a problem hiding this comment.
No. Only register the connection instance. We don't need a manager at all, I think, because we don't need the dynamic switching between different connections.
There was a problem hiding this comment.
For simplicity purposes we can go that direction. Although I would know of situations where an extension might be happy to use separate queue drivers. Ok, thanks!
|
Good job on this! 👏 Just some minor things... |
|
Why can't we use Laravel Mailer's built-in queue functionality? https://laravel.com/docs/5.3/mail#queueing-mail |
|
Because we're not on 5.3 yet. ;) |
|
It exists in 5.1, with a different API: https://laravel.com/docs/5.1/mail#queueing-mail |
|
My concern is that this proposed MailJob API is very restricted (you can't customise the Message object), and once we start trying to open it up, we'll essentially be duplicating Laravel's API unnecessarily? |
|
@tobscure with all respect, I think we need to implement a queue initially. Moving towards mailable makes a lot of sense, but - in my opinion - would be step 2 and can be easily done once @sijad is done with his compatibility towards 5.3+. I picked the mail sending initially, because that one is blocking api request. |
|
@flarum/core Should we hold this off until @sijad has finished the upgrade to L 5.3 or 5.4 or 5.5? |
Relates to #978 , please keep all discussion (not code review related) to the issue.