Page MenuHomePhabricator
Create Task
Maniphest T9845

Straighten out addTextSection() / addRemarkupSection() in mail body construction
Open, NormalPublic
Actions

Description

  • addTextSection() activates a secret mode when passed an existing section object. It should not accept a section object.
  • Inline comments are not remarkup or text sections.
  • This API is generally bewildering.

Revisions and Commits

rP Phabricator
D14589rPcf1061243882 (stable) Partially revert D14511 to fix "INLINE COMMENTS" in mail
D14589rP0d01dab5a375 Partially revert D14511 to fix "INLINE COMMENTS" in mail

Related Objects

Event Timeline

epriestley created this task.Nov 24 2015, 9:10 PM
epriestley raised the priority of this task from to Normal.
epriestley updated the task description. (Show Details)
epriestley added subscribers: epriestley, nickz.
chad added a subscriber: chad.Nov 24 2015, 9:57 PM

Nothing every goes wrong with lots of options!

epriestley added a comment.Nov 24 2015, 10:11 PM

Yeah, I'd really like to just get rid of this option. It's definitely not something we'd accept upstream today.

joshuaspence added a project: Differential.Nov 25 2015, 2:50 AM
joshuaspence added a subscriber: joshuaspence.
avivey merged a task: T9852: Inline comments not appearing in mail.Nov 25 2015, 7:29 PM
epriestley claimed this task.Nov 28 2015, 8:29 PM

I just saw this happen on this install (no metamta.differential.unified-comment-context) so my guess was probably incorrect.

epriestley added a comment.Nov 28 2015, 8:34 PM

This is likely the next cause up the line:

Daemon 123093 STDE [Sat, 28 Nov 2015 20:00:59 +0000] [2015-11-28 20:00:58] ERROR 2: strlen() expects parameter 1 to be string, object given at [/core/lib/libphutil/src/utils/utils.php:769]
Daemon 123093 STDE [Sat, 28 Nov 2015 20:00:59 +0000] arcanist(head=master, ref.master=4f1141d0c59b), phabricator(head=master, ref.master=eb62a98e0fc7), phutil(head=master, ref.master=f0881b37049c)
Daemon 123093 STDE [Sat, 28 Nov 2015 20:00:59 +0000]   #0 strlen(PhabricatorMetaMTAMailSection) called at [<phutil>/src/utils/utils.php:769]
Daemon 123093 STDE [Sat, 28 Nov 2015 20:00:59 +0000]   #1 phutil_split_lines(PhabricatorMetaMTAMailSection, boolean) called at [<phutil>/src/markup/engine/PhutilRemarkupEngine.php:148]
Daemon 123093 STDE [Sat, 28 Nov 2015 20:00:59 +0000]   #2 PhutilRemarkupEngine::splitTextIntoBlocks(PhabricatorMetaMTAMailSection) called at [<phutil>/src/markup/engine/PhutilRemarkupEngine.php:124]
Daemon 123093 STDE [Sat, 28 Nov 2015 20:00:59 +0000]   #3 PhutilRemarkupEngine::preprocessText(PhabricatorMetaMTAMailSection) called at [<phutil>/src/markup/engine/PhutilRemarkupEngine.php:94]
Daemon 123093 STDE [Sat, 28 Nov 2015 20:00:59 +0000]   #4 PhutilRemarkupEngine::markupText(PhabricatorMetaMTAMailSection) called at [<phabricator>/src/applications/metamta/view/PhabricatorMetaMTAMailBody.php:51]
Daemon 123093 STDE [Sat, 28 Nov 2015 20:00:59 +0000]   #5 PhabricatorMetaMTAMailBody::addRemarkupSection(string, PhabricatorMetaMTAMailSection) called at [<phabricator>/src/applications/differential/editor/DifferentialTransactionEditor.php:1217]
Daemon 123093 STDE [Sat, 28 Nov 2015 20:00:59 +0000]   #6 DifferentialTransactionEditor::buildMailBody(DifferentialRevision, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2376]
Daemon 123093 STDE [Sat, 28 Nov 2015 20:00:59 +0000]   #7 PhabricatorApplicationTransactionEditor::buildMailForTarget(DifferentialRevision, array, PhabricatorMailTarget) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2333]
Daemon 123093 STDE [Sat, 28 Nov 2015 20:00:59 +0000]   #8 PhabricatorApplicationTransactionEditor::buildMail(DifferentialRevision, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:1048]
Daemon 123093 STDE [Sat, 28 Nov 2015 20:00:59 +0000]   #9 PhabricatorApplicationTransactionEditor::publishTransactions(DifferentialRevision, array) called at [<phabricator>/src/applications/transactions/worker/PhabricatorApplicationTransactionPublishWorker.php:21]
Daemon 123093 STDE [Sat, 28 Nov 2015 20:00:59 +0000]   #10 PhabricatorApplicationTransactionPublishWorker::doWork() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:122]
Daemon 123093 STDE [Sat, 28 Nov 2015 20:00:59 +0000]   #11 PhabricatorWorker::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php:171]
Daemon 123093 STDE [Sat, 28 Nov 2015 20:00:59 +0000]   #12 PhabricatorWorkerActiveTask::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php:22]
Daemon 123093 STDE [Sat, 28 Nov 2015 20:00:59 +0000]   #13 PhabricatorTaskmasterDaemon::run() called at [<phutil>/src/daemon/PhutilDaemon.php:183]
Daemon 123093 STDE [Sat, 28 Nov 2015 20:00:59 +0000]   #14 PhutilDaemon::execute() called at [<phutil>/scripts/daemon/exec/exec_daemon.php:125]
epriestley added a comment.Nov 28 2015, 8:37 PM

I think this is just junked up and I hadn't pushed this server when I claimed it works.

epriestley added a comment.Nov 28 2015, 8:44 PM

Context here is:

  • D9375 added some really sketchy behavior to addTextSection() which I missed in review.
  • D13554 added addRemarkupSection().
  • D14511 swapped from addTextSection() to addRemarkupSection().

The object is not a remarkup section, nor is it a text section.

I'm going to fix this narrowly for now and repurpose this task for finding a broader fix.

epriestley added a revision: D14589: Partially revert D14511 to fix "INLINE COMMENTS" in mail.Nov 28 2015, 9:33 PM
epriestley added a commit: rP0d01dab5a375: Partially revert D14511 to fix "INLINE COMMENTS" in mail.Nov 28 2015, 9:41 PM
epriestley added a commit: rPcf1061243882: (stable) Partially revert D14511 to fix "INLINE COMMENTS" in mail.
epriestley renamed this task from `metamta.differential.unified-comment-context` not working at HEAD? to Straighten out addTextSection() / addRemarkupSection() in mail body construction.Nov 28 2015, 9:42 PM
epriestley updated the task description. (Show Details)

D14589 should fix the immediate issue. I've also cherry-picked it into stable.

There's more cleanup be done here but that can wait for another day.

talshiri added a subscriber: talshiri.Dec 2 2015, 4:44 AM

This one is totally my bad.
My intention was to make it less spooky to migrate individual pieces to the section construct (since the API remained largely the same), but the API weirdness was not worth it, and didn't even achieve that goal

epriestley merged a task: T10170: Ponder HTML email does not stylize code blocks, lists.Jan 22 2016, 12:06 PM
epriestley added a subscriber: cspeckmim.

From T10170, Ponder still has some addRawSection() calls which should be addRemarkupSection() calls.

Phabricator · Built by Phacility