Page MenuHomePhabricator

Make application customization of builtin transaction types optional
Closed, ResolvedPublic

Description

Currently, TransactionEditor objects are required to implement applyCustomInternalTransaction() and applyCustomExternalTransaction() for all types they support. This is good for application transactions, but causes issues with builtin transactions (like edges).

T6394 is an example: when dashboard panels were implemented, there was no way to generate an Edge transaction against them, so they correctly omitted internal/external cases for these transaction types. After we added mentions, it became possible and things broke.

It's still good to give editors a callback for these transactions so they can apply effects, and some applications do this. However, we should make it a separate, optional callback, like applyBuiltinInternal/ExternalTransaction. Then send builtin transactions through that one, and custom/application transactions through the existing one which requires implementation.

Event Timeline

epriestley raised the priority of this task from to Wishlist.
epriestley updated the task description. (Show Details)
epriestley added a project: Transactions.
epriestley added a subscriber: epriestley.

(We've had ~3-5 other similar issues in the past.)

btrahan claimed this task.May 19 2015, 5:46 PM

If its "built in" then its one of the PhabricatorTransactions constants and otherwise its not built in?

TYPE_EDGE diff just attached.

I seem to have broken some things with the EDGE stuff at least. Looking at it now...

btrahan added a revision: Unknown Object (Differential Revision).May 19 2015, 9:26 PM
btrahan removed a revision: Unknown Object (Differential Revision).May 19 2015, 10:25 PM