Ideally, we should maybe freeze maniphest.querystatuses (which has a sort of ridiculous output format, I think?) and provide maniphest.status.search and maniphest.priority.search, which return results similar to other modern *.search methods even if they aren't implemented quite the same way (at least make the output format similar enough that a client library with methods like "turn a x.search call into a list of objects" would work on these methods, too).

"Priority" currently has a string type because it's edited by a PhabricatorSelectEditField in ManiphestEditEngine, and "select" fields are always string-typed.

In theory, you could add some method like setYouCanUseIntegersForThis(true) on PhabricatorSelectEditField, but that feels a little fishy to me.

Maybe:

• Add a required, unique string "key" to maniphest.priorities for each priority.
• Freeze maniphest.querystatuses.
• Provide maniphest.status.search.
• Provide maniphest.priority.search.
• Swap the options the EditField uses from integers to the new strings you introduced.
• In the PriorityTransaction, convert the string (like "high") to an integer so the internal/storage/display format is the same.
• Undo the mapping in frozen method maniphest.update.

Then "50" and 50 will both break against maniphest.edit, but we'll otherwise be in a fully sane world of "high", "low", etc.

We also probably have similar issues in maniphest.search with the "priority" constraint. Probably just break it at the same time? Not sure how bad off we are there.

Transitionally, we could continue to accept "50" for a while and then schedule a break in a few months.

The small bit of good news is that I managed to forsee this well enough that the output from maniphest.search for "priority" is a dictionary, so that would be easy to extend with a "key".

I guess we already have "keywords" and should probably accept any of them, too, so "!priority high" in mail, /priority high in a comment, and "high" in Conduit all work consistently even if the actual key is "elevated".

The config also looks like this right now:

{
  123: { ... }
  124: { ... }
}

That is, it's a dictionary. Every other similar config looks like this:

[
  {thing: 123},
  {thing: 124},
]

That is, it's a list.

I'm not sure if this worth breaking alongside other changes in order to improve consistency or not.

The major reason we do lists for everything else is that the keys in JSON maps are not necessarily ordered, and other languages don't always expose an ordered primitive when you decode an object, but sometimes we rely on order (for example, the list of statuses cares about the order you enter it in). This doesn't matter for priorities since we sort by the priority numbers. However, it's an exception in an otherwise consistent field, at least among all the stuff we've added somewhat recently.

That is, the specific error "use lists" is aimed at preventing is this:

• You specify all your config in YAML (very cool) and deploy it with Chef (very cool).
• One day (perhaps today, perhaps in the future) Chef (very cool) decides that since JSON objects are not ordered, it can read the object from YAML, freely reorder it, and write it in whatever order it prefers, perhaps "for performance".
• After you deploy, your statuses now have some arbitrary order like "Closed, Invalid, Open, Unfixable, Reopened, Impossible, Double Verified, Haunted, Single Verified" instead of the nice logical order they previously had.

So should the return value for priority.search look something like this?

[
  { "value": 80,
    "name": "High",
    "short": "High",
    "color": "red",
    "keywords": [
      "high"
    ]},
  { "value": 50,
    "name": "Normal",
    "short": "Normal",
    "color": "orange",
    "keywords": [
      "normal"
    ]},
    // etc etc
]

Yeah, with whatever wrapping context maniphest.search has, so like:

{
   "data": [ (that list) ]
}

...or whatever.

Actual example output:

{
  "data": [
    {
      "name": "Unbreak Now!",
      "short": "Unbreak!",
      "color": "pink",
      "keywords": [
        "unbreak"
      ],
      "value": 100
    },
    {
      "name": "Needs Triage",
      "short": "Triage",
      "color": "violet",
      "keywords": [
        "triage"
      ],
      "value": 90
    },
    {
      "name": "High",
      "short": "High",
      "color": "red",
      "keywords": [
        "high"
      ],
      "value": 80
    },
    {
      "name": "Normal",
      "short": "Normal",
      "color": "orange",
      "keywords": [
        "normal"
      ],
      "value": 50
    },
    {
      "name": "Low",
      "short": "Low",
      "color": "yellow",
      "keywords": [
        "low"
      ],
      "value": 25
    },
    {
      "name": "Wishlist",
      "short": "Wish",
      "color": "sky",
      "keywords": [
        "wish",
        "wishlist"
      ],
      "value": 0
    }
  ]
}

That looks great to me.

Similar example for new maniphest.status.search. I'm happy to drop any of these fields if you think we won't need them (prefixes and suffixes leap to mind).

