Details
- Reviewers
epriestley - Maniphest Tasks
- T6237: Use ApplicationTransactions properly in Diff creation
- Commits
- Restricted Diffusion Commit
rPffe0765b50c5: Differential - make DifferentialDiffEditor into a real transaction editor.
made a diff from web ui and it worked. made a herald rule to block certain diffs then tried to make such a diff and saw UI letting me know i was blocked
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
This looks like a good step forward to me. I think we should introduce DifferentialDiffTransaction (see third inline); the other stuff is fluff/future.
| src/applications/differential/editor/DifferentialDiffEditor.php | ||
|---|---|---|
| 79–93 | This might be too eager on the web workflow in the future. Specifically, if we add an explicit "Repository:" field, and the user declines to fill it out, we probably should not guess for them (even though we guess via the API). I don't think this impacts anything for now. | |
| 202–203 | Does this end up running the rules twice? Are you just future-proofing? I'd expect them to run once in validateTransaction(), and then again here as part of the normal Herald workflow. Running them here only is probably slightly cleaner, but it may be hard to raise a good error if we do that: I don't think we have any other transaction rules which block the transaction. If it's a mess, maybe run them only in validation? | |
| src/applications/differential/storage/DifferentialTransaction.php | ||
| 27–29 ↗ | (On Diff #26103) | We should really be using a new DifferentialDiffTransaction, not reusing DifferentialTransaction (which is really DifferentialRevisionTransaction). This class has stuff (like this constant) which isn't correct for diffs. |
| src/applications/differential/editor/DifferentialDiffEditor.php | ||
|---|---|---|
| 202–203 | The rules do end getting run twice. This function is used to build the herald adapter, which gets used in validateTansaction and some lower level ApplicationTransactionEditor codepath way. validateTransaction itself does some tricky business since that happens so much earlier as compared to the lower level ApplicationTransactionEditor stuff. that's what the updateDiffFromDict helper function is about more or less. | |
Introduce and use differentialdifftransaction.
In a future diff, I can get these rules to run once. Per the previous code and comments here, its important to not save a Herald transaction log for these blocking rules - they could save some data we wanted to block - and the normal herald code path runs *after* save, so this has to live in validateTransaction. validateTransaction will thus stop doing the clone thing and use the actual $object, and I'll rip apart the lower level codepath to optionally skip some work if a herald engine is around. Not sure if that makes sense as written but you can pick it apart in the diff.
| resources/sql/autopatches/20141118.diffxaction.sql | ||
|---|---|---|
| 2 | There's a marginally more modern version of this table available here: This version is basically fine, but storage adjust will need to fix a bunch of stuff afterward. Using the more modern version will create the table directly with the correct column types, etc. | |
| src/applications/differential/editor/DifferentialDiffEditor.php | ||
| 195 | Maybe we just make this return false; to stop the second run, and keep everything else the same as it is now? The unusual nature of how this rule works makes special casing it seem pretty reasonable to me, but there's no reason we need to do the second run (at least, today) -- the second run never has any meaningful effect. | |
- use the more modern table definition - THANKS!
- return false and add a comment to see validateTransaction method