Page MenuHomePhorge

Refactor PhabricatorBadgesEditRecipientsController to remove dead code
ClosedPublic

Authored by aklapper on Jul 25 2024, 13:43.

Details

Summary

$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).

Test Plan

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

Repository
rP Phorge
Branch
badgesEditRecipientForm
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1454
Build 1454: arc lint + arc unit

Event Timeline

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.

This revision is now accepted and ready to land.Jul 26 2024, 07:25

Same results as Valerio after testing more. Also remove unused vars $title and $header_name - they are unused.

valerio.bozzolan retitled this revision from Fix undefined variable in PhabricatorBadgesEditRecipientsController to Refactor PhabricatorBadgesEditRecipientsController to remove dead code.
valerio.bozzolan edited the summary of this revision. (Show Details)
valerio.bozzolan edited the test plan for this revision. (Show Details)

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 👍