Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wip queue implementation #1096

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,12 @@
"illuminate/container": "5.1.*",
"illuminate/contracts": "5.1.*",
"illuminate/database": "^5.1.31",
"illuminate/encryption": "5.1.*",
"illuminate/events": "5.1.*",
"illuminate/filesystem": "5.1.*",
"illuminate/hashing": "5.1.*",
"illuminate/mail": "5.1.*",
"illuminate/queue": "5.1.*",
"illuminate/support": "5.1.*",
"illuminate/validation": "5.1.*",
"illuminate/view": "5.1.*",
Expand Down
1 change: 1 addition & 0 deletions src/Core/CoreServiceProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ public function register()
$this->app->register('Flarum\Core\Notification\NotificationServiceProvider');
$this->app->register('Flarum\Core\Search\SearchServiceProvider');
$this->app->register('Flarum\Formatter\FormatterServiceProvider');
$this->app->register('Flarum\Core\Queue\QueueServiceProvider');
}

protected function registerAvatarsFilesystem()
Expand Down
56 changes: 56 additions & 0 deletions src/Core/Jobs/MailJob.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
<?php

/*
* This file is part of Flarum.
*
* (c) Toby Zerner <toby.zerner@gmail.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Flarum\Core\Jobs;

use Flarum\Core\Queue\AbstractJob;
use Illuminate\Contracts\Mail\Mailer;
use Illuminate\Mail\Message;

class MailJob extends AbstractJob
{
/**
* Body of the mail message.
*
* @var string
*/
protected $body;

/**
* Receiver email address.
*
* @var string
*/
protected $to;

/**
* Subject of the mail message.
*
* @var string
*/
protected $subject;

public function __construct($subject, $body, $to)
{
$this->subject = $subject;
$this->body = $body;
$this->to = $to;
}

public function handle(Mailer $mailer)
{
$mailer->raw($this->body, function (Message $message) {
$message
->to($this->to)
->subject($this->subject);
});
}
}
28 changes: 17 additions & 11 deletions src/Core/Listener/EmailConfirmationMailer.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,22 @@

namespace Flarum\Core\Listener;

use Flarum\Core;
use Flarum\Core\EmailToken;
use Flarum\Core\Jobs\MailJob;
use Flarum\Core\Support\DispatchJobsTrait;
use Flarum\Core\User;
use Flarum\Event\UserEmailChangeWasRequested;
use Flarum\Event\UserWasRegistered;
use Flarum\Forum\UrlGenerator;
use Flarum\Settings\SettingsRepositoryInterface;
use Illuminate\Contracts\Bus\Dispatcher as JobDispatcher;
use Illuminate\Contracts\Events\Dispatcher;
use Illuminate\Contracts\Mail\Mailer;
use Illuminate\Mail\Message;
use Symfony\Component\Translation\TranslatorInterface;

