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 unused variables from repeatTransaction #18209

Merged
merged 1 commit into from
Aug 21, 2020

Conversation

mattwire
Copy link
Contributor

Overview

More simplification

Before

Unused variables.

After

Not

Technical Details

See inline comments

Comments

@eileenmcnaughton

@civibot
Copy link

civibot bot commented Aug 20, 2020

(Standard links)

@civibot civibot bot added the master label Aug 20, 2020
@@ -2613,7 +2613,7 @@ public static function contributionCount($contactId, $includeSoftCredit = TRUE)
* @return bool|array
* @throws CiviCRM_API3_Exception
*/
protected static function repeatTransaction(&$contribution, &$input, $contributionParams) {
protected static function repeatTransaction(&$contribution, $input, $contributionParams) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing from $input is used after repeatTransaction

@@ -4498,7 +4498,7 @@ public static function completeOrder($input, &$ids, $objects, $isPostPaymentCrea
], array_intersect_key($input, array_fill_keys($inputContributionWhiteList, 1)
));

$contributionParams['payment_processor'] = $input['payment_processor'] = $paymentProcessorId;
$contributionParams['payment_processor'] = $paymentProcessorId;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

$input['payment_processor'] is not used.

@eileenmcnaughton
Copy link
Contributor

eileenmcnaughton commented Aug 20, 2020

I don't think we can realistically test removing things that we think do nothing (ok-without-test). I took a quite look at the first line & $input is passed to another function after this so I would need to dig more thoroughly (ie I can't merge this in my triage pass)

@mattwire
Copy link
Contributor Author

@eileenmcnaughton Re. $input, it's passed to handleMembershipIDOverride() where membership_id is read but nothing is set. A follow up PR could probably simplify the parameters to handleMembershipIDOverride().

Once repeatTransaction returns you'll see similar, only amount, component, is_email_receipt and trxn_id are read, all of which are set before the call to repeatTransaction.

@eileenmcnaughton
Copy link
Contributor

OK - I've been through this a bit more carefully & I agree that nothing set in repeatTransaction is ever used again.

This is a good cleanup because the time it took us to figure this out means it doesn't have to be figured out in the context of another change later. Merging

@eileenmcnaughton eileenmcnaughton merged commit 72a028a into civicrm:master Aug 21, 2020
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.

2 participants