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

[REF] remove never-set, mispelt parameter #14907

Merged
merged 1 commit into from
Jul 29, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jul 29, 2019

Overview

On digging into an issue around multiple payments I found the parameter $form->_forcePayement if never set to TRUE (the place where it was obviously expected to be possibly set was commented out) so it should go

Before

Parameter present

After

poof

Technical Details

Note I removed an 'if' that relies on it being TRUE since it never is

Comments

I am digging into eileenmcnaughton/nz.co.fuzion.omnipaymultiprocessor#127 & I know that many years ago I wrote the code to move the payment block off the first page in this flow but abandoned it unreviewed (mostly because it was before LeXIM & it the processes were poorer back then).

I suspect this might be part of that abandoned attempt

@civibot
Copy link

civibot bot commented Jul 29, 2019

(Standard links)

@civibot civibot bot added the master label Jul 29, 2019
@@ -888,7 +888,7 @@ public static function formRule($fields, $files, $form) {
$isZeroAmount = TRUE;
}

if ($isZeroAmount && !($form->_forcePayement && !empty($fields['additional_participants']))) {
if ($isZeroAmount) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hang on have I got this right - $form->_forcePayement was ALWAYS false so it was never the case that the second condition was true & hence it was meaningless

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 that is right unless it works becasue !FALSE but urg that is annoying as hell might be worth just testing with a small PHP script of something like

if (!(FALSE && !empty(['hi'])) {
  print_r('See hi');
}
else {
  print_r('So it wasn't validating');
}

Copy link
Contributor

Choose a reason for hiding this comment

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

doing testing found that even if the 2nd part of that inner IF was TRUE the whole section was returning TRUE because forcePayment was false so Not FALSE is TRUE so makes sense to clear it all out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was a mind mess though!

@seamuslee001
Copy link
Contributor

Changes look good and fine to me and i am confident we have reasonable tests in this area merging

@seamuslee001 seamuslee001 merged commit 80c95b8 into civicrm:master Jul 29, 2019
@seamuslee001 seamuslee001 deleted the force_pay branch July 29, 2019 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants