Page MenuHomePhabricator

Resurrect setup check for cluster.mailers
ClosedPublic

Authored by amckinley on Jan 11 2019, 10:27 PM.
Tags
None
Referenced Files
F13061920: D19964.diff
Fri, Apr 19, 8:17 PM
F13054966: D19964.diff
Fri, Apr 19, 12:21 PM
F13052674: D19964.id47658.diff
Fri, Apr 19, 10:25 AM
F13052182: D19964.id47687.diff
Fri, Apr 19, 7:40 AM
Unknown Object (File)
Thu, Apr 18, 10:06 AM
Unknown Object (File)
Wed, Apr 17, 9:32 PM
Unknown Object (File)
Mon, Apr 15, 4:16 PM
Unknown Object (File)
Mon, Apr 15, 8:36 AM
Subscribers

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
Branch
mailers-check (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 21510
Build 29305: Run Core Tests
Build 29304: arc lint + arc unit

Event Timeline

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

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.Jan 13 2019, 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.