Page MenuHomePhabricator

Clean up some EditEngine meta-policies
ClosedPublic

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

Details

Summary

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

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
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.
src/applications/transactions/storage/PhabricatorEditEngineConfiguration.php
65

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())
  ->addBool(...)
  ->addInt(...)
  ->...
This revision was automatically updated to reflect the committed changes.