Core: Deleting driver from SMTP Server cause blank page/error

[Zeokat]

Bug report

• Version of Flarum: v0.1.0-beta.6 and latest development version
• Website URL where the bug is visible: https://devflarum.xyzz.work
• The webserver you are running: tested on apache and nginx
• PHP version: 7.0
• Hosted environment: local machine and VPS
• Hosting provider: N/A

Flarum info

zeokat@ubuntu:/var/www/html/flarum$ php flarum info
Flarum core 0.1.0-beta.6
PHP 7.0.15-0ubuntu0.16.04.4
Loaded extensions: Core, date, libxml, openssl, pcre, zlib, filter, hash, pcntl, Reflection, SPL, session, standard, mysqlnd, PDO, xml, calendar, ctype, curl, dom, mbstring, fileinfo, ftp, gd, gettext, iconv, intl, json, exif, mcrypt, mysqli, pdo_mysql, Phar, posix, readline, shmop, SimpleXML, sockets, sysvmsg, sysvsem, sysvshm, tokenizer, wddx, xmlreader, xmlwriter, xsl, Zend OPcache
EXT flarum-approval v0.1.0-beta.6
EXT flarum-bbcode v0.1.0-beta.5
EXT flarum-emoji v0.1.0-beta.6
EXT flarum-english v0.1.0-beta.6
EXT flarum-flags v0.1.0-beta.6
EXT flarum-likes v0.1.0-beta.6
EXT flarum-lock v0.1.0-beta.6
EXT flarum-markdown v0.1.0-beta.5
EXT flarum-mentions v0.1.0-beta.6
EXT flarum-sticky v0.1.0-beta.6
EXT flarum-subscriptions v0.1.0-beta.6
EXT flarum-suspend v0.1.0-beta.6
EXT flarum-tags v0.1.0-beta.7
Base URL: http://192.168.247.128/flarum
Installation path: /var/www/html/flarum

Additional comments

After delete driver field into SMTP Server settings, we get an error:

Warning: Missing argument 1 for Illuminate\Support\Manager::createDriver(), called in /www/devflarum/forum/vendor/illuminate/support/Manager.php on line 87 and defined in /www/devflarum/forum/vendor/illuminate/support/Manager.php on line 77

Log files

N/A

[luceos]

Aka driver field should be mandatory or default back to smtp.

[Zeokat]

Maybe add a dropdown list with all available and usable options will be the right choice. Extracted from here, options will be:

• SMTP
• Mailgun
• Mandrill
• SparkPost
• Amazon SES
• PHP's mail function
• sendmail

[luceos]

Drivers could be dynamically added via a package, not sure that whitelisting a few is smart. Unless extensions are allowed to add to the whitelist.

[gwillem]

One FreeFlarum user had his forum bricked by entering a bogus smtp driver (he entered smtp.sendgrid.net). So forcing to choose from a set, or validation upon saving, seems useful.

[LukBukkit]

I got the same error as mentioned above:

Error booting Flarum: Too few arguments to function Illuminate\Support\Manager::createDriver(), 0 passed in D:\lukas\Coding\PhpstormProjects\flarum\flarum\vendor\illuminate\support\Manager.php on line 88 and exactly 1 expected

This error is caused by InstalledSite.php#L147.
If there's no mail_driver entry in the database table settings the result of the get->('mail_driver') query is NULL. This NULL driver can't be found by the ::createDriver() function.

And also somehow while installing, it doesn't finishes the migration process.
The last migration which is stored is 2018_01_18_133100_change_notifications_add_foreign, but this migration wasn't finished. So maybe it's a problem caused by the database.

[LukBukkit]

Okay my installation process failed because of extensions, which aren't updated (flarum-ext-markdown and flarum-ext-bbcode).

The same error appreared for both of these extensions:

Enabling extension: flarum-markdown

In Container.php line 752:

  Class cache.store does not exist

I'll look into these errors.

--- Edit
But it's working, when I'm installing the extension afterwards.

[franzliedke]

Hey folks, I just pushed a change to how mail drivers are registered and configured.

This should make it much easier to build in validation of mail settings.

Other ideas that should now be pretty easy:

• dropdown of possible drivers (see #1712)
• warning message for admins when a mail driver (such as "log") is configured that does not actually send emails
• prevent saving invalid configuration
• adding some (or all) of the drivers from flagrow/mail-drivers into core

Because of how annoying this is, I'd like to see this issue fixed in the next version, possibly including some of these suggestions. Nice chance for community members to step in. :grinning:

[luceos]

@franzliedke related to https://github.com/flarum/core/commit/ac5e26a254d89e21bd4c115b6cbd40338e2e4b4b#commitcomment-32420176

What is the suggested solution to get this fixed for beta 9? Right now it seems configuring mailgun for instance (needs key/secret/domain information) is impossible due to the exception thrown. Should I give the binding solution a go?

[franzliedke]

Should I give the binding solution a go?

Yes, please.

[franzliedke]

@luceos @clarkwinkelmann I just tweaked this a bit more: mail.supported_drivers is now a mapping from driver names to class names implementing those drivers, and is the main entrypoint for adding additional drivers (replacing the approach from #1757).

~~~php
// For now, extending through private API
$container->extend('mail.supported_drivers', function ($drivers) {
return $drivers + ['fancymail' => MyFancyMailDriver::class];
});

// In the future, we may offer an extender for this use case.
~~~

Next steps: Add more methods to the interface for other use cases, such as defining and validating required configuration. When that is done, getting the remaining email drivers from flagrow/mail-drivers into core should be very easy. (For beta.9, we may simply want to update that extension to use the new DriverInterface, and for the next beta, we can bring those drivers into core.)

[franzliedke]

Okay, now I implemented the dropdown of available mail drivers, and made sure the SMTP settings are only shown when that driver is selected.

Full TODO list:

• [x] dropdown of possible drivers (see #1712)
• [x] dynamic fields per driver
• [ ] improved help texts and example values
• [ ] warning message for admins when a mail driver (such as "log") is configured that does not actually send emails
• [ ] validation of configuration
• [x] more drivers (e.g. from flagrow/mail-drivers)

[luceos]

First of all, initially I had no idea what approach you had in mind, now that you laid it out with subtasks it's much easier to comprehend. Thank you!

Secondly do you really want to bloat core with all those drivers? It makes more sense to move them to their own extension and allow separate installation to suit the webmaster needs.

Thirdly, with the tasks laid out is that something to delay beta 9 for?

[franzliedke]

Thanks for the reminder, I still had two unfinished commits lying around. Just pushed those and we now have support for more Laravel drivers, and their configuration through the admin UI. :)

[franzliedke]

I am happy with this. The rest can be taken care of in #1762, #1763 and the already existing #1655.