Details
Hit all the form edit controllers, viewed resulting transaction timeline.
Diff Detail
- Repository
- rP Phabricator
- Branch
- modularize-editor-xaction (branched from master)
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 23015 Build 31599: Run Core Tests Build 31598: arc lint + arc unit
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 ↗ | (On Diff #49128) | (Per below, swap to renderValue() / renderOldValue() / etc.) |
| 40 ↗ | (On Diff #49128) | This should be a "Required" error, not an "Invalid" error, I think. |
| src/applications/transactions/xaction/PhabricatorEditEngineConfigurationSubtypeTransaction.php | ||
| 23–24 ↗ | (On Diff #49128) | 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
One inline thing, but the rest of this looks correct as far as I can tell.
| src/applications/transactions/xaction/PhabricatorEditEngineDefaultTransaction.php | ||
|---|---|---|
| 20–24 | 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". | |