Page MenuHomePhabricator

Fix PolicyControl defaulting nonsense in every application
Closed, ResolvedPublic

Description

We currently do this thing in a bunch of places:

$some_value = $request->getStr('viewPolicy');
try {
  $editor->apply(...);
} catch (ValidationException $ex) {
  $object->setViewPolicy($some_value);
}

id(new AphrontFormPolicyControl())
  ->setObject($object);

This is unintuitive and silly. It should look like this instead:

$some_value = $request->getStr('viewPolicy');
...
id(new AphrontFormPolicyControl())
  ->setObject($object)
  ->setValue($some_value);

That is, behave like other types of controls do. The logic should be:

  • If an explicit value is set, use that.
  • Otherwise, use the value on the object.

I think this actually already works, based on reading AphrontFormPolicyControl. We should convert Calendar to work like this, then convert all other applications which use the bad version to work like this.

Revisions and Commits

Restricted Differential Revision

Event Timeline

epriestley assigned this task to lpriestley.
epriestley raised the priority of this task from to Normal.
epriestley updated the task description. (Show Details)
epriestley added a project: Calendar.
epriestley added a subscriber: epriestley.
lpriestley added a revision: Restricted Differential Revision.Jun 24 2015, 9:52 PM
lpriestley closed this task as Resolved by committing Restricted Diffusion Commit.Jun 26 2015, 5:41 PM
lpriestley added a commit: Restricted Diffusion Commit.