merging of associative arrays broken
| Project: | Ubercart |
| Version: | 6.x-2.0-rc7 |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
I'm taking _uc_price_get_handlers() as an example, but this may occur elsewhere too.
Using hook_uc_price_handler I should be able to override the default 'options' array to be used in uc_price(). The _uc_price_get_handlers() function sets up a default $handlers['options'] before invoking each of the hook_uc_price_handler implementations. When a hook is found which provides some options overrides, the following happens:
if (isset($hook['options']) && is_array($hook['options'])) {
$handlers['options'] += $hook['options'];
}However, this doesn't work in the way I think it was intended. It's my understanding that the options returned by the hooks ($hook['options']) should overwrite the default options configured in $handlers['options']. This code doesn't do that however - it will only merge in new options and won't overwrite the defaults. This is because when merging associative arrays using += and when there is the same key in both arrays, the value of the key on the left will be retained, for example,
$a = array('a' => 123, 'b' => 456);
$b = array('a' => 999, 'c' => 888);
$a += $b;The new value for
$a will be:$a = array('a' => 123, 'b' => 456, 'c' => 888);Is this really the intended behaviour? I hope not.
#1
Good point. And no I'd also hope that is not the intended behaviour - as shown by:
<?php// Merge any incoming options, giving them precedence.
$options += $handlers['options'];
?>
Rather I'd say it's just a miss on the fact that += keeps the left most keys. Whereas the key's at left need to be overwritten.
array_mergeoverwrites existing string keys.So it should be:
<?php// Merge any incoming options, giving them precedence.
$options = array_merge($options, $handlers['options']);
?>
And the same for (most other?) += occurrences in uc_price().
array_merge handles numeric keys differently, but this shouldn't be an as these arrays always have string based keys.
Would be good for chaos (who was largely responsible for uc_price) to chime in here and make sure this is correct.
#2
I think when the hook options are added to the handler options is the only time it's wrong. Every other time it's adding the default values to the user-supplied options.
I've kept the + operator just in case something passes in numeric keys. They shouldn't, but this should mean that nothing unexpected happens to them.
#3
#4
I think the same thing needs to be done to line 114 of uc_store/includes/uc_price.inc too, doesn't it?
// Merge any incoming options, giving them precedence.$options += $handlers['options'];
#5
I don't believe so. Whoever calls uc_price() can pass in $options themselves, and they may want to do things differently from the handlers. Hard-coded values are trumped by alter handlers, but they are trumped by user-given data.
However, that means the 'cache' key is set in the wrong place. Handlers can't change that setting because it is added to the $options before their settings. I suppose it really belongs with the other default values for $options.