Changeset View
Standalone View
src/applications/phame/mail/PhamePostMailReceiver.php
| <?php | <?php | ||||
| final class PhamePostMailReceiver | final class PhamePostMailReceiver | ||||
| extends PhabricatorObjectMailReceiver { | extends PhabricatorObjectMailReceiver { | ||||
| public function isEnabled() { | public function isEnabled() { | ||||
| return PhabricatorApplication::isClassInstalled( | return PhabricatorApplication::isClassInstalled( | ||||
| 'PhabricatorPhameApplication'); | 'PhabricatorPhameApplication'); | ||||
| } | } | ||||
| protected function getObjectPattern() { | protected function getObjectPattern() { | ||||
| return 'J[1-9]\d*'; | return 'J[1-9]\d*'; | ||||
| } | } | ||||
| protected function loadObject($pattern, PhabricatorUser $viewer) { | protected function loadObject($pattern, PhabricatorUser $viewer) { | ||||
| $id = (int)substr($pattern, 4); | $id = (int)substr($pattern, 1); | ||||
amckinley: I'm only thinking about this because I've read similar changes about 30 times in this diff, but… | |||||
Done Inline ActionsYeah, the general structure of this isn't great. Some objects have a mail pattern which isn't a monogram, usually because they don't have a monogram. ANSR is one example. ObjectQuery can load M123, C123, etc., but not ANSR123. A better approach might be to separate this into two parts:
So instead of D123+10+abcdef@phabricator.com, we'd get a reply address like D+123+10+abcdef@phabricator.com (one extra + after D at the beginning). This is like a tiny tiny bit worse for users, maybe? But probably no one cares and these addresses are clearly not human-readable. Then parsing the pieces out would be completely standard. But the whole ReplyHandler vs MailReceiver tree is kind of not great, and I think they should really be one class. That's just kind of a lot of scope to add. T11934 may be a better opportunity to fix this. epriestley: Yeah, the general structure of this isn't great.
Some objects have a mail pattern which isn't… | |||||
Done Inline ActionsWe could also use PHIDs, but in theory it's possible for the same object to have multiple addresses or something, where you send to one address to add comments and another address to update the mock or something, I suppose. This will probably never happen. epriestley: We could also use PHIDs, but //in theory// it's possible for the same object to have multiple… | |||||
| return id(new PhamePostQuery()) | return id(new PhamePostQuery()) | ||||
| ->setViewer($viewer) | ->setViewer($viewer) | ||||
| ->withIDs(array($id)) | ->withIDs(array($id)) | ||||
| ->executeOne(); | ->executeOne(); | ||||
| } | } | ||||
| protected function getTransactionReplyHandler() { | protected function getTransactionReplyHandler() { | ||||
| return new PhamePostReplyHandler(); | return new PhamePostReplyHandler(); | ||||
| } | } | ||||
| } | } | ||||
I'm only thinking about this because I've read similar changes about 30 times in this diff, but why is it the mail handler's responsibility to turn a callsign into an ID? Shouldn't the application itself have a standardized way of doing this?