Pseudoconstants - Fix and improve handling of option callbacks

[colemanw]

Overview

Ensures field option lists returned by APIv4 always have a stable id and name and that option matching works in non-English setups.

Before

Pseudoconstant callback functions must always return a flat [key => label] array.
When the API (v3 and v4) matches option names to ids, name is derived from label when converting simple associative to an option list. This would break non-English environments, but our test coverage of those is poor.

After

Pseudoconstant callback functions may return either a flat array or a multidimensional array containing id, name, label, or abbr. This allows more precision when matching.
When a callback returns only a flat array, name is derived from the key instead of the label.

Technical Details

Each item in a field option list contains keys like id, name, label, color, icon, description, abbr.
But some option lists only consist of simple key/value pairs (e.g. everything in CRM_Core_SelectValues). To convert those simple associative arrays into a full option list, the name should be derived from the id, not the label, because machine names are expected to be stable, and labels can be translated.

Switching the way options are matched by the API is is technically a breaking change, and it caused several test regressions in the first version of this PR.

Comments

Switching the way options are matched by the API is is technically a breaking change, and it caused several test regressions in the first version of this PR. The solution is to update callback functions to return the new multidimensional format, as this allows the name to remain the same as before, without passing it through ts() and breaking non-English envs.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

Test Result (51 failures / +51)
api\v4\Action\BasicActionsTest.testPseudoconstantMatch
api\v4\Entity\TranslationTest.testCreateOK with data set "asDraft"
api\v4\Entity\TranslationTest.testUpdateBad with data set "badStatus"
api\v4\Entity\TranslationTest.testUpdateBad with data set "malformedField"
api\v4\Entity\TranslationTest.testUpdateBad with data set "badTable"
api\v4\Entity\TranslationTest.testUpdateBad with data set "badFieldName"
api\v4\Entity\TranslationTest.testUpdateBad with data set "badFieldType"
api\v4\Entity\TranslationTest.testUpdateBad with data set "badEntityId"
api\v4\SearchDisplay\SearchRunTest.testCssRules
api_v3_JobTestCustomDataTest.testBatchMergeDateCustomFieldHandling
Show all failed tests >>>

[colemanw]

retest this please

[totten]

We talked about this online. It's a good idea. 👍

For r-run, I grepped xml/schema and pulled out a list of callbacks - then executed all of them and skimmed the result. I confirmed that there was a mix of old/short (key=>value) style and new//long style ([id=>...,name=>...,label=>...]).

This gave a list where I could pick a few differing examples to spot-check. Spot-checked each in the APIv4 Explorer and using some Quickform admin screens that referenced these fields. All of the spot-checks appeared to work in the before+after. 👍

With respect to r-tech compatibility, the main Q for me was how it distinguishes callbacks that return old-style and new-style. The key bit seems to be this:

  private static function formatArrayOptions($context, array &$options) {
    // Already flat; return keys/values according to context
    if (!isset($options[0]) || !is_array($options[0])) { .../* old format: key=>value pairs */ }
    /* else: new format: list of records, with numerical values */

I think this makes sense. It should correctly classify all three of these cases:

• Old style, string indexes ([r=>Red, g=>Green, b=>Blue])
• Old style, numerical indexes ([0=>Red, 1=>Green, 2=>Blue])
• New style, numerical indexes with array data ([0=>[name=>r,label=>Red], 1=>[name=>b,label=>Blue]])

Also, I think there's a lot of implicit test-coverage for these code-paths.

Looks good. Mrrrging.