Discussion on: Can you find the bug in this piece of php code? 🤹

[RRKallan]

My assumption would be that pin is an alphanumeric (letter or digit) value.

There are a couple possible fixes to prevent a bug / unexpected behaviour

• change the Abstract Equality Comparison (!=) to Strict Equality Comparison (!==)
• check if $params['pin'] is not empty. This will avoid <b>Warning</b>: Undefined array key when pin doesn't exist as param
• check if $user->getPin() is not empty else throw specific error that user doesn't have a pin
• trim value of $params['pin'] to avoid spaces on begin and end

My suggested solution would be as follow, with the assumption that $user->getPin() is a trimmed alphanumeric value.

$userPin = $user->getPin();
if (!empty($userPin)) throw new HttpException(422, "User has no pin, contact your system administrator");

$pin = !empty($params["pin"]) && ctype_alnum(trim($params["pin"])) ? trim($params["pin"]) : null;
if (!empty($pin)) throw new HttpException(403, "Pin is empty or isn't alphanumeric");

if ($pin === $userPin) return "The pin is correct!";

throw new HttpException(403, "The pin is incorrect");

[Keff]

Nice stuff, now that's how you do it!! Thanks for the detailed explanation

[RRKallan]

@nombrekeff thx

I see only one missing explanation

In my example i use !empty($pin) and not isset($pin)
Explanation as code

$x = " ";
$trimX = trim($x); // $trimX = ""

var_dump(isset($trimX)); // return true
var_dump(!empty($trimX)); // return false

as we can see from the output it would be wise to use empty() in this case

BTW i'm very curious about your solution :)

[Keff]

Ohh nice point, I did not know that (I'm not a php dev), so it's really helpfull. I guess isset only checks whether the variable exists right? And empty checks if the length > 0, have I gotten that right?

My solution is quite similar to yours actually, I just make sure the input is what I expect, and I don't trust user input whatsoever.

[RRKallan]

I would not define my self as PHP developer. My cup of thea is HTML(5), (S)CSS and Javascript :p

I guess isset only checks whether the variable exists right?

Yes isset checks if variable is defined.
If i'm right $x = undefined is not possible and needs to be null when no value is assigned

empty checks if the length > 0

correct if you mean !empty() > 0 is not a empty string if character length > 0, better said empty() returns true as boolean when character length = 0. NOTE a space is a character

[Keff]

Fantastic, thanks again! I might need to refresh my php knowledge