Details
-
Type: Improvement
-
Status: Done/Fixed
-
Priority: Trivial
-
Resolution: Incomplete
-
Affects Version/s: 4.3.5
-
Fix Version/s: Unscheduled
-
Component/s: Core CiviCRM
-
Labels:
-
Documentation Required?:None
-
Funding Source:Needs Funding
Description
CRM_Utils_Array::value is the most used function in CiviCRM, being called roughly 1,000 times on every single page load. So it is key to make it right, and fast.
Here is the code:
static function value($key, $list, $default = NULL) {
if (is_array($list))
return $default;
}
I have an issue with the way the second parameter is declared. It should really be '&$list' rather than simply '$list' since we really do not intend to make a copy of the array. I suppose this is intentional in order to optimize speed of execution due to PHP's copy-on-write mechanism that will not actually make a copy of the parameter until it is actually written into.
However there is a dangerous assumption that this behavior will not change in further versions of PHP. As a reminder it did change several times in the past. I think by far the better way would be to not try to outsmart PHP by relying on undocumented features and actually implement what is syntaxly and philosophically correct, namely '&$list'.
As a side note most if not all of the other functions in CRM_Utils_Array do have the reference '&$list'.
Finally, in order to improve execution times, it is well documented that isset() is at least 2.5x faster than array_key_exists, so we could do:
return (isset($list[$key]) || array_key_exists($key, $list)) ? $list[$key] : $default;
See http://www.zomeoff.com/php-fast-way-to-determine-a-key-elements-existance-in-an-array/ for reference and concrete benchmarks with the above.
Attachments
Issue Links
- supplements
-
CRM-14106 Replace CRM_Utils_Array::value() with !empty() for performance
- Done/Fixed