Page MenuHomePhabricator

Move all ApplicationTransaction publishing to daemons
ClosedPublic

Authored by epriestley on Jun 2 2015, 2:11 PM.
Tags
None
Referenced Files
F14061803: D13115.diff
Mon, Nov 18, 8:39 AM
F14046219: D13115.diff
Wed, Nov 13, 7:21 PM
F14036646: D13115.diff
Sun, Nov 10, 11:20 AM
F14030193: D13115.id.diff
Sat, Nov 9, 12:26 AM
F14029225: D13115.id.diff
Fri, Nov 8, 8:04 PM
F14021899: D13115.id31770.diff
Wed, Nov 6, 1:32 PM
F14021567: D13115.diff
Wed, Nov 6, 9:57 AM
F14021458: D13115.id31770.diff
Wed, Nov 6, 9:03 AM
Subscribers

Details

Summary

Ref T6367. Do all mail, feed, notification and search stuff from the daemons, in all editors.

There are four relatively-stateful editors (Audit, Differential, Phriction, PhortuneCart) which needed special care to move state into the daemons properly.

Beyond that, I moved mailTo/mailCC/feedRelated/feedNotify to be computed before we enter the worker:

  • This is simpler, since a lot of editors rely on being able to call $object->getReviewers() or similar to compute them.
  • This is more correct, since we want to freeze the lists at this moment in time.

Finally, I renamed loadEdges to willPublish and made it a slightly more general hook.


This is a bit fragile and I'm not thrilled about it.

It would probably be cleaner to have separate Editor and Publisher classes (something like @fabe's D11329 did). However, I think that's quite a lot of work, and I'd like to see stronger motivation for it (either in this actually being more fragile than I think, or there being other things we get out of it). Overall, I'm comfortable with this change, just definitely not a big fan of the "save" + "load" pattern since I think it's really fragile, nonobvious, hard to debug/predict, etc.

Test Plan

Directly updated editors:

  • Created a new Phriction page, saw "Document Content".
  • Edited a Phriction page, saw "Document Diff".
  • Edited a revision, got normal looking mail.
  • Faked in changedPriorToCommitURI and verified it survived the state boundary.
  • Sent Audit mail.
  • Sent invoice mail.

Indirect editors - for these, I just made a change and made sure the mail generated:

  • Updated a paste.
  • Updated an event.
  • Updated a thread.
  • Updated a task.
  • Updated a mock.
  • Updated a question.
  • Updated a project.
  • Updated a file.
  • Updated an initiative.
  • Updated a Legalpad document.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Move all ApplicationTransaction publishing to daemons.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: btrahan.
epriestley added a subscriber: fabe.
btrahan edited edge metadata.

Seems like the right first step to me and hopefully it holds up and the split into a Publisher class won't be necessary.

This revision is now accepted and ready to land.Jun 2 2015, 4:54 PM
This revision was automatically updated to reflect the committed changes.