do not strtolower() string values while building where clause for cus…

[jackrabbithanna]

…tom field queries

Overview

Querying the database via API by custom field values that contain uppercase strings returns no results when it should. Found for Alphanumeric fields with multi-select widgets, but could affect other types/widgets

For https://lab.civicrm.org/dev/core/issues/436

Before

Create a custom field, Alphanumeric, multi-select widget, setup at least one option value with an uppercase character. Hypothetical field has id of custom_21 . Update one contact that uses that custom field and set the option to the one that has an uppercase character.

$apiParams = [
'sequential' => 1,
'custom_21' => 'Alabama',
];

$result = civicrm_api3('Contact', 'get', $apiParams);

Returns no results

After

API call returns results correctly

Technical Details

Fixes for REGEX searches included adding a BINARY option to the RLIKE operator in
https://github.com/civicrm/civicrm-core/blob/master/CRM/Contact/BAO/Query.php#L5687
This makes the query case sensitve, so we can no longer strtolower() string values in where clauses.

[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).

[jackrabbithanna]

I'm keen to add some test coverage for this. Should I add/augment a test for the API? Perhaps in tests/phpunit/api/v3/CustomFieldTest.php ?

[jackrabbithanna]

Can this patch be worked into an upcoming minor release of 5.6.x ? I'll make a PR against the 5.6 branch if so.

[eileenmcnaughton]

@jackrabbithanna actually I might be a small tinker with api_v3_SyntaxConformanceTest::testCustomDataGet to make it try caps combos.

The thing we need to be really careful about is removing any php strtolowers in cases where the string could get passed to mysql LOWER. Without mysql LOWER it doesn't matter if the strtolower is present or not but with LOWER present it does. I can't see any evidence the string is going through mysql LOWER but that's what we should be wary of & checking for in tests.

From what I can tell that class is used for a variety of custom field types - but not for any fields other than custom fields. It's also used for a variety of operators. So I guess the test is to try a few operators - '=', 'REGEX' & 'LIKE' - assuming we can get there from the api.

[jackrabbithanna]

Blippity Blop, wasn't expecting so many test failures...obviously this has a few unexpected ramifications. Do you think these failures are valid?

[eileenmcnaughton]

7 tests have been failing since the server died :-( Tim hasn't had time to resolve yet. Not sure about the other one - let's retest & see if it is an intermittant

[eileenmcnaughton]

test this please

[jackrabbithanna]

Ok, now it has only 7 failures...Why would the other one go away, I didn't commit anything else? Maybe it's just a gremlin then...

Seems good, I'll look at adding/updating a test for this.

[jackrabbithanna]

I took a stab at some test updates for api_v3_SyntaxConformanceTest::testCustomDataGet to meet the needs here.

[eileenmcnaughton]

@jackrabbithanna there are a small number of tests that fail 'from time to time' - & the 8th on was one of those. Obviously there is some issue with those tests - e.g the assumptions in them are a bit date dependent - but they haven't been pressing enough to address

[jackrabbithanna]

@eileenmcnaughton I got some updates to the tests, not sure if you like it or not, could use a good review, seeing how it's a pretty crucial set of tests there. finally got apii_v3_SyntaxConformanceTest::testCustomDataGet to work, now it creates a multi-select custom field with a variety of options, lowercase, uppercase, integer, and makes sure that it gets right results back. Now all that fails is the seven "normal" test failures.

[eileenmcnaughton]

@jackrabbithanna this is very close - it's setting up the right stuff! I kinda hoped we could do a little less set up because it runs a lot of times & one custom data group would be 'cheaper than 2' - however there would be a bit more wrangling to get there & this set up works.

Where it isn't working is that it should fail without the patch & pass with. Currently it passes with & without. I figured it out though - you are testing what is returned rather than the filter - so

$getParams = [$customFieldNameMultiSelect => $option_value];

is more what you need. The section below worked for me in your test

    $ids2 = $this->entityCustomGroupWithSingleStringMultiSelectFieldCreate(__FUNCTION__, $usableName . 'Test.php');
    $customFieldNameMultiSelect = 'custom_' . $ids2['custom_field_id'];
    // String custom field, Multi-Select html type
    foreach ($ids2['custom_field_group_options'] as $option_value => $option_label) {
      $params = ['id' => $objects[0]->id, 'custom_' . $ids2['custom_field_id'] => $option_value];
      $result = $this->callAPISuccess($entityName, 'create', $params);
      $getParams = [$customFieldNameMultiSelect => $option_value, 'return' => [$customFieldNameMultiSelect]];
      $this->callAPISuccessGetCount($entityName, $getParams, 1);
    }

[eileenmcnaughton]

@jackrabbithanna did you see above -we are close to merging this

[jackrabbithanna]

@eileenmcnaughton ok committed your recommended fix to the test

[jackrabbithanna]

test this please

[jackrabbithanna]

Ran the tests twice here, getting 3 fails the first time after the commit, next time a different error...Not sure if it's the code, or something in infrastructure getting behind.

[eileenmcnaughton]

@jackrabbithanna I found out why. It really is special to the Note api & I think easiest is to add this to the top of the test

  public function testCustomDataGet($entityName) {
    if ($entityName === 'Note') {
      $this->markTestIncomplete('Note can not be processed here because of a vagary in the note api, it adds entity_table=contact to the get params when id is not present - which makes sense almost always but kills this test');
    }

[eileenmcnaughton]

This is a good change - it fixes a bug & removes a bad code pattern (per #12494 mysql handles lcase) and adds tests. Merge on pass

[colemanw]

@civicrm-builder retest this please

[eileenmcnaughton]

@jackrabbithanna pretty green tick - we are all go!