Deleting driver from SMTP Server cause blank page/error

Zeokat commented 7 years ago

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 commented 7 years ago

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

Zeokat commented 7 years ago

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 commented 7 years ago

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 commented 6 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 flarum/framework#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 commented 5 years ago

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 commented 5 years ago

Should I give the binding solution a go?

Yes, please.

franzliedke commented 5 years ago

@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 flarum/framework#1757).

// 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 commented 5 years ago

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 flarum/framework#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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

I am happy with this. The rest can be taken care of in flarum/issue-archive#252, flarum/framework#1763 and the already existing flarum/framework#1655.

flarum / framework