Page MenuHomePhabricator

Clean up some EditEngine meta-policies
ClosedPublic

Authored by epriestley on Dec 7 2015, 11:13 PM.
Tags
None
Referenced Files
F12846825: D14700.id35556.diff
Fri, Mar 29, 1:34 AM
Unknown Object (File)
Sun, Mar 10, 11:15 AM
Unknown Object (File)
Tue, Mar 5, 7:10 PM
Unknown Object (File)
Tue, Mar 5, 7:10 PM
Unknown Object (File)
Tue, Mar 5, 7:06 PM
Unknown Object (File)
Feb 15 2024, 5:04 AM
Unknown Object (File)
Feb 3 2024, 7:40 AM
Unknown Object (File)
Jan 28 2024, 3:53 AM
Subscribers
None

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
Branch
ex18
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 9371
Build 11141: Run Core Tests
Build 11140: arc lint + arc unit

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.