Page MenuHomePhabricator

Resurrect setup check for cluster.mailers
ClosedPublic

Authored by amckinley on Jan 11 2019, 10:27 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 12, 8:48 AM
Unknown Object (File)
Mon, Dec 9, 12:33 PM
Unknown Object (File)
Tue, Dec 3, 7:34 PM
Unknown Object (File)
Tue, Dec 3, 11:44 AM
Unknown Object (File)
Sun, Dec 1, 7:42 AM
Unknown Object (File)
Mon, Nov 25, 6:55 AM
Unknown Object (File)
Sat, Nov 23, 10:23 AM
Unknown Object (File)
Nov 21 2024, 3:32 PM
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 21509
Build 29303: Run Core Tests
Build 29302: 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.