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.

CommentFileSizeAuthor
#5 610916_price_hook_options.patch987 bytesIsland Usurper
#2 610916_price_hook_options.patch464 bytesIsland Usurper

Comments

fenstrat’s picture

fenstrat
he/him
Primary language English
Location Australia
commented

Good point. And no I'd also hope that is not the intended behaviour - as shown by:

  // 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_merge overwrites existing string keys.

array_merge: "If the input arrays have the same string keys, then the later value for that key will overwrite the previous one."

So it should be:

  // 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.

Island Usurper’s picture

Island Usurper commented
StatusFileSize
new 610916_price_hook_options.patch464 bytes

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.

Island Usurper’s picture

Island Usurper commented
Status: Active » Needs review
stella’s picture

stella commented
Status: Needs review » Needs work

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'];
Island Usurper’s picture

Island Usurper commented
Status: Needs work » Needs review
StatusFileSize
new 610916_price_hook_options.patch987 bytes

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.

klapper’s picture

klapper commented
Version: 6.x-2.0-rc7 » 6.x-2.4
Status: Needs review » Reviewed & tested by the community

Patch #5 works perfectly.

Island Usurper’s picture

Island Usurper commented
Status: Reviewed & tested by the community » Fixed

Excellent. Committed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.