Enable 'Phone' custom data

[mattwire]

Overview

If you enable custom data for "Phone" entity type in CiviCRM it should be usable in webform_civicrm (enable via cg_extends option group).

Before

Cannot use custom data for "Phone" entity.

After

Can use custom data for "Phone" entity.

Technical Details

This just adds the entity type to a list of allowed types in webform_civicrm

Comments

Tested that "Phone" custom data is loaded, selectable and saved by webform_civicrm (note that you cannot see it in the CiviCRM UI but it can be retrieved via the API.

[colemanw]

Hmm, that if ($dao->entity_type == ... conditional is getting long and unwieldy. I think we could improve this setup by adding a little metadata to the sets, e.g.

 $sets = array(
   'contact' => array('entity_type' => 'contact', 'label' => t('Contact Fields'), 'custom_fields' => 'separate'),
    ...
   'phone' => array('entity_type' => 'contact', 'label' => t('Phone'), 'max_instances' => 9, 'custom_fields' => 'combined'),

and then change that unwieldy conditional to read if ($set['custom_fields'] == 'combined')

That would also improve this pr to not just work with phones but also emails, etc.

[mattwire]

@colemanw Thankyou for the review. I've updated the PR to use metadata instead of expanding the conditional - I agree that's a much better way.

However, I also realised that on create, submitted values are being set (ie. custom data is set for a phone number when creating it via webform). But on update the slightly mysterious (to me) reorderLocationValues() function does some magic which results in only the actual phone number (or email address) being updated in CiviCRM as the other parameters are stripped.

This change (https://github.com/colemanw/webform_civicrm/pull/147/files#diff-59be2e079a691088e15ab3a80e0640a0R2513) allows all submitted values to be updated in CiviCRM - what do you think?

[colemanw]

The above 3 should also be 'combined'

[mattwire]

ok. Done (I'll squash commits once we've completed review)

[colemanw]

This is looking good. For the sake of consistency I think we should either add this to the other sets or remove it from the "contact" entity. For now I think let's just do the latter as the property isn't being used for contacts, etc.

[colemanw]

To your question about the reorderLocationValues function. I honestly don't understand that function. It was contributed as a PR and got several positive reviews so I merged it but it's proved to be buggy since then and has caused a few problems. If you say that your change fixes something, ok I'll merge it, but personally trying to read that function makes my head sore.

[KarinG]

What @colemanw said - including the bit about reorderLocationValues - gives me headaches too.

[mattwire]

@powdevel Any thoughts on the change in reorderLocationValues please?

[colemanw]

Thanks for squashing this @mattwire. Good to go.

[beto-aveiga]

@mattwire sorry it seems I'm the best at missing notifications 😞
I would have to look that in action with Xdebug to see how it affects the execution of the code. I think for tomorrow I can answer your question.

[colemanw]

@powdevel thanks for taking a look. Please let us know what you find.

[colemanw]

Oh, I see the discussion is continued on #148