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

Deleting driver from SMTP Server cause blank page/error #1169

Closed
Zeokat opened this issue Apr 25, 2017 · 14 comments
Closed

Deleting driver from SMTP Server cause blank page/error #1169

Zeokat opened this issue Apr 25, 2017 · 14 comments

Comments

@Zeokat
Copy link
Contributor

Zeokat commented Apr 25, 2017

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
Copy link
Member

luceos commented Apr 26, 2017

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

@Zeokat
Copy link
Contributor Author

Zeokat commented Apr 26, 2017

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
Copy link
Member

luceos commented Apr 28, 2017

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
Copy link
Contributor

gwillem commented Dec 30, 2017

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
Copy link
Contributor

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
Copy link
Contributor

lukbukkit commented Oct 1, 2018

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
Copy link
Contributor

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:

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. 😀

@franzliedke franzliedke added this to the 0.1.0-beta.9 milestone Dec 20, 2018
franzliedke added a commit that referenced this issue Dec 20, 2018
This can be used for e.g. validation, or a dropdown in the frontend.
It can also be extended by extensions, such as flagrow/mail-drivers.

Refs #1169.
@luceos
Copy link
Member

luceos commented Mar 7, 2019

@franzliedke related to ac5e26a#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
Copy link
Contributor

Should I give the binding solution a go?

Yes, please.

@luceos luceos modified the milestones: 0.1.0-beta.10, 0.1.0-beta.9 Mar 8, 2019
@luceos luceos self-assigned this Mar 8, 2019
franzliedke added a commit that referenced this issue Mar 13, 2019
This adds an interface for mail drivers to implement, defining several
methods that we need throughout Flarum to configure, validate and use
the various email drivers we can support through Laravel.

More mail drivers can be added by `extend()`ing the container binding
"mail.supported_drivers" with an arbitrary key and the name of a class
that implements our new `DriverInterface`.

This will ensure that drivers added by extensions can be properly built
and validated, even in the frontend.
@franzliedke
Copy link
Contributor

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

// 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
Copy link
Contributor

franzliedke commented Mar 15, 2019

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:

  • dropdown of possible drivers (see Mail drivers should be a dropdown #1712)
  • 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
  • more drivers (e.g. from flagrow/mail-drivers)

franzliedke added a commit to flarum/lang-english that referenced this issue Mar 16, 2019
@luceos
Copy link
Member

luceos commented Mar 19, 2019

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 added a commit that referenced this issue Mar 19, 2019
This includes an API endpoint for fetching the list of possible
drivers and their configuration fields. In the future, this can
be extended to include more meta information about each field.
franzliedke added a commit to flarum/lang-english that referenced this issue Mar 19, 2019
@franzliedke
Copy link
Contributor

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
Copy link
Contributor

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

wzdiyb pushed a commit to wzdiyb/core that referenced this issue Feb 16, 2020
This can be used for e.g. validation, or a dropdown in the frontend.
It can also be extended by extensions, such as flagrow/mail-drivers.

Refs flarum#1169.
wzdiyb pushed a commit to wzdiyb/core that referenced this issue Feb 16, 2020
This adds an interface for mail drivers to implement, defining several
methods that we need throughout Flarum to configure, validate and use
the various email drivers we can support through Laravel.

More mail drivers can be added by `extend()`ing the container binding
"mail.supported_drivers" with an arbitrary key and the name of a class
that implements our new `DriverInterface`.

This will ensure that drivers added by extensions can be properly built
and validated, even in the frontend.
wzdiyb pushed a commit to wzdiyb/core that referenced this issue Feb 16, 2020
wzdiyb pushed a commit to wzdiyb/core that referenced this issue Feb 16, 2020
wzdiyb pushed a commit to wzdiyb/core that referenced this issue Feb 16, 2020
This includes an API endpoint for fetching the list of possible
drivers and their configuration fields. In the future, this can
be extended to include more meta information about each field.
wzdiyb pushed a commit to wzdiyb/core that referenced this issue Feb 16, 2020
askvortsov1 pushed a commit to flarum/lang-english that referenced this issue Mar 11, 2022
askvortsov1 pushed a commit to flarum/lang-english that referenced this issue Mar 11, 2022
askvortsov1 pushed a commit to flarum/lang-english that referenced this issue May 10, 2022
askvortsov1 pushed a commit to flarum/lang-english that referenced this issue May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants