These should have a 'template' property now I think, pointing at the PHID type.

This is really "default edit policy", right?

(You probably can't ever be rejected for this.)

handleRequest / getViewer() (see below)

Never used.

This is inconsistent with other applications, and should always redirect to the badge detail page (with ID determined by $badge->getID()).

Never used.

These should just be set on the controls instead of on the object.

handlerequest / getViewer (see below)

handleRequest / getViewer (see below)

I think this controller does not enforce policies correctly.

Although it changes the form based on the user's edit permissions, it does not enforce that a user must actually have permission in order to perform edits. An attacker can just edit the HTML of the form on the client, then submit whatever parameters they want, removing other users' badges.

The actual edits need to have the checks applied to them.

(Or maybe I'm misunderstanding, I haven't actually patched this locally.)

Prefer $handles = $viewer->loadHandles($phids) in modern code.

Prefer $this->getApplicationURI('x') over $this->getApplicationURI().'x'. The former is more flexible, since we can, e.g., correct or verify URIs inside getApplicationURI(...).

handleRequest (see bleow)

handleRequest() / getViewer() (see below)

Unused.

In modern code, this can be simplified as $this->newDialog(), which automatically calls setUser() for you.

In modern code, just return $dialog;.

Prefer to implement handleRequest(AphrontRequest $request) in modern code, instead.

Then get id with $request->getURIData('id').

Prefer $this->getViewer() to get the viewer.

Using the PHID or "Badge 123" is better here, so the thread doesn't break if the badge is renamed. This header isn't visible to users, it's only used by (some) mail clients to do mail threading.

I don't think we need this, and can remove it.

You can omit this if it's the same as setName().

Since this PHIDType does not implement canLoadNamedObject() (and should not -- I think badges aren't core/important enough to get monograms), this method will never be called. You can just remove it.

This should be simplified to:

return $this->loadStandardPage($this->newResultObject());

Then implement newResultObject() (as return new PhabricatorBadge();).

Unused.

This load+attach stuff should happen in didFilterPage(), so we don't need to load data for objects we're going to throw away a second later when we do policy filtering.

This should also only happen if needRecipients() is set. Otherwise, we'll load all these edges every time, which might be a huge amount of data if there are some very common badges with thousands of recipients.

This is fine as written, but could possibly be simplified a bit with getDestinationPHIDs() on the EdgeQuery.

This can be simplified slightly by implementing buildWhereClauseParts() instead, in modern code.

This should more properly be called PhabricatorBadgesBadge.

This key is probably unnecessary.

Does this conflict with Fund now?

Prefer // comments inside methods.

Worth noting then that this is copy/pastey from Project Membership. Not sure what you want to to change here (iNoob).

https://secure.phabricator.com/diffusion/P/browse/master/src/applications/project/controller/PhabricatorProjectMembersEditController.php;ef556090f86d57b3d334f486c412dedd6314d2d8$23-25

Can't see to get $this->newDialog() to work, crashes and I get:

EXCEPTION: (PhabricatorFileStorageConfigurationException) Malformed local disk storage root. You must provide an absolute path, and can not use '/' as the root. at [<phabricator>/src/applications/files/engine/PhabricatorLocalDiskFileStorageEngine.php:104]

All other dialogs see fine. I'll look more into next upate.

I couldn't get that to work (returns different results).

I fixed this error, but the Dialog still won't submit for me with $this->newDialog(). Nothing now in the logs.

I made these up?

Keep allowPublic, nuke + getURIData

Nuke / use $request->getURIData().

Per elsewhere, you can remove the $id stuff now.

Oh, haha. Let me shoot you a diff for T6859, which will show the right values.

Since you're using modern approaches (notably, you've implemented buildCustomSearchFields()), you should be able to just remove this method without anything breaking/changing.

We might want to make these numeric so you can ORDER BY them, although that's more involved since the qualities would no longer be meaningful on their own.