dev/core#3063 APIv3 - Fix numeric option matching

[colemanw]

Overview

Fixes regression documented in dev/core#3063. The issue was caused by #21099.

Before

Option matching was skipped for all FK fields if a numeric value was given

After

Only skipped for campaign_id field, if positive integer given

Comments

The optimization was overly broad and had unintended side-effects

[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).

[eileenmcnaughton]

@colemanw I think campaign_id only is OK - it was a performance regression on campaign_id - we should target the rc

[colemanw]

@eileenmcnaughton I've added a fix, updated the description, and changed the base.

[eileenmcnaughton]

@colemanw can you put up a 5.46 too since it looks like a 5.46 drop is iminent. I'll check out once tests have passed

[eileenmcnaughton]

2107.3401 549103192 13. PHPUnit\Util\Filter::shouldPrintFrame($frame = ['file' => 'phar:///home/jenkins/bknix-dfl/extern/phpunit8/phpunit8.phar/phpunit/Framework/Constraint/IsEqual.php', 'line' => 83], $prefix = 'phar://phpunit-8.5.15.phar', $blacklist = class PHPUnit\Util\Blacklist { }) phar:///home/jenkins/bknix-dfl/extern/phpunit8/phpunit8.phar/phpunit/Util/Filter.php:57
2107.3401 549103192 14. realpath($path = '/home/jenkins/bknix-dfl/bin/cv') phar:///home/jenkins/bknix-dfl/extern/phpunit8/phpunit8.phar/phpunit/Util/Filter.php:72

Fatal error: Uncaught TYPO3\PharStreamWrapper\Exception: Unexpected file extension in "phar:///home/jenkins/bknix-dfl/extern/phpunit8/phpunit8.phar/phpunit/Framework/Constraint/IsEqual.php" in /home/jenkins/bknix-dfl/build/core-22740-6uo3l/web/sites/all/modules/civicrm/Civi/Core/Security/PharExtensionInterceptor.php on line 41

TYPO3\PharStreamWrapper\Exception: Unexpected file extension in "phar:///home/jenkins/bknix-dfl/extern/phpunit8/phpunit8.phar/phpunit/Framework/Constraint/IsEqual.php" in /home/jenkins/bknix-dfl/build/core-22740-6uo3l/web/sites/all/modules/civicrm/Civi/Core/Security/PharExtensionInterceptor.php on line 41

[eileenmcnaughton]

test this please

[colemanw]

retest this please

[colemanw]

I'm not sure why our test server keeps crashing when a test fails, but it makes it hard to spot what's going on due to the extremely high signal to noise ratio in the console output. The failing test was \CRM_Utils_TokenConsistencyTest::testContributionRecurTokenConsistency

[colemanw]

@eileenmcnaughton ok I finally tracked it down. The CRM_Utils_TokenConsistencyTest::testContributionRecurTokenConsistency test was failing because the ContributionRecur.payment_processor_id pseudoconstant was behaving incorrectly, and the previous is_numeric && FK check was bypassing the pseudoconstant and the test was (I assume by mistake) locking in the incorrect behavior.
#22750 fixes it (which complicates backporting this PR)

[totten]

Yeah, the failure mode in that test run was really weird.

@colemanw I'm a bit confused by the split between the 3 PRs. This seems to indicate that the failure in 22740 can be resolved by either:

• Modifying the patch (like in dev/core#3063 APIv3 - Fix numeric option matching #22751 for 5.46),
• Adding a supplemental fix (like Fix ContributionRecur.payment_processor_id pseudoconstant #22750 for master).

But then which approach does 5.47 take? It seems like the course of action would have to be one of these:

Approach	5.46	5.47	master
A	Modified patch (eg 22751)	Orig patch plus supplement (eg 22740+22750)	Orig patch plus supplement (eg 22740+22750)
B	Modified patch (eg 22751)	Modified patch (eg 22751)	Orig patch plus supplement (eg 22740+22750)

[colemanw]

@totten yes you are correct about the 2 options. I think it depends a bit on the impact of the supplement and how confident we are about releasing it next month rather than the month after. The safer approach is probably B, but A is easier git-wise because of the way we forward-merge RC branches into master. Personally I'd be fine with A.

If we go with A, then #22750 should get merged into 5.47, then this PR can be rebased and should pass tests.

[totten]

@colemanw Alright, in that case, #22750 needs to be rebased for 5.47 first.

[colemanw]

@totten done

[totten]

jenkins, test this please

[colemanw]

@eileenmcnaughton @totten this passes tests now that #22750 is merged