Page MenuHomePhabricator

Modularize application transactions in Paste, mostly
ClosedPublic

Authored by epriestley on Jun 13 2016, 9:49 PM.

Details

Summary

Ref T9789. Transaction and Editor classes are the last major pieces of infrastructure that haven't been fully modularized.

Some of the specific issues are:

  • Editor classes rely on a bunch of instanceof stuff in the base class to pick up transaction types like "subscribe", "projects", etc. Instead, applications should be adding these, and third-party applications should be able to add them.
  • Code is spread across Transaction and Editor classes somewhat oddly. For example, generating old/new values would probably make more sense at the Transaction level, but it currently exists at the Editor level.
  • Both types of classes have a lot of functions based on switch() statements, which require a ton of boilerplate and are just generally kind of hard to work with.

This creates classes for each type of transaction, and moves almost all of the logic to them. These classes are simpler and more focused than the old stuff was, and can organize related code better.

This starts inching toward defining CoreTransactions for features shared across applications. It only defines the "Create" transaction so far, but at some point I plan to move all the other shared transactions to Core and let them control which objects they're available for.

Test Plan
  • Created pastes with web UI and API.
  • Edited all paste properites.
  • Archived/activated.
  • Verified files got reasonable names.
  • Reviewed timeline and feed.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley updated this revision to Diff 38764.Jun 13 2016, 9:49 PM
epriestley updated this revision to Diff 38765.
epriestley retitled this revision from to Modularize application transactions in Paste, mostly.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: chad.
  • Fix feed titles for object creation.
epriestley added inline comments.Jun 13 2016, 10:26 PM
resources/sql/patches/20130801.pastexactions.php
3–5

I just nuked these since they aren't too interesting and aren't even correct anymore (in modern code, they should be TYPE_CREATE). Didn't seem worthwhile to try to update this since it's ancient.

src/applications/paste/editor/PhabricatorPasteEditor.php
36–38

The old version of this was weird. I blamed it and it looks like it got sort-of-broken at some point. The original intent was this (only send mail for updates, not for creation). We could take another look at this eventually, not sure if this rule makes sense in modern apps.

src/applications/paste/storage/PhabricatorPasteTransaction.php
26–42

I'm just not dealing with mail tags for now, not really sure what I want to do with them.

src/applications/paste/xaction/PhabricatorPasteLanguageTransaction.php
4

The "Content" transaction above has a lot going on, but the other three are how most transactions should look going forward. Feels a lot better to me than switch(...) stuff.

chad accepted this revision.Jun 13 2016, 10:37 PM
chad edited edge metadata.
This revision is now accepted and ready to land.Jun 13 2016, 10:37 PM
This revision was automatically updated to reflect the committed changes.

Paste comment transactions seem to have a borked Feed title here, guessing that's something from this that isn't hooked up yet?

https://secure.phabricator.com/feed/6296015785671195191/