Page MenuHomePhabricator

Resurrect setup check for cluster.mailers
ClosedPublic

Authored by amckinley on Fri, Jan 11, 10:27 PM.

Details

Summary

D19940 removed this file entirely, which has led to at least one user who was unsure how to proceed now that cluster.mailers is required for outbound mail: https://discourse.phabricator-community.org/t/invalid-argument-supplied-for-foreach-phabricatormetamtamail-php/2287

This isn't always a setup issue for installs that don't care about sending mail, but this at least this gives a sporting chance to users who don't follow the changelogs.

Also, I'm not sure if there's a way to use pht() to generate links; right now the phurl is just in plain text.

Test Plan

Removed cluster.mailers config; observed expected setup issue.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

amckinley created this revision.Fri, Jan 11, 10:27 PM
amckinley requested review of this revision.Fri, Jan 11, 10:28 PM

This could also be rewritten as if ($no_cluster_mailers_configured && $some_deprecated_mailer_configured) to target this message more narrowly.

amckinley updated this revision to Diff 47658.Sat, Jan 12, 1:35 AM

Fix weird indentation.

epriestley accepted this revision.Sun, Jan 13, 6:33 PM

Thanks! I have a documentation revisit somewhere in queue with T8636, but the master vs off-grid-in-Tahoe timing ended up a little rough.

Since the config option already has a documentation link, we could skip the redundant one here -- I think it's okay to just point users at "cluster.mailers", and then the help there can guide them toward configuration. I'll probably beef up the cluster.mailers help a bit in an upcoming change.

(In the general case, you can generate links with pht(...) by using %s with phutil_tag(...), or use PhabricatorEnv::getDoclink(). The theoretical advantage of getDoclink() is that it could some day be configurable to point at a local documentation source rather than the upstream documentation, in case installs want to fork the documentation.)

Because this is routine during setup, consider a more first-time-setup focused name with less emphasis on the technical detail that the option is named cluster.mailers, e.g. just "Mailers Not Configured" + "You haven't configured mailers yet, so Phabricator won't be able to send outbound mail or receive inbound mail. To configure mailers..." or something along those lines. My expectation is that everyone will hit this during setup now, since we no longer default to sendmail (which was probably a bad default in the first place).

(We should probably also add this to the "Quick Start" Guide thing or maybe throw that away, but it's a bit off the beaten path and I can deal with that during the followup documentation pass.)

This revision is now accepted and ready to land.Sun, Jan 13, 6:33 PM

Also, cluster.mailers should default to array() to fix the "invalid foreach" thing. You can do that here or I can do it in a followup.

cluster.mailers should default to array()

In D19966.

This revision was automatically updated to reflect the committed changes.