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

CRM-20154 dedupe code improvement, remove unused autoflip parameter #9873

Merged
merged 2 commits into from
Feb 26, 2017

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Feb 22, 2017

@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton
Copy link
Contributor Author

@JKingsnorth if you get a chance to look at this - its just a tidy up following that work we did on flipping in the prev_next table earlier - the $autoFlip parameter is not used so I wanted to get it out

@JKingsnorth
Copy link
Contributor

Sure I'll take a look asap, today or tomorrow I hope

Copy link
Contributor

@JKingsnorth JKingsnorth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's another 'autoFlip' param in the 'batchMerge' function that is always sent as 'TRUE' and then not actually used in the method.

This is good cleanup though, thanks for the time Eileen.

@@ -281,7 +281,7 @@ public static function &valuesByID($id, $flip = FALSE, $grouping = FALSE, $local
*
* @param bool $flip
*/
public static function lookupValues(&$params, &$names, $flip = FALSE) {
public static function lookupValues(&$params, $names, $flip = FALSE) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In comments just above it still refers to this as a 'reference array' of names. But looks like a good improvement since the names never get modified.

@JKingsnorth
Copy link
Contributor

Pushed up a new PR with these changes (extending your commits here): #9893

@eileenmcnaughton
Copy link
Contributor Author

I'm going to merge this now because these changes are agreed. I also agree with your extension to the changes, except there is one place missed, which I commented on your PR. I'll try to co-ordinate with you next week to get that last extra piece in and your additional changes merged.

@eileenmcnaughton eileenmcnaughton merged commit a7aed60 into civicrm:master Feb 26, 2017
@eileenmcnaughton eileenmcnaughton deleted the dedupe_code branch February 26, 2017 02:32
monishdeb pushed a commit to monishdeb/civicrm-core that referenced this pull request May 2, 2017
CRM-20154 dedupe code improvement, remove unused autoflip parameter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants