While trying to update the flagrow/mail-drivers
extension I noticed it would't work correctly on beta 8.
In this extension, there's a special use case:
- Flarum registers Laravel's mail provider
- That provider includes drivers not handled by Flarum
- The Flagrow extension registers the settings needed in Laravel's provider
This works because the mailer is registered as a singleton, and should only be resolved later when needed. We have the freedom to make changes to the container because those singletons are used. We can even replace the singletons with something else.
Now... The problem is that when my extension's service providers run, the mail driver appears to already have been booted. So I have no way at all to alter the settings it uses when booting.
The culprit are multiple core extensions, but it could also be third-party extensions.
In the likes and suspend extensions, listeners are registered. Those listeners have a constructor and require NotificationSyncer
, which resolves NotificationMailer
, which resolves Mailer
and therefore cause the whole mailing part of Flarum to boot as soon as the listener is instantiated.
There are multiple issues:
- This boots the mailer very early, making it impossible to configure later. And this will depend on the order extensions were loaded, as extensions loading before those won't have it booted, while extensions loading after will already have the mailer booted
- This has some performance impact. The mailer is booted every time while rarely being used
- This happens during every single request
Here it's the Mailer, but such performance issues can probably be seen with other classes registered in the container.
I wanted to discuss what we should do here.
My first idea is to submit a PR to all those core extensions and remove the constructor dependencies and replace them with app()
make calls when actually needed.
Or in this case we could also make NotificationSyncer
responsible for loading the mail driver only when needed. Then it doesn't matter too much if it's resolved in the constructor of listeners.
What approach should we take ? Should we maybe remove (in core exts)/discourage (in general) the use of constructor dependencies in event listeners for all extensions ?
I've historically been a big fan of the constructor dependencies being resolved through the container because of the readability it gives. But when used in listeners I'm under the impression this often cause many unneeded resources to be resolved.