Page MenuHomePhabricator
Create Task
Maniphest T8216

Make mailKey-related errors more obvious when testing new mail code
Closed, WontfixPublic
Actions

Assigned To
epriestley
Authored By
hach-que
May 16 2015, 3:31 AM
Tags
Referenced Files
F406208: pasted_file
May 16 2015, 3:31 AM
Subscribers
btrahan
hach-que
tycho.tatitscheff
Tokens
"Doubloon" token, awarded by btrahan.

Description

I got this when editing the details of a project (rebased against HEAD this morning):

pasted_file (808×1 px, 155 KB)

It looks like the changes were actually saved though, based on viewing the project details afterwards.

Revisions and Commits

rP Phabricator
AuditedrP1f6b53be0e0d Populate mail keys for new projects
AuditedrP72a896f772b7 Add missing mail keys to Projects

Event Timeline

hach-que created this task.May 16 2015, 3:31 AM
hach-que assigned this task to epriestley.
hach-que raised the priority of this task from to Needs Triage.
hach-que updated the task description. (Show Details)
hach-que added a project: Projects.
hach-que added a subscriber: hach-que.
epriestley added a subscriber: btrahan.May 16 2015, 4:43 AM

I'll headed to bed right now but I'll probably fix this tomorrow morning and cowboy commit it before deploying, just a heads up for when that lands.

epriestley triaged this task as Normal priority.May 16 2015, 1:09 PM
epriestley added a commit: rP72a896f772b7: Add missing mail keys to Projects.May 16 2015, 1:13 PM
epriestley added a comment.May 16 2015, 3:01 PM

We should fix this permanently since this is something like the third time we've hit it and it's super dumb that this mistake is possible (you'll hit this pathway only if your email is configured in a certain way). Some possible approaches:

  1. Via formal typing: Add some kind of interface to these objects to require they have getMailKey().
  2. Via informal typing: Just always call this method on all pathways -- even if we aren't actually going to use it -- to make sure it exists and works.

(1) is arguably the "right" way but might be really messy and require defining a whole pile of useless getMailKey() stubs. (2) is a little sketchier but maybe pretty defensible in this case.

tycho.tatitscheff added a subscriber: tycho.tatitscheff.May 17 2015, 2:27 AM

After this fix still get an unhandled exception :

#1364: Field 'mailKey' doesn't have a default value
Stack trace:
phlog called at [/srv/phabricator/phabricator/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php:226]
AphrontDefaultApplicationConfiguration::handleException called at [/srv/phabricator/phabricator/src/aphront/configuration/AphrontApplicationConfiguration.php:230]
AphrontApplicationConfiguration::processRequest called at [/srv/phabricator/phabricator/src/aphront/configuration/AphrontApplicationConfiguration.php:140]
AphrontApplicationConfiguration::runHTTPRequest called at [/srv/phabricator/phabricator/webroot/index.php:19]
epriestley added a commit: rP1f6b53be0e0d: Populate mail keys for new projects.May 17 2015, 2:42 AM
epriestley added a comment.May 17 2015, 2:42 AM

Thanks.

tycho.tatitscheff added a comment.May 17 2015, 2:49 AM

Re

#1048: Column 'mailKey' cannot be null
Stack trace:
phlog called at [/srv/phabricator/phabricator/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php:226]
AphrontDefaultApplicationConfiguration::handleException called at [/srv/phabricator/phabricator/src/aphront/configuration/AphrontApplicationConfiguration.php:230]
AphrontApplicationConfiguration::processRequest called at [/srv/phabricator/phabricator/src/aphront/configuration/AphrontApplicationConfiguration.php:140]
AphrontApplicationConfiguration::runHTTPRequest called at [/srv/phabricator/phabricator/webroot/index.php:19]
tycho.tatitscheff added a comment.May 17 2015, 2:51 AM

Simple curiosity what is this (ennoying) mailKey ? A new feature ?

epriestley renamed this task from Exception when editing project detail to Make mailKey-related errors more obvious when testing new mail code.May 18 2015, 4:34 PM
epriestley lowered the priority of this task from Normal to Low.
btrahan awarded a token.May 19 2015, 5:54 PM
epriestley closed this task as Wontfix.May 19 2015, 6:16 PM

I can't actually figure out what configuration avoids this error -- metamta.public-replies on and off both try to call getMailKey() for me.

I wrote part of a diff to introduce "generators" instead of requiring save() overrides -- it's probably a small step forward, but feels heavy without other use cases. I'm not going to pursue it for now:

https://secure.phabricator.com/differential/diff/31120/

Particularly, it wouldn't really make this less error-prone (you can forget to provide a generator as easily as you can forget to provide a value in save()) and defaulting the generator onto any property called "mailKey" felt too magical to me.

I'm just going to shelve this for now and maybe dig it up the next time I'm touching mail or come up with some other use for default generators.

epriestley added a comment.May 19 2015, 6:18 PM

There's also no actual technical reason that objects legitimately need to have a mailKey at the PhabricatorMailReplyHandler level right now.

btrahan added a comment.May 19 2015, 6:25 PM

Looking through my test instance from when I was building the offending diff that broke projects saves, looks like I was able to re-name projects and add members / watchers without getting mail key errors. I haven't really done any configuration to mail on this test instance and it generally doesn't actually send mail. Anyway, I think I can just remember next time I add mail functionality, and otherwise thanks for fixing projects!

epriestley added a comment.May 19 2015, 6:26 PM

Yeah, on further thought I suspect the magical config is just metamta.reply-handler-domain not being set.

Phabricator · Built by Phacility