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

Fix incorrect operator on previous Export fix #12278

Merged
merged 1 commit into from
Jun 7, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jun 7, 2018

Overview

Follow up fix to #12212 where I used the wrong operator on $value[4]

Before

code has different meaning to pre-refactor

After

code has same meaning

Technical Details

This re-instates the original code more closely but on testing through the UI I can't see any way that $value[2] would be empty if $value[4] is not ie the elseif actually seems unreachable to me - so perhaps we don't need this & should instead remove the possibility of it. Hmmm

          if (!empty($value[2])) {
            $relationField = CRM_Utils_Array::value(2, $value);
            if (trim(CRM_Utils_Array::value(3, $value))) {
              $relLocTypeId = CRM_Utils_Array::value(3, $value);
            }
            else {
              $relLocTypeId = 'Primary';
            }

            if ($relationField == 'phone') {
              $relPhoneTypeId = CRM_Utils_Array::value(4, $value);
            }
            elseif ($relationField == 'im') {
              $relIMProviderId = CRM_Utils_Array::value(4, $value);
            }
          }
          elseif (!empty($value[4])) {
            $relationField = CRM_Utils_Array::value(4, $value);
            $relLocTypeId = CRM_Utils_Array::value(5, $value);
            if ($relationField == 'phone') {
              $relPhoneTypeId = CRM_Utils_Array::value(6, $value);
            }
            elseif ($relationField == 'im') {
              $relIMProviderId = CRM_Utils_Array::value(6, $value);
            }
          }

Comments

@monishdeb per #12272 do you agree we need this in the rc?

@civibot
Copy link

civibot bot commented Jun 7, 2018

(Standard links)

@monishdeb
Copy link
Member

Yes without a doubt. I am merging this as I tested earlier in my local and again on test build.

@eileenmcnaughton
Copy link
Contributor Author

@monishdeb did you figure out how to reach those rows I mentioned?

@eileenmcnaughton eileenmcnaughton merged commit 5100ef2 into civicrm:5.3 Jun 7, 2018
@eileenmcnaughton eileenmcnaughton deleted the 5.3 branch June 7, 2018 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants