dev/core#4355 - Fix crash for radio custom fields

[demeritcowboy]

Overview

https://lab.civicrm.org/dev/core/-/issues/4355

Before

1. Create a radio field for activities.
2. Go to Contacts - New Activity.
3. Red alert box "network error" and the custom fields don't display. TypeError: Cannot access offset of type string on string in include() (line 31 of .../templates_c/en_US/%%1D/1DB/1DB03A28%%CustomField.tpl.php).

Using php 8. Doesn't happen in php 7.

After

• No crash.
• Reason for the mystery index looping solved.

Technical Details

Sooo, the $formElement variable it loops over contains both non-numeric keys, for things like the label and metadata, and numeric keys, for the option choices. The way the loop dealt with that, since it was first introduced in the 2000's, was to note that the array starts with 9 non-numeric keys, so let's skip them by counting to 9.

In #26265 it added a 10th (the change in CRM/Core/Form/Renderer).

It looks something like:

Array (12)
name => "custom_23_-1"
value => null
type => "group"
frozen => false
required => false
error => null
label => "<label>rad</label>"
separator => null
html => "<input data-crm-custom="Activity_file..."
textLabel => "rad"
1 => Array (10)
  name => "custom_23_-1"
  value => 1
  type => "radio"
  frozen => false
  required => false
  error => null
  id => "CIVICRM_QFID_1_custom_23_1"
  label => ""
  html => "<input data-crm-custom="Activity_file..."
  textLabel => ""
2 => Array (10)
  name => "custom_23_-1"
  value => 2
  type => "radio"
  frozen => false
  required => false
  error => null
  id => "CIVICRM_QFID_2_custom_23_1"
  label => ""
  html => "<input data-crm-custom="Activity_file..."
  textLabel => ""

Comments

Master-only regression.
FYI @larssandergreen

[civibot]

No issue was found matching the number given in the pull request title. Please check the issue number.

[civibot]

(Standard links)

• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[colemanw]

Well that was a creative solution! What could possibly go wrong with a hard-coded assumption about how many things are in an array?

[demeritcowboy]

Haha.
Actually I think this must be what the comment outside the block is talking about when it says "sort by". I'm wondering if I should remove that comment. In any case it's not a helpful comment if I can't tell what it means. And the assign for $index can go too.

[demeritcowboy]

jenkins retest this please

[larssandergreen]

Wow, fun. Thanks @demeritcowboy.

If I'm reading this right, the only reason for this is options per line anyways, which seems like it could be handled much more cleanly before we get to the tpl.

[demeritcowboy]

Thanks everybody.
Lars if you want to look into that then sure - I didn't look too much spending most of the time finding what caused it and trying to figure out what this tpl was doing. One thing to look out for if you're thinking of changing the array is if there's hooks expecting a certain structure. But it might be self-contained.

[MegaphoneJon]

I'm not sure why this is marked as a master-only regression - on June 12th, 5.63 should have been the rc, right? And this is present in 5.63. I think this should be backported.

[demeritcowboy]

Hmm yeah I must have missed backporting this one to 5.63. The other instances look like they were backported.

[demeritcowboy]

Ok added #26824