Page MenuHomePhabricator

Clean up some EditEngine meta-policies

Authored by epriestley on Dec 7 2015, 11:13 PM.



Ref T9908. Simplify some of the policies here:

  • If you can edit an application (currently, always "Administrators"), you can view and edit all of its forms.
  • You must be able to edit an application to create new forms.
  • Improve some error messages.
  • Get about halfway through letting users reorder forms in the "Create" menu if they want to sort by something weird since it'll need schema changes and I can do them all in one go here.
Test Plan
  • Tried to create and edit forms as an unprivileged user.
  • Created and edited forms as an administrator.

Diff Detail

rP Phabricator
Lint Not Applicable
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Clean up some EditEngine meta-policies.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: chad.
chad edited edge metadata.
chad added inline comments.

0563cd1937bea536929a85e33fa6bfbe.jpg (750×483 px, 18 KB)

This revision is now accepted and ready to land.Dec 7 2015, 11:16 PM

That junk is just implementing a comparison function like this:

function compare($config_x, $config_y) {
  // Put configs that have been given an order first, then new
  // configs at the bottom.
  $x_new = ($config_x->getOrder() == 0);
  $y_new = ($config_y->getOrder() == 0);
  if ($x_new && !$y_new) {
    return "sort x after y in the list";
  } else if (!$x_new && $y_new) {
    return "sort y after x in the list";

  // (Omitting a bit where we compare the actual orders here...)

  // They both have the same newness, so try comparing their names;
  $x_name = $config_x->getName();
  $y_name = $config_y->getName();
  if ($x_name > $y_name) {
    return "sort x after y in the list";
  } else if ($x_name < $y_name) {
    return "sort y after x in the list";

  // Okay, they have the same newness and the same names, so
  // use their IDs to make a decision, since those are definitely
  // unique.
  $x_id = $x->getID();
  $y_id = $y->getID();
  if ($x_id > $y_id) {
    return "sort x after y in the list";
  } else {
    return "sort y after x in the list";

However, when you implement a comparison function like this in PHP and call it with one of the usort()-family of functions, it's incredibly slow for large lists.

It's much faster to just build a big string which represents that operation and use the sort() family of functions. So this builds a string like this:

A000000000001Create New Paste\0000000000123
B000000000000New Feature Request\0000000000124

The strings are build in such a way that the default sorting functions sort them the way we want. It's pretty ugly/weird but it works fairly well I guess?

Maybe at some point I can build a little abstraction that makes it easier to read, like:

return build_sort_key(
  array('bool', ($config->getOrder == 0)),
  array('int', $config->getOrder()),
  array('string', $config->getName()),
  array('int', $config->getID()),

...meaning "sort by order == 0, then order, then name, then ID" without needing any of the sprintf() stuff, which is admittedly gross and easy to get wrong. There was a related case the other day with D14680.

Or maybe:

id(new PhutilSortKey())
This revision was automatically updated to reflect the committed changes.