This is the last major application which needs to switch to ApplicationTransaction infrastructure.
Description
Description
Revisions and Commits
Revisions and Commits
Event Timeline
Comment Actions
I'm going to land D10016 through D10022 (which don't change the UI, unless I broke something) but hold D10023 (which does, by splitting inlines into separate transactions) until the followups which get the UI back in shape are ready. If I messed anything up that should let us identify it before the big migration lands, too.
Comment Actions
The major migrations are in HEAD now, yell if you notice anything off. There are a couple of UI strings that are a bit funky, but that will get cleaned up in future diffs. The expectation is that no data should be missing, and we're in good shape as long as that's the case.
(I checked a handful of audits on this install and they all look correct.)
Comment Actions
Essentially all that remains here is fixing PhabricatorRepositoryCommitHeraldWorker. The problems are:
- This adds auditors and CCs manually. It should add them with transactions.
- This builds and sends an email manually. It should send it via PhabricatorAuditEditor, like all of the rest of the emails in the thread.
- It invokes Herald directly, but should invoke it via transactions.
- Generally, there's a lot of code duplication with stuff now handled natively by transactions.
Overall flow should look more like:
- At top level, apply a transaction like "<author> pushed this commit."
- This should invoke a Herald engine, like Maniphest and Differential do (methods shouldApplyHeraldRules(), buildHeraldAdapter(), etc.)
- On the editor, didApplyHeraldRules() should handle transaction construction (see Maniphest/Differential).
- Returning the transactions will make the original editor apply them properly.
- There's some special stuff going on with the email too, which should also be on the Editor.
- You might have to fiddle with getIsNewObject() to get some of this to behave correctly, since the Commit will already have a PHID by the time we act on it.
Comment Actions
You could also try doing Pholio first if you want to run through a less complicated Herald-In-Transactions implementation (T4484) before this one. That one should be by the book, I think.
Comment Actions
One possible issue is that HeraldCommitAdapter may take a long time to build some of the fields, and the parent TransactionEditor may be holding a (database) transaction open during this time.
I think we can ignore this for the first pass since I'm not sure it will cause problems. If it does, the best fix I can see would be:
- Create the adapter ahead of time.
- Preload all the data it needs based on the rules it is going to run. The slow thing is the raw diff.
- Pass it to the Editor, which starts the (database) transactions.
- The editor returns the pre-built adapter (which can execute quickly) instead of creating a fresh one.
There may be a similar issue with loading the raw patch text to generate an inline diff, where preloading it before applying transactions may be prudent. This one shouldn't hold database transactions open, but could leave a big gap between a commit processing in the UI and the email getting sent.