class EmailConfirmationMailer
{
use DispatchJobsTrait;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

Copy link
Member Author

@luceos luceos Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be fair, that one at least encapsulates some logic ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must have missed that 👍

/**
* @var SettingsRepositoryInterface
*/
Expand All @@ -50,13 +52,15 @@ class EmailConfirmationMailer
* @param Mailer $mailer
* @param UrlGenerator $url
* @param TranslatorInterface $translator
* @param JobDispatcher $queue
*/
public function __construct(SettingsRepositoryInterface $settings, Mailer $mailer, UrlGenerator $url, TranslatorInterface $translator)
public function __construct(SettingsRepositoryInterface $settings, Mailer $mailer, UrlGenerator $url, TranslatorInterface $translator, JobDispatcher $queue)
{
$this->settings = $settings;
$this->mailer = $mailer;
$this->url = $url;
$this->translator = $translator;
$this->queue = $queue;
}

/**
Expand All @@ -83,10 +87,11 @@ public function whenUserWasRegistered(UserWasRegistered $event)

$body = $this->translator->trans('core.email.activate_account.body', $data);

$this->mailer->raw($body, function (Message $message) use ($user, $data) {
$message->to($user->email);
$message->subject('['.$data['{forum}'].'] '.$this->translator->trans('core.email.activate_account.subject'));
});
$this->dispatch(new MailJob(
'['.$data['{forum}'].'] '.$this->translator->trans('core.email.activate_account.subject'),
$body,
$user->email
));
}

/**
Expand All @@ -99,10 +104,11 @@ public function whenUserEmailChangeWasRequested(UserEmailChangeWasRequested $eve

$body = $this->translator->trans('core.email.confirm_email.body', $data);

$this->mailer->raw($body, function (Message $message) use ($email, $data) {
$message->to($email);
$message->subject('['.$data['{forum}'].'] '.$this->translator->trans('core.email.confirm_email.subject'));
});
$this->dispatch(new MailJob(
'['.$data['{forum}'].'] '.$this->translator->trans('core.email.confirm_email.subject'),
$body,
$email
));
}

/**
Expand Down
22 changes: 22 additions & 0 deletions src/Core/Queue/AbstractJob.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<?php

/*
* This file is part of Flarum.
*
* (c) Toby Zerner <toby.zerner@gmail.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Flarum\Core\Queue;

use Illuminate\Contracts\Bus\SelfHandling;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Queue\InteractsWithQueue;
use Illuminate\Queue\SerializesModels;

abstract class AbstractJob implements ShouldQueue, SelfHandling
{
use InteractsWithQueue, SerializesModels;
}
37 changes: 37 additions & 0 deletions src/Core/Queue/QueueManager.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
<?php

/*
* This file is part of Flarum.
*
* (c) Toby Zerner <toby.zerner@gmail.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Flarum\Core\Queue;

use Illuminate\Queue\QueueManager as Manager;

class QueueManager extends Manager
{
/**
* {@inheritdoc}
*/
protected function getConfig($name)
{
if ($name === 'sync') {
return ['driver' => 'sync'];
}

return parent::getConfig($name);
}

/**
* {@inheritdoc}
*/
public function getDefaultDriver()
{
return 'sync';
}
}
78 changes: 78 additions & 0 deletions src/Core/Queue/QueueServiceProvider.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
<?php

/*
* This file is part of Flarum.
*
* (c) Toby Zerner <toby.zerner@gmail.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Flarum\Core\Queue;

use Illuminate\Bus\Dispatcher;
use Illuminate\Contracts\Bus\Dispatcher as DispatcherContract;
use Illuminate\Encryption\Encrypter;
use Illuminate\Encryption\McryptEncrypter;
use Illuminate\Queue\QueueServiceProvider as Provider;

class QueueServiceProvider extends Provider
{
/**
* {@inheritdoc}
*/
protected function registerManager()
{
$this->registerEncrypter();
$this->registerQueue();
$this->registerDispatcher();
}

protected function registerEncrypter()
{
$this->app->singleton('encrypter', function ($app) {
$key = md5($app->url());

$cipher = 'AES-256-CBC';

if (Encrypter::supported($key, $cipher)) {
return new Encrypter($key, $cipher);
} elseif (McryptEncrypter::supported($key, $cipher)) {
return new McryptEncrypter($key, $cipher);
} else {
throw new RuntimeException('No supported encrypter found. The cipher and / or key length are invalid.');
}
});
}

protected function registerQueue()
{
$this->app->singleton('queue', function ($app) {
// Once we have an instance of the queue manager, we will register the various
// resolvers for the queue connectors. These connectors are responsible for
// creating the classes that accept queue configs and instantiate queues.
$manager = new QueueManager($app);

// We've disabled all other queue connectors for now. Once we're able to
// configure any of the other connectors, we can easily reset the old
// behavior of the parent provider by registering all connectors.
$this->registerSyncConnector($manager);

return $manager;
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@franzliedke so you want me to only set up the default connection and then call the QueueServiceProvider of Illuminate?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


$this->app->singleton('queue.connection', function ($app) {
return $app['queue']->connection();
});
}

protected function registerDispatcher()
{
$this->app->singleton(DispatcherContract::class, function ($app) {
return new Dispatcher($app, function () use ($app) {
return $app['queue']->connection();
});
});
}
}
32 changes: 32 additions & 0 deletions src/Core/Support/DispatchJobsTrait.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php

/*
* This file is part of Flarum.
*
* (c) Toby Zerner <toby.zerner@gmail.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Flarum\Core\Support;

use Flarum\Core\Queue\AbstractJob;
use Illuminate\Contracts\Bus\Dispatcher;

trait DispatchJobsTrait
{
/**
* @var Dispatcher
*/
protected $queue;

/**
* @param AbstractJob $job
* @return mixed
*/
public function dispatch(AbstractJob $job)
{
return $this->queue->dispatch($job);
}
}