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

Remove CRM_Utils_Array::value from BAO_Contribution #18207

Closed
wants to merge 1 commit into from

Conversation

mattwire
Copy link
Contributor

Overview

Remove deprecated CRM_Utils_Array::value from CRM_Contribute_BAO_Contribution

Before

CRM_Utils_Array::value

After

??

Technical Details

Comments

@civibot
Copy link

civibot bot commented Aug 20, 2020

(Standard links)

@eileenmcnaughton
Copy link
Contributor

First fail relates.

Note we have hit regressions with this sort of change so I wouldn't personally be comfortable reviewing changes to so many different functions in one hit
Stacktrace
api\v4\Action\SqlFunctionTest::testGroupAggregates
Undefined index: id

/home/jenkins/bknix-dfl/build/core-18207-6xqpc/web/sites/all/modules/civicrm/CRM/Contribute/BAO/Contribution.php:102
/home/jenkins/bknix-dfl/build/core-18207-6xqpc/web/sites/all/modules/civicrm/CRM/Contribute/BAO/Contribution.php:493
/home/jenkins/bknix-dfl/build/core-18207-6xqpc/web/sites/all/modules/civicrm/Civi/Api4/Generic/Traits/DAOActionTrait.php:140
/home/jenkins/bknix-dfl/build/core-18207-6xqpc/web/sites/all/modules/civicrm/Civi/Api4/Generic/DAOSaveAction.php:41
/home/jenkins/bknix-dfl/build/core-18207-6xqpc/web/sites/all/modules/civicrm/Civi/Api4/Provider/ActionObjectProvider.php:68
/home/jenkins/bknix-dfl/build/core-18207-6xqpc/web/sites/all/modules/civicrm/Civi/API/Kernel.php:150
/home/jenkins/bknix-dfl/build/core-18207-6xqpc/web/sites/all/modules/civicrm/Civi/Api4/Generic/AbstractAction.php:238
/home/jenkins/bknix-dfl/build/core-18207-6xqpc/web/sites/all/modules/civicrm/tests/phpunit/api/v4/Action/SqlFunctionTest.php:49
/home/jenkins/bknix-dfl/extern/phpunit7/phpunit7.phar:615

@mattwire mattwire force-pushed the baocontributionutilsarray branch 2 times, most recently from 2265776 to 4813a59 Compare August 21, 2020 10:54
@mattwire mattwire force-pushed the baocontributionutilsarray branch 3 times, most recently from ab26cf0 to cd6076a Compare August 26, 2020 12:22
@mattwire mattwire marked this pull request as draft August 26, 2020 12:22
@mattwire mattwire force-pushed the baocontributionutilsarray branch from cd6076a to e5ed0fa Compare August 26, 2020 16:48
@mattwire mattwire marked this pull request as ready for review August 27, 2020 08:33

$currency = $params['prevContribution']->currency;
if ($params['contribution']->currency) {
$currency = $params['contribution']->currency;
}
$previousLineItemTotal = CRM_Utils_Array::value('line_total', CRM_Utils_Array::value($fieldValueId, $previousLineItems), 0);
$previousLineItemTotal = $previousLineItems[$fieldValueId]['line_total'] ?? 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically when there's a default value passed in it's not a 1-1 swap with ??. Consider cv ev "$a['b'] = NULL; var_dump(CRM_Utils_Array::value('b', $a, 4)); var_dump($a['b'] ?? 4);". The first outputs null and the second outputs 4. So to review you need to think about whether null would be possible/make sense in the original. For these 0 ones I think they're ok.

@@ -2312,7 +2312,7 @@ public static function transitionComponents($params, $processContributionObject
$membership->membership_type_id,
(array) $membership
);
$membership->status_id = CRM_Utils_Array::value('id', $status, $membership->status_id);
$membership->status_id = $status['id'] ?? $membership->status_id;
$membership->save();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this one's ok because of the way getMembershipStatusByDate works. id would either be missing completely or would be a number, so the swap is valid.

@@ -4022,7 +4011,7 @@ public static function buildOptions($fieldName, $context = NULL, $props = []) {
'version' => 3,
'id' => ($props['contribution_page_id']),
]);
$types = (array) CRM_Utils_Array::value('payment_processor', $page, 0);
$types = (array) $page['payment_processor'] ?? 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Need brackets since the cast will apply to the first one first.

Copy link
Contributor

Choose a reason for hiding this comment

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

@demeritcowboy if you want to pull out a handful of these to a reviewer's commit I'm happy to merge them - maybe a half dozen?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I pulled out a few - #18391.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks - I'll close this for now & if anyone wants to use it to help with another round they can (although from past experience doing these is easy - getting them reviewed is hard)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants