Page MenuHomePhabricator

Modularize PhabricatorEditEngineConfigurationTransaction
ClosedPublic

Authored by amckinley on Jun 19 2019, 10:19 PM.
Tags
None
Referenced Files
F12847425: D20595.id49153.diff
Fri, Mar 29, 2:11 AM
F12840881: D20595.id49143.diff
Thu, Mar 28, 8:49 PM
F12838159: D20595.id49128.diff
Thu, Mar 28, 5:55 PM
Unknown Object (File)
Sat, Mar 23, 12:46 PM
Unknown Object (File)
Sat, Mar 23, 12:46 PM
Unknown Object (File)
Sat, Mar 23, 12:46 PM
Unknown Object (File)
Sat, Mar 23, 12:46 PM
Unknown Object (File)
Sat, Mar 23, 12:46 PM
Subscribers

Details

Summary

Ref T13319. Ref PHI1302. Migrate PhabricatorEditEngineConfigurationTransaction to modular transactions and add some additional transaction rendering to make these edits less opaque.

Test Plan

Hit all the form edit controllers, viewed resulting transaction timeline.

Event Timeline

To avoid the extreme case of ComComComJava-itis we could maybe just drop the word Configuration from these classes? No other type of EditEngine ... Transaction is ever likely to exist or make sense.

Couple of minor things inline but everything looks good to me.

This should also rebase above or below D20594 or something, but we can sort that out when title rendering comes in.

src/applications/transactions/xaction/PhabricatorEditEngineConfigurationNameTransaction.php
22

(Per below, swap to renderValue() / renderOldValue() / etc.)

40

This should be a "Required" error, not an "Invalid" error, I think.

src/applications/transactions/xaction/PhabricatorEditEngineConfigurationSubtypeTransaction.php
23โ€“24

Here or in some future change, these should convert from "%s" + $old to %s + renderOldValue(), which has the nicer italic rendering in the web UI that reduce ambiguity when the value is a bunch of quotation marks.

To avoid the extreme case of ComComComJava-itis we could maybe just drop the word Configuration from these classes?

That's how I had it originally ๐Ÿ˜ฟ I need to write a Sublime plugin for renaming classes.

  • Renamed classes to remove Configuration
  • Created PhabricatorApplicationTransactionJSONDiffDetailView to reduce JSON-related boilerplate
  • Found one callsite to switch to PhabricatorApplicationTransactionJSONDiffDetailView
  • Implemented getTitle for the various JSON-encoded transactions
  • Requested fixes

Rename some variables for clarity.

One inline thing, but the rest of this looks correct as far as I can tell.

src/applications/transactions/xaction/PhabricatorEditEngineDefaultTransaction.php
19โ€“23 โ†—(On Diff #49135)

D20594 should rebase in here somehow still, I think -- the part where we look up the field label, like "Visible To" instead of "policy.view".

This revision is now accepted and ready to land.Jun 20 2019, 9:07 PM