I found this bug when visiting the Payment Settings overview page.

I have a custom payment method module installed, and the default value of one of the settings on the Payment Methods settings page is an array (to populate a multiple select). The function summarize_element() compiles the default values together to show a summary of the configuration options. The value of my configuration option is an array, but summarize_element() throws the following error:

warning: Illegal offset type in sites/all/modules/ubercart/uc_store/includes/summaries.inc on line 99.

I have applied a patch which, for selects-only, will first check if the value is an array, and if so, it will return the selected options in a nested list. Attached is the patch, and a screenshot so you can see how it looks.

CommentFileSizeAuthor
#5 512900_summarize_multiple_select.patch938 bytesIsland Usurper
#4 ubercart.patch1.3 KBjoelstein
Screenshot.png36.26 KBjoelstein
ubercart.patch1.22 KBjoelstein

Comments

rszrama’s picture

rszrama commented
Status: Needs review » Needs work

Thanks for the bug report! I reviewed the patch, but I'm not sure I want to commit it as is. Basically, I'm wondering if it would be a little more foolproof to look for #multiple to determine whether or not a select element is multiple select. Also, I think I'd prefer to see a comma separated list instead of an HTML list. None of the other default summaries use HTML, and I think it would be best to keep it that way. That could totally be overridden with a custom summary callback of course.

cha0s’s picture

cha0s commented

Aw, but the HTML lists are so purrrty ;>

rszrama’s picture

rszrama commented

I could be convinced. I suppose it's not realistic to have a comma separated list for long options like this...

joelstein’s picture

joelstein commented
Status: Needs work » Needs review
StatusFileSize
new ubercart.patch1.3 KB

I prefer the list, because it follows the nested list convention on that page, and it is easier to read (especially for long lists). But I did replace the HTML with a nested array, which theme_item_list() handles superbly. I also added the check to make sure the select is a multiple select. Works like a charm!

Island Usurper’s picture

Island Usurper commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 512900_summarize_multiple_select.patch938 bytes

Did some code cleanup. Removed the outer loop over the options, and removed the duplicated code in the else block.

I had to make up a multiple select element since we aren't using any yet, but I think it looks alright as a list on the summary pages.

Island Usurper’s picture

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

Committed.

Status: Fixed » Closed (fixed)

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