From 0bf2753b88ccd66b3f4dd510cb5bda820531096f Mon Sep 17 00:00:00 2001 From: Marek Sapota Date: Fri, 14 Oct 2011 12:03:16 -0700 Subject: [PATCH 1/3] PhabricatorMailImplementationPHPMailerLiteAdapter ignores parameter in setIsHTML function. Summary: Fix PhabricatorMailImplementationPHPMailerLiteAdapter to actually use given parameter. Test Plan: Use setIsHTML with false as parameter, sent mail should be in plain text. Reviewers: jungejason Reviewed By: jungejason CC: aran, jungejason, epriestley Differential Revision: 1001 --- .../PhabricatorMailImplementationPHPMailerLiteAdapter.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/metamta/adapter/phpmailerlite/PhabricatorMailImplementationPHPMailerLiteAdapter.php b/src/applications/metamta/adapter/phpmailerlite/PhabricatorMailImplementationPHPMailerLiteAdapter.php index a4fa11b57c..343e898208 100644 --- a/src/applications/metamta/adapter/phpmailerlite/PhabricatorMailImplementationPHPMailerLiteAdapter.php +++ b/src/applications/metamta/adapter/phpmailerlite/PhabricatorMailImplementationPHPMailerLiteAdapter.php @@ -75,7 +75,7 @@ class PhabricatorMailImplementationPHPMailerLiteAdapter } public function setIsHTML($is_html) { - $this->mailer->IsHTML(true); + $this->mailer->IsHTML($is_html); return $this; } From fee718435069c0308315ccc55b72f4c99ee91ce2 Mon Sep 17 00:00:00 2001 From: Marek Sapota Date: Fri, 14 Oct 2011 12:07:29 -0700 Subject: [PATCH 2/3] Phabricator mail Test Plan: EMPTY Reviewers: aran, epriestley Reviewed By: epriestley CC: aran, mareksapota, epriestley, jungejason, nh, drnikki Differential Revision: 1002 --- .../PhabricatorMailImplementationAdapter.php | 1 + ...rMailImplementationPHPMailerLiteAdapter.php | 10 ++++++++++ ...icatorMailImplementationSendGridAdapter.php | 6 ++++++ ...habricatorMailImplementationTestAdapter.php | 9 +++++++++ .../storage/mail/PhabricatorMetaMTAMail.php | 18 ++++++++++++++++++ 5 files changed, 44 insertions(+) diff --git a/src/applications/metamta/adapter/base/PhabricatorMailImplementationAdapter.php b/src/applications/metamta/adapter/base/PhabricatorMailImplementationAdapter.php index 3d20f53d6d..72500c4da7 100644 --- a/src/applications/metamta/adapter/base/PhabricatorMailImplementationAdapter.php +++ b/src/applications/metamta/adapter/base/PhabricatorMailImplementationAdapter.php @@ -22,6 +22,7 @@ abstract class PhabricatorMailImplementationAdapter { abstract public function addReplyTo($email, $name = ''); abstract public function addTos(array $emails); abstract public function addCCs(array $emails); + abstract public function addAttachment($data, $filename, $mimetype); abstract public function addHeader($header_name, $header_value); abstract public function setBody($body); abstract public function setSubject($subject); diff --git a/src/applications/metamta/adapter/phpmailerlite/PhabricatorMailImplementationPHPMailerLiteAdapter.php b/src/applications/metamta/adapter/phpmailerlite/PhabricatorMailImplementationPHPMailerLiteAdapter.php index 343e898208..e10a23131e 100644 --- a/src/applications/metamta/adapter/phpmailerlite/PhabricatorMailImplementationPHPMailerLiteAdapter.php +++ b/src/applications/metamta/adapter/phpmailerlite/PhabricatorMailImplementationPHPMailerLiteAdapter.php @@ -55,6 +55,16 @@ class PhabricatorMailImplementationPHPMailerLiteAdapter return $this; } + public function addAttachment($data, $filename, $mimetype) { + $this->mailer->AddStringAttachment( + $data, + $filename, + 'base64', + $mimetype + ); + return $this; + } + public function addHeader($header_name, $header_value) { if (strtolower($header_name) == 'message-id') { $this->mailer->MessageID = $header_value; diff --git a/src/applications/metamta/adapter/sendgrid/PhabricatorMailImplementationSendGridAdapter.php b/src/applications/metamta/adapter/sendgrid/PhabricatorMailImplementationSendGridAdapter.php index 3d3e56cd3a..72dc3286e2 100644 --- a/src/applications/metamta/adapter/sendgrid/PhabricatorMailImplementationSendGridAdapter.php +++ b/src/applications/metamta/adapter/sendgrid/PhabricatorMailImplementationSendGridAdapter.php @@ -55,6 +55,12 @@ class PhabricatorMailImplementationSendGridAdapter return $this; } + public function addAttachment($data, $filename, $mimetype) { + throw new Exception( + 'SendGrid adapter does not currently support attachments.' + ); + } + public function addHeader($header_name, $header_value) { $this->params['headers'][] = array($header_name, $header_value); return $this; diff --git a/src/applications/metamta/adapter/test/PhabricatorMailImplementationTestAdapter.php b/src/applications/metamta/adapter/test/PhabricatorMailImplementationTestAdapter.php index abf99bf0ef..9890974492 100644 --- a/src/applications/metamta/adapter/test/PhabricatorMailImplementationTestAdapter.php +++ b/src/applications/metamta/adapter/test/PhabricatorMailImplementationTestAdapter.php @@ -61,6 +61,15 @@ class PhabricatorMailImplementationTestAdapter return $this; } + public function addAttachment($data, $filename, $mimetype) { + $this->guts['attachments'][] = array( + 'data' => $data, + 'filename' => $filename, + 'mimetype' => $mimetype + ); + return $this; + } + public function addHeader($header_name, $header_value) { $this->guts['headers'][] = array($header_name, $header_value); return $this; diff --git a/src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php index 4b4cb0a537..eabaf0d587 100644 --- a/src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php @@ -104,6 +104,15 @@ class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { return $this; } + public function addAttachment($data, $filename, $mimetype) { + $this->parameters['attachments'][] = array( + 'data' => $data, + 'filename' => $filename, + 'mimetype' => $mimetype + ); + return $this; + } + public function setFrom($from) { $this->setParam('from', $from); return $this; @@ -284,6 +293,15 @@ class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { $mailer->addHeader($header_key, $header_value); } break; + case 'attachments': + foreach ($value as $attachment) { + $mailer->addAttachment( + $attachment['data'], + $attachment['filename'], + $attachment['mimetype'] + ); + } + break; case 'body': $mailer->setBody($value); break; From 87a2987ad6940ad8e784ec72826ce67a5ec83b11 Mon Sep 17 00:00:00 2001 From: Marek Sapota Date: Fri, 14 Oct 2011 12:08:31 -0700 Subject: [PATCH 3/3] Differential mail Test Plan: EMPTY Reviewers: aran, epriestley Reviewed By: epriestley CC: aran, epriestley, mareksapota Differential Revision: 1004 --- conf/default.conf.php | 5 ++ ...ConduitAPI_differential_getdiff_Method.php | 55 +------------------ .../method/differential/getdiff/__init__.php | 1 - ...uitAPI_differential_getrevision_Method.php | 3 +- .../differential/getrevision/__init__.php | 1 - .../mail/base/DifferentialMail.php | 30 +++++++++- .../DifferentialReviewRequestMail.php | 35 ++++++++++++ .../mail/reviewrequest/__init__.php | 4 ++ .../storage/diff/DifferentialDiff.php | 52 ++++++++++++++++++ .../differential/storage/diff/__init__.php | 1 + 10 files changed, 126 insertions(+), 61 deletions(-) diff --git a/conf/default.conf.php b/conf/default.conf.php index 2b9eaf0fd1..4c44ce01cd 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -232,6 +232,11 @@ return array( // Prefix prepended to mail sent by Differential. 'metamta.differential.subject-prefix' => '[Differential]', + // Set this to true if you want patches to be attached to mail from + // Differential. This won't work if you are using SendGrid as your mail + // adapter. + 'metamta.differential.attach-patches' => false, + // By default, Phabricator generates unique reply-to addresses and sends a // separate email to each recipient when you enable reply handling. This is // more secure than using "From" to establish user identity, but can mean diff --git a/src/applications/conduit/method/differential/getdiff/ConduitAPI_differential_getdiff_Method.php b/src/applications/conduit/method/differential/getdiff/ConduitAPI_differential_getdiff_Method.php index 1d1c24c7be..2968079017 100644 --- a/src/applications/conduit/method/differential/getdiff/ConduitAPI_differential_getdiff_Method.php +++ b/src/applications/conduit/method/differential/getdiff/ConduitAPI_differential_getdiff_Method.php @@ -72,60 +72,7 @@ class ConduitAPI_differential_getdiff_Method extends ConduitAPIMethod { $changeset->attachHunks($changeset->loadHunks()); } - return $this->createDiffDict($diff); - } - - public static function createDiffDict(DifferentialDiff $diff) { - $dict = array( - 'id' => $diff->getID(), - 'parent' => $diff->getParentRevisionID(), - 'revisionID' => $diff->getRevisionID(), - 'sourceControlBaseRevision' => $diff->getSourceControlBaseRevision(), - 'sourceControlPath' => $diff->getSourceControlPath(), - 'unitStatus' => $diff->getUnitStatus(), - 'lintStatus' => $diff->getLintStatus(), - 'changes' => array(), - 'properties' => array(), - ); - - foreach ($diff->getChangesets() as $changeset) { - $hunks = array(); - foreach ($changeset->getHunks() as $hunk) { - $hunks[] = array( - 'oldOffset' => $hunk->getOldOffset(), - 'newOffset' => $hunk->getNewOffset(), - 'oldLength' => $hunk->getOldLen(), - 'newLength' => $hunk->getNewLen(), - 'addLines' => null, - 'delLines' => null, - 'isMissingOldNewline' => null, - 'isMissingNewNewline' => null, - 'corpus' => $hunk->getChanges(), - ); - } - $change = array( - 'metadata' => $changeset->getMetadata(), - 'oldPath' => $changeset->getOldFile(), - 'currentPath' => $changeset->getFileName(), - 'awayPaths' => $changeset->getAwayPaths(), - 'oldProperties' => $changeset->getOldProperties(), - 'newProperties' => $changeset->getNewProperties(), - 'type' => $changeset->getChangeType(), - 'fileType' => $changeset->getFileType(), - 'commitHash' => null, - 'hunks' => $hunks, - ); - $dict['changes'][] = $change; - } - - $properties = id(new DifferentialDiffProperty())->loadAllWhere( - 'diffID = %d', - $diff->getID()); - foreach ($properties as $property) { - $dict['properties'][$property->getName()] = $property->getData(); - } - - return $dict; + return $diff->getDiffDict(); } } diff --git a/src/applications/conduit/method/differential/getdiff/__init__.php b/src/applications/conduit/method/differential/getdiff/__init__.php index 5ad4547ee8..15399190d3 100644 --- a/src/applications/conduit/method/differential/getdiff/__init__.php +++ b/src/applications/conduit/method/differential/getdiff/__init__.php @@ -9,7 +9,6 @@ phutil_require_module('phabricator', 'applications/conduit/method/base'); phutil_require_module('phabricator', 'applications/conduit/protocol/exception'); phutil_require_module('phabricator', 'applications/differential/storage/diff'); -phutil_require_module('phabricator', 'applications/differential/storage/diffproperty'); phutil_require_module('phabricator', 'applications/differential/storage/revision'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/conduit/method/differential/getrevision/ConduitAPI_differential_getrevision_Method.php b/src/applications/conduit/method/differential/getrevision/ConduitAPI_differential_getrevision_Method.php index 30ccd480ae..04ef4014f8 100644 --- a/src/applications/conduit/method/differential/getrevision/ConduitAPI_differential_getrevision_Method.php +++ b/src/applications/conduit/method/differential/getrevision/ConduitAPI_differential_getrevision_Method.php @@ -62,8 +62,7 @@ class ConduitAPI_differential_getrevision_Method extends ConduitAPIMethod { foreach ($diff->getChangesets() as $changeset) { $changeset->attachHunks($changeset->loadHunks()); } - $diff_dicts[] = - ConduitAPI_differential_getdiff_Method::createDiffDict($diff); + $diff_dicts[] = $diff->getDiffDict(); } $commit_dicts = array(); diff --git a/src/applications/conduit/method/differential/getrevision/__init__.php b/src/applications/conduit/method/differential/getrevision/__init__.php index 66949aaa71..7865804c42 100644 --- a/src/applications/conduit/method/differential/getrevision/__init__.php +++ b/src/applications/conduit/method/differential/getrevision/__init__.php @@ -7,7 +7,6 @@ phutil_require_module('phabricator', 'applications/conduit/method/base'); -phutil_require_module('phabricator', 'applications/conduit/method/differential/getdiff'); phutil_require_module('phabricator', 'applications/conduit/protocol/exception'); phutil_require_module('phabricator', 'applications/differential/constants/revisionstatus'); phutil_require_module('phabricator', 'applications/differential/field/selector/base'); diff --git a/src/applications/differential/mail/base/DifferentialMail.php b/src/applications/differential/mail/base/DifferentialMail.php index 3b0b67c917..ea898bed4c 100644 --- a/src/applications/differential/mail/base/DifferentialMail.php +++ b/src/applications/differential/mail/base/DifferentialMail.php @@ -70,9 +70,10 @@ abstract class DifferentialMail { throw new Exception('No "To:" users provided!'); } - $cc_phids = $this->getCCPHIDs(); - $subject = $this->buildSubject(); - $body = $this->buildBody(); + $cc_phids = $this->getCCPHIDs(); + $subject = $this->buildSubject(); + $body = $this->buildBody(); + $attachments = $this->buildAttachments(); $template = new PhabricatorMetaMTAMail(); $actor_handle = $this->getActorHandle(); @@ -89,6 +90,14 @@ abstract class DifferentialMail { ->setParentMessageID($this->parentMessageID) ->addHeader('Thread-Topic', $this->getRevision()->getTitle()); + foreach ($attachments as $attachment) { + $template->addAttachment( + $attachment['data'], + $attachment['filename'], + $attachment['mimetype'] + ); + } + $template->setThreadID( $this->getThreadID(), $this->isFirstMailAboutRevision()); @@ -165,6 +174,21 @@ EOTEXT; return $body; } + /** + * You can override this method in a subclass and return array of attachments + * to be sent with the email. Each attachment is a dictionary with 'data', + * 'filename' and 'mimetype' keys. For example: + * + * array( + * 'data' => 'some text', + * 'filename' => 'example.txt', + * 'mimetype' => 'text/plain' + * ); + */ + protected function buildAttachments() { + return array(); + } + public function getReplyHandler() { if ($this->replyHandler) { return $this->replyHandler; diff --git a/src/applications/differential/mail/reviewrequest/DifferentialReviewRequestMail.php b/src/applications/differential/mail/reviewrequest/DifferentialReviewRequestMail.php index 95ad604a5b..f08d6f3a85 100644 --- a/src/applications/differential/mail/reviewrequest/DifferentialReviewRequestMail.php +++ b/src/applications/differential/mail/reviewrequest/DifferentialReviewRequestMail.php @@ -77,4 +77,39 @@ abstract class DifferentialReviewRequestMail extends DifferentialMail { return implode("\n", $body); } + + protected function buildAttachments() { + $attachments = array(); + if (PhabricatorEnv::getEnvConfig('metamta.differential.attach-patches')) { + $revision = $this->getRevision(); + $revision_id = $revision->getID(); + + $diffs = $revision->loadDiffs(); + $diff_number = count($diffs); + $diff = array_pop($diffs); + + $filename = "D{$revision_id}.{$diff_number}.diff"; + + $diff->attachChangesets($diff->loadChangesets()); + // TODO: We could batch this to improve performance. + foreach ($diff->getChangesets() as $changeset) { + $changeset->attachHunks($changeset->loadHunks()); + } + $diff_dict = $diff->getDiffDict(); + + $changes = array(); + foreach ($diff_dict['changes'] as $changedict) { + $changes[] = ArcanistDiffChange::newFromDictionary($changedict); + } + $bundle = ArcanistBundle::newFromChanges($changes); + $unified_diff = $bundle->toUnifiedDiff(); + + $attachments[] = array( + 'data' => $unified_diff, + 'filename' => $filename, + 'mimetype' => 'text/x-diff; charset=utf-8' + ); + } + return $attachments; + } } diff --git a/src/applications/differential/mail/reviewrequest/__init__.php b/src/applications/differential/mail/reviewrequest/__init__.php index 2189b1648a..e47543ab06 100644 --- a/src/applications/differential/mail/reviewrequest/__init__.php +++ b/src/applications/differential/mail/reviewrequest/__init__.php @@ -6,8 +6,12 @@ +phutil_require_module('arcanist', 'parser/bundle'); +phutil_require_module('arcanist', 'parser/diff/change'); + phutil_require_module('phabricator', 'applications/differential/mail/base'); phutil_require_module('phabricator', 'applications/phid/handle/data'); +phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/differential/storage/diff/DifferentialDiff.php b/src/applications/differential/storage/diff/DifferentialDiff.php index 18c0e2e43c..310ca810eb 100644 --- a/src/applications/differential/storage/diff/DifferentialDiff.php +++ b/src/applications/differential/storage/diff/DifferentialDiff.php @@ -146,4 +146,56 @@ class DifferentialDiff extends DifferentialDAO { return $diff; } + public function getDiffDict() { + $dict = array( + 'id' => $this->getID(), + 'parent' => $this->getParentRevisionID(), + 'revisionID' => $this->getRevisionID(), + 'sourceControlBaseRevision' => $this->getSourceControlBaseRevision(), + 'sourceControlPath' => $this->getSourceControlPath(), + 'unitStatus' => $this->getUnitStatus(), + 'lintStatus' => $this->getLintStatus(), + 'changes' => array(), + 'properties' => array(), + ); + + foreach ($this->getChangesets() as $changeset) { + $hunks = array(); + foreach ($changeset->getHunks() as $hunk) { + $hunks[] = array( + 'oldOffset' => $hunk->getOldOffset(), + 'newOffset' => $hunk->getNewOffset(), + 'oldLength' => $hunk->getOldLen(), + 'newLength' => $hunk->getNewLen(), + 'addLines' => null, + 'delLines' => null, + 'isMissingOldNewline' => null, + 'isMissingNewNewline' => null, + 'corpus' => $hunk->getChanges(), + ); + } + $change = array( + 'metadata' => $changeset->getMetadata(), + 'oldPath' => $changeset->getOldFile(), + 'currentPath' => $changeset->getFileName(), + 'awayPaths' => $changeset->getAwayPaths(), + 'oldProperties' => $changeset->getOldProperties(), + 'newProperties' => $changeset->getNewProperties(), + 'type' => $changeset->getChangeType(), + 'fileType' => $changeset->getFileType(), + 'commitHash' => null, + 'hunks' => $hunks, + ); + $dict['changes'][] = $change; + } + + $properties = id(new DifferentialDiffProperty())->loadAllWhere( + 'diffID = %d', + $this->getID()); + foreach ($properties as $property) { + $dict['properties'][$property->getName()] = $property->getData(); + } + + return $dict; + } } diff --git a/src/applications/differential/storage/diff/__init__.php b/src/applications/differential/storage/diff/__init__.php index 7192642539..cdd9d21fe0 100644 --- a/src/applications/differential/storage/diff/__init__.php +++ b/src/applications/differential/storage/diff/__init__.php @@ -8,6 +8,7 @@ phutil_require_module('phabricator', 'applications/differential/storage/base'); phutil_require_module('phabricator', 'applications/differential/storage/changeset'); +phutil_require_module('phabricator', 'applications/differential/storage/diffproperty'); phutil_require_module('phabricator', 'applications/differential/storage/hunk'); phutil_require_module('phabricator', 'applications/repository/storage/arcanistproject');