{
  "data": [
    {
      "name": "Open",
      "special": "default",
      "prefixes": [
        "open",
        "opens",
        "reopen",
        "reopens"
      ],
      "value": "open"
    },
    {
      "name": "Resolved",
      "name.full": "Closed, Resolved",
      "closed": true,
      "special": "closed",
      "transaction.icon": "fa-check-circle",
      "prefixes": [
        "closed",
        "closes",
        "close",
        "fix",
        "fixes",
        "fixed",
        "resolve",
        "resolves",
        "resolved"
      ],
      "suffixes": [
        "as resolved",
        "as fixed"
      ],
      "keywords": [
        "closed",
        "fixed",
        "resolved"
      ],
      "value": "resolved"
    },
    {
      "name": "Wontfix",
      "name.full": "Closed, Wontfix",
      "transaction.icon": "fa-ban",
      "closed": true,
      "prefixes": [
        "wontfix",
        "wontfixes",
        "wontfixed"
      ],
      "suffixes": [
        "as wontfix"
      ],
      "value": "wontfix"
    },
    {
      "name": "Invalid",
      "name.full": "Closed, Invalid",
      "transaction.icon": "fa-minus-circle",
      "closed": true,
      "claim": false,
      "prefixes": [
        "invalidate",
        "invalidates",
        "invalidated"
      ],
      "suffixes": [
        "as invalid"
      ],
      "value": "invalid"
    },
    {
      "name": "Duplicate",
      "name.full": "Closed, Duplicate",
      "transaction.icon": "fa-files-o",
      "special": "duplicate",
      "closed": true,
      "claim": false,
      "value": "duplicate"
    },
    {
      "name": "Spite",
      "name.full": "Closed, Spite",
      "name.action": "Spited",
      "transaction.icon": "fa-thumbs-o-down",
      "silly": true,
      "closed": true,
      "prefixes": [
        "spite",
        "spites",
        "spited"
      ],
      "suffixes": [
        "out of spite",
        "as spite"
      ],
      "value": "spite"
    }
  ]
}

Maybe just include name, value, closed and special?

I can't come up with much of a reason for most of the other fields, and if one exists we might want to expose them differently (for example, drop the "fa" prefix from icons; deal with i18n for the prefixes/suffixes).

Looks generally good, though.

With only those fields:

{
  "data": [
    {
      "name": "Open",
      "value": "open",
      "special": "default",
      "closed": false
    },
    {
      "name": "Resolved",
      "value": "resolved",
      "special": "closed",
      "closed": true
    },
    {
      "name": "Wontfix",
      "value": "wontfix",
      "closed": true
    },
    {
      "name": "Invalid",
      "value": "invalid",
      "closed": true
    },
    {
      "name": "Duplicate",
      "value": "duplicate",
      "special": "duplicate",
      "closed": true
    },
    {
      "name": "Spite",
      "value": "spite",
      "closed": true
    }
  ]
}

@aurelijus, D18111 is landed now in HEAD if you'd like to test this. Let us know if you have any issues.

Thanks! Will check.

After some additional poking, there are a couple of other compatibility issues here:

• Existing saved EditEngine forms will have the wrong priority setting after this change, since they're saved with a numerical value.
• You still can't /maniphest/?priorities=high,low, and if we make that change in the same vein as this we'll break existing saved queries.

I'm inclined to tweak things so we accept either 90, or "90" or "high" from internal sources (EditEngine, ApplicationSearch) for compatibility, and accept only "high" from maniphest.edit. We could migrate ApplicationSearch/EditEngine in the future if we want to get rid of this glue code at some point.

Are saved queries/EditEngine forms things we could write migrations for? That might be even more brittle (for example, what to do about saved queries for priorities that no longer exist on the install), but accepting three different priority specifications of two different types seems a little too Postel's Principle for me.

Yes, we could migrate these instead.

I'm personally more confident I can fix this quickly and safely by being more liberal in what we accept than I can by migrating.

If you're around this week and want to grab this from me, feel free to migrate instead -- I wasn't sure how off-the-grid Vegas meant. I think we should either fix this or revert D18111 (until we have a fix) today since, e.g., I think WMF is going to have 30 broken forms as soon as they upgrade otherwise and the only available remedy is "manually edit each one, hoping you remembered the right priority setting".

If you're happy long-term with accepting all three types, I'm ok with that as a permanent fix. I'm planning on working through this week, just with weird/inconsistent hours.

Oh, sorry, maybe I wasn't clear. My plan is this:

• Today: glue this together by accepting more values.
• Future: consider migrating.

That's what this was about, from T12124#227575 above:

We could migrate ApplicationSearch/EditEngine in the future if we want to get rid of this glue code at some point.

I'm more comfortable doing a glue step first because I think the migration is riskier, harder to test, and harder to recover from. If we glue and miss something else, we can keep adding glue until it's stable, then migrate at our leisure.

Ahhh, that makes more sense. I parsed that as saying "we could migrate the code" instead of the saved objects. If you want to add the glue code now and then assign it back to me after, I'll work on the migration in my Copious Free Time™.