Ref T5245. Currently, task/project links rely on side effects in save(). Make them more transaction-oriented, with the goal of moving fully to edges a few diffs down the line.
Details
Details
- Reviewers
joshuaspence btrahan chad - Maniphest Tasks
- T5245: Migrate Maniphest Projects to use edge infrastructure
- Commits
- Restricted Diffusion Commit
rP7deec8208ff9: Make Maniphest project edits more transaction-oriented
- Added and removed projects using "Edit Task", "Associate Projects" comment action, and Herald.
- Verified database ended up in the expected state.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- projedge1
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 1556 Build 1556: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
Comment Actions
I'm not very familiar with this code, but from what I can tell this diff is definitely an improvement.
Comment Actions
(This sequence of diffs ended up going deep into the dark corners of the edge stuff, so feel free to bail out if things get too murky -- none of this is urgent, and we can wait for @btrahan to get back from paternity leave.)
Comment Actions
Yeah I noticed. The edge stuff isnt so bad and I think I have a reasonable understanding. I'm not confident with the old code (pre-edges) I'll take a closer look tomorrow.
Comment Actions
At the same time, based on the calendar it appears that @btrahan is due back shortly... so if you don't trust my reviewing capabilities (no offence taken), feel free to wait a little longer. I'm taking a look through the diffs now.
Comment Actions
This may be addressed in another diff, but is the intention to eventually remove/deprecate the ManiphestTaskProject class? It seems that this class does what will be done with edges.
Comment Actions
Just looking over this again as a sanity check... still looks good to me. One minor inline.
| src/applications/maniphest/editor/ManiphestTransactionEditor.php | ||
|---|---|---|
| 419–421 | It might be cleaner to just move this into the above if statement now. | |
Comment Actions
Yeah, the class is removed in D9852 and there will be a diff at the end of this to drop the table.