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

refs issue 788 fix Warning: A non-numeric value encountered in ... #13795

Merged
merged 2 commits into from
Mar 18, 2019

Conversation

kewljuice
Copy link
Contributor

Overview

Warning: A non-numeric value encountered in ...

Before

Warning: A non-numeric value encountered in ... was thrown on contribution pages & event registrations

After

Warnings are gone

Technical Details

Added extra '&' sign

Comments

https://lab.civicrm.org/dev/core/issues/788

@civibot
Copy link

civibot bot commented Mar 8, 2019

(Standard links)

@civibot civibot bot added the master label Mar 8, 2019
@MegaphoneJon
Copy link
Contributor

Thanks for the PR @kewljuice! Unfortunately I don't think this is the right fix.

A single ampersand is a bitwise operation; two ampersands is a logical "AND". The original code is meant to do some binary number trickery to arrive at a single result. Using AND means there are two statements to evaluate. The second one is a constant and will always evaluate to TRUE.

I suspect that the correct solution is either a) to cast $self->_paymentProcessor['billing_mode'] to an int, or (better) prevent it from being stored as a string in the first place. Unfortunately PHP was designed to consider strings containing integers the same as integers in some (but not all) scenarios; this is one where it matters.

@kewljuice
Copy link
Contributor Author

I've removed the extra '&' sign and added (int) casting.

@kewljuice kewljuice changed the title refs issue 788 added extra & sign. refs issue 788 Mar 9, 2019
@eileenmcnaughton eileenmcnaughton changed the title refs issue 788 refs issue 788 fix Warning: A non-numeric value encountered in ... Mar 18, 2019
@eileenmcnaughton
Copy link
Contributor

Changes are in line with recommendations by @MegaphoneJon & I agree that's the right fix

@eileenmcnaughton eileenmcnaughton merged commit 0bd8046 into civicrm:master Mar 18, 2019
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.

3 participants