Moves participants over to ModularTransactions, simplified a lot of the code. Fixes T12550
Details
- Reviewers
chad - Maniphest Tasks
- T12550: Move Conpherence to Modular Transactions
- Commits
- rP51485a1f8257: Add participants ModularTransactions to Conpherence
Create a new room with just myself and myself + fake accounts.
Remove a person.
Remove myself.
Edit a room, topic.
Type some messages.
???
Profit
Diff Detail
- Repository
- rP Phabricator
- Branch
- conph-xaction-2 (branched from master)
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 16467 Build 21922: Run Core Tests Build 21921: arc lint + arc unit
Event Timeline
src/applications/conpherence/xaction/ConpherenceThreadParticipantsTransaction.php | ||
---|---|---|
41 | Not sure how / where to grab a current transaction PHID |
To get the current transaction PHID, you can do something like this on ModularTransactionType:
final protected function getCurrentTransactionPHID(() { $xaction_phid = $this->getStorage()->getPHID(); if (!$xaction_phid) { throw new Exception(pht('Current transaction does not yet have a PHID yet!')); } return $xaction_phid; }
It's possible that will throw. If it does, you can do this instead:
final protected function getCurrentTransactionPHID(() { $storage = $this->getStorage(); $xaction_phid = $storage->getPHID(); if (!$xaction_phid) { $xaction_phid = $storage->generatePHID(); $storage->setPHID($xaction_phid); } return $xaction_phid; }
That's a bit more magic but I think it's still OK and should work.
As written, I think this change breaks all the rendering of historic transactions? It also doesn't support the '=' action, which is OK for now, but if it's later exposed via EditEngine the controls there will think that = is supported.
Possibly better is:
- Keep oldValue as a list of old participants.
- Keep newValue as a list of new participants.
- Put a method on ModularTransactionType that calls $this->getEditor()->getPHIDTransactionNewValue() to make the method callable from ModularTransactions and handle converting - / + / = into a list of PHIDs.
- Use it to generate a list of PHIDs for the newValue.
- Keep applyExternalEffects, getTitle() and validateTransactions() based around lists of PHIDs.
This should also handle cases like array('-' => array('A'), '+' => array('A')) (adding and removing the same user in one transaction) correctly, but I think the current code has less-ideal handling of these weird edge cases.
src/applications/conpherence/editor/ConpherenceEditor.php | ||
---|---|---|
249 | Not currently. They're sort of obsoleted by EditEngine and I think I want to get rid of them completely, but probably best to just leave them here for now and we'll figure this out later. | |
src/applications/conpherence/xaction/ConpherenceThreadParticipantsTransaction.php | ||
38 | Since we're touching this code, maybe prefer PhabricatorTime::getNow(). |
EXCEPTION: (Error) Call to a member function generatePHID() on null at [<phabricator>/src/applications/transactions/storage/PhabricatorModularTransactionType.php:315]
Seems like getStorage is null here.
This is getting pretty far into sketchy territory but you could do this, in PhabricatorApplicationTransactionEditor->applyExternalEffects():
$xtype = $this->getModularTransactionType($type); if ($xtype) { + $xtype = clone $xtype; + $xtype->setStorage($xaction); return $xtype->applyExternalEffects($object, $xaction->getNewValue()); }
Or just leave this as PHID-VOID-0000 now and I'll counter-diff you some less questionable version of this after it lands?
Yeah my sneaky plan for a good version of this is like:
- Set it to null here or something like that.
- If it's null when we get to applyFinalEffects(), always set it to the last transaction PHID.
Not sure if that'd work but then we wouldn't have to do anything fancy here.
getPHIDTransactionNewValue takes a transaction, which.... still having problems getting
I have to go pick up the pup in a little bit but let me see if I can steal all your internet points first
I don't really know what applyFinalEffects() is actually doing so I probably need to go look at the code a bit before I can fix that, but I think the bits other than that sorta work.
src/applications/transactions/storage/PhabricatorModularTransactionType.php | ||
---|---|---|
312–317 ↗ | (On Diff #42651) | so clean. |