Converts Owners package transactions to modular transactions.
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Commits
- rP8247edff98e8: Modularize Owners package transactions
- created a new package
- edited all simple properties from the web ui
- checked that project and user owners were added as reviewers appropriately to new diffs
- inspected the change details for various types of path add / remove / update / reorder changes
Diff Detail
- Repository
- rP Phabricator
- Branch
- owners_modular_xactions (branched from master)
- Lint
Lint Passed Severity Location Code Message Advice src/applications/owners/xaction/PhabricatorOwnersPackageOwnersTransaction.php:43 XHP16 TODO Comment Advice src/applications/owners/xaction/PhabricatorOwnersPackagePathsTransaction.php:125 XHP16 TODO Comment - Unit
Tests Passed - Build Status
Buildable 14055 Build 18233: Run Core Tests Build 18232: arc lint + arc unit
Event Timeline
Couple of minor things but this all looks like it's headed in the right direction.
src/applications/owners/xaction/PhabricatorOwnersPackageDescriptionTransaction.php | ||
---|---|---|
17–19 | With EditEngine, we don't need to do this stuff anymore -- it automatically folds transactions which apply at create time into an "X created this object" aggregate by default. | |
27–29 | Likewise. | |
src/applications/owners/xaction/PhabricatorOwnersPackageNameTransaction.php | ||
47 | No more quotes required around values in the string -- we style them in the web UI and quote them in text email, so if you don't strip quotes it will generate sarcastic-looking double-quoted email. |
I still have stylistic stuff to do throughout, I just focused on faithfully moving functionality out of the editor/transaction into the modular transactions here. There are a few things that don't work, haven't even looked at custom fields yet.
src/applications/owners/storage/PhabricatorOwnersPackageTransaction.php | ||
---|---|---|
13–14 | This is probably not right, but it has nowhere to move. Is it even needed? |
src/applications/owners/editor/PhabricatorOwnersPackageTransactionEditor.php | ||
---|---|---|
22–23 | This has nowhere to go, should transactions types implement "has effect"? |
src/applications/owners/editor/PhabricatorOwnersPackageTransactionEditor.php | ||
---|---|---|
22–23 | This is OK to bring into ModularTransactions, but it might also be OK to just remove it completely. I'd expect no-op updates to already no-op correctly, and if you, say, reorder paths it seems OK to count that as a change? This might be vestigial from long ago. I'd say:
| |
src/applications/owners/storage/PhabricatorOwnersPackageTransaction.php | ||
13–14 | Should no longer be necessary. Getting rid of this was an explicit goal of ModularTransactions (and, generally, a goal in new code). It is replaced by HandlePools (e.g., via $viewer->loadHandles()) which just more-or-less do this correctly without requiring you to do any work. The handle rendering methods in ModularTransaction use HandlePools automatically. (This magic isn't completely transparent in all cases but it's pretty close.) |
- Fix display of quoted transaction values
- Remove unneeded empty checks for create transactions
- Remove getRequiredHandlePHIDs and transactionHasEffect methods
- Use generateOldValue instead of getOldValue when applying external effects
This seems to work afaik. Still need to do custom fields and try this in conjunction.
src/applications/owners/xaction/PhabricatorOwnersPackageOwnersTransaction.php | ||
---|---|---|
21 | I had to use generate here because the transaction doesn't have its storage attached at this point, I think? I got an exception from the web UI if I called getOldValue. | |
src/applications/owners/xaction/PhabricatorOwnersPackagePathsTransaction.php | ||
101 | Same deal as the owners transaction. |
I'll follow up on the get/generate thing after this lands.
src/applications/owners/xaction/PhabricatorOwnersPackageAutoreviewTransaction.php | ||
---|---|---|
13 | Should be fine to just = array();, I wanted to move away from this "you have to call parent:: all the time" stuff as much as possible. There's at least one more of these elsewhere. | |
src/applications/owners/xaction/PhabricatorOwnersPackageOwnersTransaction.php | ||
21 | This is probably a ModularTransactions bug, but I can shoot you a followup. generate... instead of get... shouldn't really cause any issues. |