dev/core#3784 fix contribution/membership/participant import matching on external id or contact id

[highfalutin]

Overview

Fixes 5.51 regression https://lab.civicrm.org/dev/core/-/issues/3784 and adds a test for it.

Before

When importing contributions, supposedly matching on external identifier or contact id, imported contributions are instead attached to contact ids 1-9.

After

Imported contributions are attached to the contacts specified by external id/contact id in the input CSV.

Comments

Thank you @eileenmcnaughton for the fabulous work on refactoring nightmarish import code. The code was relatively easy to read/trace through, so finding this bug wasn't too hard!

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[seamuslee001]

this feels like the same sort of issue that @agileware-justin and folks hit but them on an participant import here #24153

[highfalutin]

this feels like the same sort of issue that @agileware-justin and folks hit but them on an participant import here #24153

Yep. I chose to make my fix a bit downstream from where @agileware-justin did. As far as the contribution import issue is concerned, the two fixes are redundant, but shouldn't be incompatible.

[highfalutin]

Correction: after looking more closely, I don't think the two fixes are exactly redundant. The version here actually does a better job of fixing this issue, and a similar patch would do a better job of fixing the Participant issue as well. Also I think the test is worth keeping.

[MegaphoneJon]

Nothing wrong with this, but could also be expressed more succinctly as:

$matchedIDs = (array) $error['error_message']['params'];

[colemanw]

@MegaphoneJon if you use the suggestion button then your suggestion can be committed (but you have to select both lines before you make your comment).

[highfalutin]

Thanks @MegaphoneJon. I incorporated this suggestion.

[agileware-fj]

The version here actually does a better job of fixing this issue, and a similar patch would do a better job of fixing the Participant issue as well.

How? The other patch fixes both of these issues and any other location that's still relying on the deprecated function that this issue comes from.

Never mind that.

If a better job is to be done than reverting the change in interface of a function that is in a class for "deprecated" utilities that should be being phased out, then it should be achieved by removing the dependency on said function.

The test is good because it lays out knowing when this removal has been achieved successfully, but ultimately patching the consumer of a deprecated interface to deal with a change that should never have happened in that interface is fundamentally wrong.

[highfalutin]

No offense intended, @agileware-fj. I should have explained. We have the following code in both the Contribution parser and the Participant parser:

$error = $this->checkContactDuplicate($formatValues);
// ...
$matchedIDs = explode(',', $error['error_message']['params'][0]);

Looking upstream, it appears that $error['error_message']['params'] might be a multiple-item array of matches, if there's no match by contact id/external id and the algorithm resorts to matching by some other criteria. If so, we shouldn't just take the first array item and ignore the rest.

Also, $error['error_message']['params'][0] will never be a comma-separated string; the explode is just burning energy. So it's "better" to skip it.

This PR changes the above code to

$error = $this->checkContactDuplicate($formatValues);
// ...
$matchedIDs = (array) $error['error_message']['params'];

The test function tests whether match-to-contact on external identifier does what it's supposed to do. That should be a valuable test even after all the deprecated code is done away with.

I do hear your and @agileware-justin 's frustration about this ancient spaghetti code and the changes to it. My understanding is that the recent major refactoring is a step in the direction of eliminating the mess. Nobody would have wished for regressions, but the eventual payoff seems worth having to fix a couple bugs in the interim code.

Also of course I wish neither of us had had to discover the bug as a result of our clients' live imports failing. I recall Eileen putting out a loud call for RC testers on this one, for exactly that reason. If only I had heeded that request!

[highfalutin]

Applied the fix to the other places it was needed (Membership import parser, elsewhere in Contribution import parser, and Participant import parser), used more succinct syntax suggested by @MegaphoneJon, and rebased on master.

[agileware-fj]

@highfalutin no offence was taken, just not convinced your assertion is correct and want it to be clear that just because the solutions overlap doesn't mean that they don't both apply.

The third thing that might need to be accounted for is if third party code is extending the CRM_Import_Parser class and calls checkContactDuplicate (which as far as I know is not actually deprecated, just calls into deprecated code). If it's followed the pattern laid out by the core classes then it will be expecting input that results in the same issue we see here. Hence the deprecated interface really still ought to (a) remain consistent (frozen as originally deprecated) and/or (b) start throwing warnings about incorrect usage if you try to index it – which sounds overly complicated for a deprecated function when you're already accounting for an implementation that sends an array anyway.

[totten]

OK, I think there's a good case for merging both, so I've posted a PR with both (#24182 for 5.52). If that passes, then I think we can merge all three PRs (master's #24153 + #24166 and 5.52's #24182).

[eileenmcnaughton]

I'm good with merging @totten's PR with both. With this PR there is a unit test so I can use that to remove the function call entirely from the Contribution import flow in master - and hopefully by the time the next rc is cut the DeprecatedUtils file will be entirely gone from core. However, for getting a release out with the bug fix I think Tim's approach is fine

Note that thrid party code is not supported for calling core functions 'just because they exist' - the expectation is that they will use the api & hooks

[eileenmcnaughton]

I added a PR to mark a few more functions as deprecated since it seems that would have been clearer - #24185 - but our top level message is that the things that are documented in the developer docs as supported interfaces are the supported interfaces.....