dev/core#1230 [Ref] Rationalise dedupe code loop. #15184
Conversation
|
(Standard links)
|
7246ed2 to
46b06e8
Compare
|
test this please |
|
@pfigel now you've done the important ones... this is just code tidy up but would be good to merge |
|
@demeritcowboy if you are still reviewing in this area maybe you could look at this one - no worries if not it's just a minor tidy up |
| @@ -873,16 +873,17 @@ public static function merge($dupePairs = [], $cacheParams = [], $mode = 'safe', | |||
| unset($dupePairs[$index]); | |||
| continue; | |||
| } | |||
| CRM_Utils_Hook::merge('flip', $dupes, $dupes['dstID'], $dupes['srcID']); | |||
There was a problem hiding this comment.
These lines relate to a single pair - so I moved them into the single pair function
| // return error | ||
| return FALSE; | ||
| if (($result = self::dedupePair($dupes, $mode, $checkPermissions, $cacheKeyString)) == FALSE) { | ||
| unset($dupePairs[$index]); |
There was a problem hiding this comment.
These relate to managing the loop - so I moved them OUT of the single pair function
|
Sure - looking... |
|
It's not really my main area and I've never looked at this block of code so it took me a bit to get into it but I was able to do some review.
|
We have a function 'dedupePair' which is intended to act on a specific pair and a loop function which iterates them. The 'pair actions' and the 'looping actions' are currently jumbled together. This moves the 'pair actions' to the dedupePair function and keeps the 'looping actions' in the parent looping function. Athough I left it out of scope for this PR the api that calls this function should only call the 'dedupePair' function not the multiple pair wrapper
46b06e8 to
b068bfd
Compare
|
@demeritcowboy I fixed the non strict comparison - strict is better practice. WRT the other - I really can't think of when it would be hit - it seems to be a check for something that should never be true. However, the 2 most valid behaviours seem to me to be 'skip this pair & continue' or throw an exception. In the 2 places this function is called from the batch merge expects a returned array & will get kinda weirded out if it doesn't get one. The api does cope with the 'FALSE' but would also cope with an exception. I think the handling might validation in the wrong place for the api call (any opinions @pfigel @JKingsnorth ?) |
|
@pfigel @bjendres @JKingsnorth Please can one of you comment on the final comment from @eileenmcnaughton and confirm if the issues identified by @demeritcowboy could be an issue? |
|
I think the change to unsetting makes sense in my opinion. i think it feels more correct to keep trying to go around the loop if we hit any issues |
|
Added the merge ready flag to give others any chance to comment but plan to merge in a couple of days time |
|
Merging per @seamuslee001 'in a couple of days time' comment |
Overview
Minor code cleanup
Before
Well tested function behaves as expected
After
Well tested function behaves as expected, but it also makes more sense
Technical Details
We have a function 'dedupePair' which is intended to act on a specific pair
and a loop function which iterates them.
The 'pair actions' and the 'looping actions' are currently jumbled together.
This moves the 'pair actions' to the dedupePair function
and keeps the 'looping actions' in the parent looping function.
Athough I left it out of scope for this PR the api that calls this
function should only call the 'dedupePair' function not the multiple
pair wrapper
Comments