Page MenuHomePhabricator

Make it easier to write custom transaction types
Closed, ResolvedPublic

Assigned To
Authored By
joshuaspence
Nov 15 2015, 12:05 AM
Referenced Files
F1715512: pasted_file
Jul 7 2016, 11:33 PM
Tokens
"Mountain of Wealth" token, awarded by hach-que."Love" token, awarded by 20after4."Doubloon" token, awarded by yelirekim."Piece of Eight" token, awarded by epriestley.

Description

It is currently rather difficult to write custom transaction types because transaction types are implemented as class constants rather than subclasses. See the PhabricatorTransactions class, for example.

We have previously had similar issues with edges and PHIDs, both which were solved by moving to subclasses rather than class constants (see D6502 and D9837).

See discussion in D14481. There's probably also some discussion on the modularization of PHID and edge types in T2715 and T5245, respectively.

  • Per T10214, we should put a typecheck on transactions somewhere in the editor pipeline. You can currently apply, say, a PasteTransaction against a Task, and things get much further than they should.

Related Objects

Mentioned In
T10448: Modularize mail tags
rP70505062673a: Fix a bad getURI() call in Profile Panel handle construction
D16249: Fix a bad getURI() call in Profile Panel handle construction
rP05699388804f: expose renderHandle in PhabricatorModularTransactionType
T9360: Unbeta Phame
D16140: Add ability to set a header image per Phame blog
T11035: Only evaluate newly added mentions/subscribers when applying transactions to remarkup blocks
T5214: Add Conduit API methods for workboards
T10537: Nuance Infrastructure
T10214: Daemons try and fail to publish transactions for newly created diffs
D15112: Use correct transaction types when creating diffs
rP3f8e5c96209d: Straighten out reorder permissions on form configurations
D14824: Straighten out reorder permissions on form configurations
T5873: Update Conduit for ApplicationTransactions, CustomFields and Edges
D14646: Render Remarkup poorly in Phame Feed stories
rPacd955c6c9b9: Modularize application extensions to EditEngine
D14599: Modularize application extensions to EditEngine
T9806: Exception while editing Ponder comment to an answer
D14481: Add a `PhutilEnum` class
Mentioned Here
D17402: Modular Transactions for Badges
T10448: Modularize mail tags
T10978: Modernize Audit
T5055: Distribution mechanism for arc extensions
T7643: Improve prose diffs (was: description changes don't generate usable diffs)
T9275: Move Calendar to EditEngine
T10855: Allow external systems to edit the content of dashboard panels so they can publish information
T11035: Only evaluate newly added mentions/subscribers when applying transactions to remarkup blocks
T11114: Move Differential to EditEngine
T10214: Daemons try and fail to publish transactions for newly created diffs
T9132: Build an ApplicationEditor abstraction
T5245: Migrate Maniphest Projects to use edge infrastructure
D9837: Make edge types modular
D14481: Add a `PhutilEnum` class

Event Timeline

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

I think we can do this gradually by:

  • Subclassing PhabricatorApplicationTransactionEditor into ...Pro or ...ModularEditor or maybe just calling the new one PhabricatorTransactionEditor or even PhabricatorEditor given how core the abstraction is to modern applications. If we do a goofy one, we should rename at the end.
  • Subclassing PhabricatorTransaction in the same way.
  • Implement all behaviors in terms of modular transaction subclasses.
  • Migrate an application.
  • Migrate everything else.
  • Throw the old stuff away.

Every method should already have all the parameters it needs and all the required context, I think. This isn't really shifting any responsibilities, just reorganizing code.

I'm continuing to warm to this idea and am considering doing it in the relatively near term, either as part of T9132 or directly afterward. In particular, that will also require touching a bunch of applications in a big, related modernization effort, so it may be worthwhile to double up on testing and do this too.

On the other hand, the modernization implied by T9132 is likely much smaller (and much higher-value in the short term per effort spent) than this one.

epriestley triaged this task as Normal priority.Nov 24 2015, 5:26 PM
epriestley awarded a token.

I'm leaning toward exploring this in the near term. We have four applications which may convert soon:

  • Calendar is prioritized and certainly converting. (T9275)
  • Differential is probably converting? (T11114)
  • Dashboards are probably converting? (T10855)
  • Packages may be coming into existence? (T5055)

Additionally, T11035 is prioritized and would be cleaner alongside structural changes.

I'd feel much better about all of this if transactions were modularized first.

One concern here is that Differential, Dashboards and Calendar are all relatively poor starter applications (all have complex, unique transaction behaviors) and Packages doesn't exist yet, and building purely as a testbed for modular transactions feels questionable. So maybe I'll just convert Paste (the canonical starter application) and then go from there. Some weak adjacency to T7643.

I spent a couple hours on this last night and a couple hours this morning and I think I have Paste pretty much converted, so things look promising. I'm going to hold this until after the release cut, but expect to put Calendar on top of it and whatever else converts in the future.

(Just bookkeeping; there's no external priority on this.)

I think we broke plain-text emails here:

pasted_file (107×663 px, 12 KB)

epriestley claimed this task.

The system described in this task became "ModularTransactions", which feel like they're in a good place.

The change between old Transactions and ModularTransactions is not a large one (essentially, switch ($xaction_type) { ... } just became a subclass hierarchy instead) and we weren't really addressing any direct or immediate problems, but the change let us fix some minor frustrations and the new code feels quite a bit easier to work with to me than the old code did. It is also extensible, has meshed well with EditEngine, and the conversion process has been fairly painless (e.g., see D17402#207942). We're also in a better place to pursue some changes like T10448, now.

A number of applications (including Paste, Badges, Calendar, Differential, and Diffusion/Audit) have converted in later tasks like T11114 and T10978. Maniphest hasn't yet, but I don't expect the conversion process to be a painful one.