Details
Dragged cards to a column with a trigger, saw them close.
Diff Detail
- Repository
- rP Phabricator
- Branch
- trigger4
- Lint
Lint Passed Severity Location Code Message Advice src/applications/project/storage/PhabricatorProjectTrigger.php:68 XHP16 TODO Comment - Unit
Tests Passed - Build Status
Buildable 22290 Build 30485: Run Core Tests Build 30484: arc lint + arc unit
Event Timeline
| src/applications/project/exception/PhabricatorProjectTriggerCorruptionException.php | ||
|---|---|---|
| 3–4 | Why does this get its own class? Are we going to be catching it specifically somewhere? | |
| src/applications/project/storage/PhabricatorProjectTrigger.php | ||
| 69 | Ok this was unclear on my first pass through this, but in reality, $rule_specifications is going to be a JSON blob, right? Which this code is turning back into objects? | |
| 194–198 | Shouldn't we have a way to expand this info? | |
| src/applications/project/trigger/PhabricatorProjectTriggerInvalidRule.php | ||
| 10 | Maybe add a "valid values for rules of this type are..." blurb? | |
| src/applications/project/trigger/PhabricatorProjectTriggerRule.php | ||
| 36–38 | Shouldn't $this->getRecord()->getType() also be exposed? Or is that only used for construction of the rules and we won't need it again? | |
| src/applications/project/trigger/PhabricatorProjectTriggerRuleRecord.php | ||
| 8 | Unused field. | |
| src/applications/project/trigger/PhabricatorProjectTriggerStatusRule.php | ||
| 3 | Shouldn't this be PhabricatorProjectTriggerManiphestStatusRule? I know that Maniphest is the only application that has workboards and could therefore have columns and triggers, but wasn't it somewhere in the planning docs that we want to keep this appropriately generalizable? | |
| src/applications/project/exception/PhabricatorProjectTriggerCorruptionException.php | ||
|---|---|---|
| 3–4 | Yeah, I'm planning to eventually catch it on the trigger edit page and say "this trigger is garbo, you can edit if you want but you're going to overwrite the old stuff if you do." | |
| src/applications/project/storage/PhabricatorProjectTrigger.php | ||
| 69 | Right, this is simulating a value coming out of the DB. In the future, this will just be $rule_specifications = $this->getRuleset(). I just haven't written a way to write to that property yet, since it involves a bunch of JS. | |
| 194–198 | You can click "View Rule" to get everything (well, in a future diff). This is just a little tooltip that appears if you hover over the trigger icon on the workboard. | |
| src/applications/project/trigger/PhabricatorProjectTriggerInvalidRule.php | ||
| 10 | This is currently only used in the tooltip, so I didn't want to get a paragraph of text in. When the edit part happens, that should get more details. (This is going to have a Herald-style visual editor eventually, so it should be hard for users to specify invalid values -- really just extension/upgrade/manually-edit-the-db stuff.) | |
| src/applications/project/trigger/PhabricatorProjectTriggerRule.php | ||
| 36–38 | Maybe, we just don't need it for anything right now. (Today, getValue() is only really for building transactions, and rules generally don't need to know their type to build transactions since the type is implicit in whichever class the method lives in.) Normally, $this->getTriggerType() and $this->getRecord()->getType() will be the same. The only case where they differ is "Unknown" rules, where the getTriggerType() is "unknown" but the record type is the underlying storage record type ("some-extension-type" or whatever). | |
| src/applications/project/trigger/PhabricatorProjectTriggerStatusRule.php | ||
| 3 | Yeah, this is probably a good idea. I'm sort of unsure we'll ever actually generalize, but no reason to make it harder than needbe. | |