Locking a credential does not need to prevent it from being destroyed -- you should still be able to destroy it, just not read it.

When a credential is locked, we should prevent the actual changes from being applied here.

Right now if I'm editing a locked credential the "secret" input will be disabled, but I can control-click the document, find the textarea, deleted the disabled="disabled" attribute, and then use the form normally -- even if the credential is locked.

Instead, we should only build the $type_is_locked, $type_destroy and $type_secret_id transactions if the credential is not locked. Then even if a user removes the "disabled" attribute they still can not edit the secret.

That is, the code should look something like:

$xactions[] = name;
if (!$credential->getIsLocked()) {
  $xactions[] = username;
  $xactions[] = secret;
  $xactions[] = locked;
}

If the credential is locked, it might be nice to appendRemarkupInstructions() here with a message like "This credential is permanently locked and can not be edited.", to explain why the controls are disabled.

Let's only show this if the credential is being created. The major reason for this convenience checkbox is that some workflows (like creating a new repository in Diffusion) create credentials using this form as a popup dialog, while you're in the middle of some other workflow -- so once you're done, you don't end up on the credential detail page, but back in the middle of creating a repository or whatever. In these cases it would be inconvenient to navigate to the credential page in order to lock the credential.

You can test $is_new to see if the credential is being freshly created vs edited.

This block should move above the $request->isFormPost() test on line 37.

Currently, you can control-click the document and edit things a bit (this is a little involved) and produce a submit button. Then you click that and end up with an isFormPost() request, which will get you into the if ($request->isFormPost()) block and let you take the action.

In this case, taking the action multiple times doesn't matter, but as a general rule it's better to put a check like this earlier, so the main action of the controller is completely unreachable.

As a recently-introduced optional convenience, you can now replace this longer dialog style:

$dialog = id(new AphrontDialogView())
  ->setUser($viewer)
  ->...;
return id(new AphrontDialogResponse())->setDialog($dialog);

...with this slightly shorter, less boilerplatey style:

return $this->newDialog()
  ->...;

...provided $this is a PhabricatorController (when you want to build a dialog, it almost always is).

This is correct, but should move above the $request->isFormPost() call on line 37.