Details
- Reviewers
joshuaspence btrahan chad - Maniphest Tasks
- T5245: Migrate Maniphest Projects to use edge infrastructure
- Commits
- Restricted Diffusion Commit
rPd4b2bfa2f4c1: Modernize commit/edge transaction when parsing commit messages
Pushed a closes Txxx commit and got a close + transaction.
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Looks good. A couple of minor points that mostly belong to other diffs.
| src/applications/diffusion/controller/DiffusionCommitController.php | ||
|---|---|---|
| 423 | I feel that this would be cleaner as new DiffusionEdgeTypeCommitHasTask(), perhaps | |
| src/applications/diffusion/edge/DiffusionEdgeTypeCommitHasTask.php | ||
| 3 ↗ | (On Diff #23619) | As in D9839, I'm not sure that I agree with the class name. I would prefer DiffusionCommitHasTaskEdgeType. |
| 5–9 ↗ | (On Diff #23619) | As in D9839, this feels sort of strange... do we need a function and a constant? |
| 83–88 ↗ | (On Diff #23619) | As in D9837, the argument order is wrong. |
| src/applications/maniphest/edge/ManiphestEdgeTypeTaskHasCommit.php | ||
| 3 ↗ | (On Diff #23619) | As above. |
| 5–9 ↗ | (On Diff #23619) | As above. |
| 83–88 ↗ | (On Diff #23619) | As above. |
I don't love passing objects instead of scalar constants. It's cleaner in some sense, but discards the explicitness of "this is definitely a scalar value which you could safely store in a database or put in a URL".
We also might end up with a weird "take a list of constants, look up all the objects, pass them through, then the other side of the method extracts the constants again" dance somewhere. Not a big deal, and I'm not sure we ever do this, but it feels silly.
It would become challenging to query edges which no longer exist (e.g., for a migration).
The majority of Query classes also accept scalars to the majority of withXYZ() methods, although this isn't a hard-and-fast rule.
Scalars are easier to put in a map on the way in, and some methods of EdgeQuery return maps by type, which would become cumbersome to manage if we primarily switched to using objects.
All pretty weak arguments, but I don't think there's a really compelling argument for using objects instead of constants.
Maybe reexamine this after moving off EdgeConfig? The API would need to work in both modes until then anyway, and doing it this way for now doesn't create much more work if we decide to switch later.
- Rename to BlahBlahEdgeType.
- Remove getEdgeConstant() methods, now implemented with reflection.