[REF] do not needlessly pass as reference, enforce valid param

[eileenmcnaughton]

Overview

[REF] do not needlessly pass as reference, enforce valid param

Before

Unnecessary pass-by-reference, array type not enforced

After

pass by ref removed, strict typing on array, simplifed return structure (do not use elseif after a return)

Technical Details

Quietly skipping an invalid param feels like a recipe for weirdness

Comments

This function isn't that heavily used & I couldn't find any cases of calling functions doing anything weird - it would fail hard & fast if there were an issue - probably during tests

[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]

@colemanw are you OK to merge this?

[colemanw]

@eileenmcnaughton I'd be happier if the function had a unit test :)

[eileenmcnaughton]

@colemanw I guess our tests go through this

[colemanw]

Ok

[demeritcowboy]

This is causing all the PR's to fail tests (CRM_Core_BAO_MessageTemplateTest::testContactTokens). An easy way to see what's happening is to run cv ev "$x = array('foo' => '0'); var_dump(CRM_Utils_Array::retrieveValueRecursive($x, 'foo'))"

BEFORE: NULL
AFTER: '0'

The second is more correct, and for tokens used in smarty if people are doing something like branching based on that value then typically it would evaluate the same either way since most people write smarty as {if $thing} which evaluates '0' as false. And to use a token value in such a way you'd have to {capture} it which flattens the typing anyway.

So the question is the other places this function is used that isn't covered by tests and whether there'd be a difference there.

[demeritcowboy]

It's not used much outside tokens. Maybe here is the only place it would make a difference, if you're importing blanks instead of '0's, but it might be an improvement, i.e. before blanks might overwrite (haven't tested):

civicrm-core/CRM/Contact/Import/Parser/Contact.php

Lines 1709 to 1716 in 64c5747

	$getValue = CRM_Utils_Array::retrieveValueRecursive($contact, $key);
	}
	if ($key == 'contact_source') {
	$params['source'] = $params[$key];
	unset($params[$key]);
	}
	if ($modeFill && isset($getValue)) {

[eileenmcnaughton]

Oh - the green light was BEFORE I added that test & it was when I was working on that code that I found the function so confusing I guess

[demeritcowboy]

So I am seeing a difference with import in fill mode before and after the patch. Suppose you have a contact that does not have DO NOT PHONE checked.

BEFORE: Importing a "1" in the import file checks the DO NOT PHONE setting as expected.
AFTER: Importing a "1" does NOT check the setting.

The difference seems to be that before the patch it treats the existing value as NULL and so goes ahead and imports it. After the patch it treats the existing value as "0" and so according to fill mode it shouldn't import it.

Blanks and "0" behave the same before and after the patch when importing against a contact the DOES have the setting checked, but then that's expected if the effect is on how it treats the existing value.

[eileenmcnaughton]

@demeritcowboy so we need to change this to if (!empty() ) to get no change

[demeritcowboy]

A quick run says yes.

[eileenmcnaughton]

OK - I'll put that up

[eileenmcnaughton]

#19667