Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When generating case-link token use the first array index for contact_id instead of hardcoded index 1 #118

Merged
merged 1 commit into from
Mar 19, 2018

Conversation

mattwire
Copy link
Collaborator

For historical reasons the Case API returns a 1-based array for contact_id/client_id. This is the only bit of code in webform_civicrm that relies on the first index being 1 instead of just using the first element in the array.

This PR removes that hardcoding without any functional changes but means that the code would continue working if the array indexing changed as we only care about the first element of the array, not what (meaningless) index it has.

@colemanw
Copy link
Owner

Looks safe to me.

@colemanw colemanw merged commit 3e716f1 into colemanw:7.x-4.x Mar 19, 2018
@colemanw
Copy link
Owner

colemanw commented Apr 13, 2018

Hey @mattwire this just came up in https://www.drupal.org/project/webform_civicrm/issues/2960553 and upon looking at the code again I'm finding it difficult to read and understand what's going on. The expression you use is

if ((count($caseEntity['values'][$caseID]['contact_id']) > 0) && !empty(reset($caseEntity['values'][$caseID]['contact_id'])))

which makes me scratch my head. You're counting the items in an array, and then resetting it and checking if the first value is empty. I think a simplified way to say that would be

if (!empty($caseEntity['values'][$caseID]['contact_id']) && reset($caseEntity['values'][$caseID]['contact_id']))

there may be an even more elegant expression, but at least the above avoids passing a function to empty() which really isn't necessary.

@mattwire
Copy link
Collaborator Author

@colemanw The problem of just using !empty is that we need to check that is an array and has at least one element, whereas !empty just tells us that there is something in ['contact_id'] which may be a value or an array. I think we could probably use CRM_Utils_Array::first() here but I think you like to avoid civi functions?

@colemanw
Copy link
Owner

It looks like the check specifically wants to know that it

  • Is an array
  • Has at least one value
  • The first value is not falsey

If that's the correct logic then I guess the code should be

if (
  !empty($caseEntity['values'][$caseID]['contact_id'])
  && is_array($caseEntity['values'][$caseID]['contact_id'])
  && reset($caseEntity['values'][$caseID]['contact_id'])
) {

@mattwire
Copy link
Collaborator Author

@colemanw Yes makes sense. See #131

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants