$form in $dialog = id(new AphrontDialogView())->appendForm($form) is only defined when if ($can_edit) was true beforehand. But that was always true. Thus add a variable definition (and remove some unused variables like $form_box).
Details
Details
- Reviewers
valerio.bozzolan - Group Reviewers
O1: Blessed Committers - Commits
- rP59678094fb77: Refactor PhabricatorBadgesEditRecipientsController to remove dead code
Visit the page /badges/recipients/1/ or also directly /badges/recipients/1/add/ and add some recipients. It still works as before.
Visit the page without enough permissions. It does not allow to edit them, as before.
Diff Detail
Diff Detail
- Repository
- rP Phorge
- Branch
- badgesEditRecipientForm
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 1454 Build 1454: arc lint + arc unit
Event Timeline
Comment Actions
Thanks. It seems that line 17 impose edit capability. Also, appendForm() does not accept a null.
These two things, together, make me consider that the line 59 is 100% always true, so, the $can_edit variable is an unuseful extra check that only wastes CPU cycles.
Thanks. Please consider the suggested bigger cleanup, since it seems a super-safe cleanup. (To double check I've also tried to omit line 15, causing a crash by appendForm(null) so it's really a needed cleanup 👍
| src/applications/badges/controller/PhabricatorBadgesEditRecipientsController.php | ||
|---|---|---|
| 52–55 | We should probably remove this block. See line 17. | |
| 59 | We should probably omit this condition. It's always true. See line 17. | |
Comment Actions
Same results as Valerio after testing more. Also remove unused vars $title and $header_name - they are unused.
Comment Actions
Thaaaaanks, it works
Feel free to re-edit a bit the summary to talk about your new entries
| src/applications/badges/controller/PhabricatorBadgesEditRecipientsController.php | ||
|---|---|---|
| 16 | I'm commenting here to make this PhabricatorPolicyCapability::CAN_EDIT more prominent in the diff 👍 | |