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

Flatten fix #13176

Merged
merged 4 commits into from
Dec 1, 2018
Merged

Flatten fix #13176

merged 4 commits into from
Dec 1, 2018

Conversation

aydun
Copy link
Contributor

@aydun aydun commented Nov 29, 2018

Overview

Bug fix for CRM_Utils_Array::flatten() extracted from #12012 Flatten should restructure the array and return all original keys, not filter based on value.

Includes a test

Before

Keys with empty values are not in the flattened array

After

Keys with empty values are included in the flattened array

Comments

There are only a few uses of this function in the codebase. So far as I can see, none of them assume empty values will be filtered out.

@civibot
Copy link

civibot bot commented Nov 29, 2018

(Standard links)

@civibot civibot bot added the master label Nov 29, 2018
@eileenmcnaughton
Copy link
Contributor

test this please

@eileenmcnaughton
Copy link
Contributor

@aydun these are the places that call that function. I think it's logical that tokens should process empty tokens too and I can't see why any of the DAO / handler places would be empty.

screenshot 2018-11-30 09 01 35

I am a bit less sure about the dedupe call. I feel like the change is likely OK but could have unexpected consequences so I think it makes sense to add an array_filter call in the dedupe path

My suggestion is always to do any wrangling as close to where it is used as possible & in this case the array is passed through to a function that is only called from one place - so I would go with

diff --git a/CRM/Dedupe/Finder.php b/CRM/Dedupe/Finder.php
index 45689b9b2c..1929b9e38b 100644
--- a/CRM/Dedupe/Finder.php
+++ b/CRM/Dedupe/Finder.php
@@ -125,6 +125,8 @@ class CRM_Dedupe_Finder {
     if (!$params) {
       return array();
     }
+    // This may no longer be required - see https://github.com/civicrm/civicrm-core/pull/13176
+    $params = array_filter($params);
 
     $foundByID = FALSE;
     if ($ruleGroupID) {

@eileenmcnaughton
Copy link
Contributor

eileenmcnaughton commented Nov 29, 2018

let's try again - test fails are

  Test Result (3 failures / ±0)Civi\API\Subscriber\WhitelistSubscriberTest::testEach.testEach with data set #25Civi\API\Subscriber\WhitelistSubscriberTest::testEach.testEach with data set #26Civi\API\Subscriber\WhitelistSubscriberTest::testEach.testEach with data set #28

@eileenmcnaughton
Copy link
Contributor

test this please

@aydun
Copy link
Contributor Author

aydun commented Nov 30, 2018

Thanks @eileenmcnaughton I've added that change to CRM/Dedupe/Finder.php

@eileenmcnaughton
Copy link
Contributor

test this please

@aydun
Copy link
Contributor Author

aydun commented Nov 30, 2018

test this please

@aydun
Copy link
Contributor Author

aydun commented Nov 30, 2018

@eileenmcnaughton Are you ok to merge this now?

@eileenmcnaughton eileenmcnaughton merged commit fe4a5e8 into civicrm:master Dec 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants