unexpected cast to boolean?

Question

Given this input: http://example.com/item.php?room=248&supply_id=18823, the following 2 blocks ought to produce the same result. Why don't they? What am I missing other than coffee?

This block gives the expected values:

if (isset($_GET['supply_id']) && isset($_GET['room'])) {
    $id=validkey($_GET['supply_id']); //18823
    $room=validkey($_GET['room']); //248
    $arr=array('s'=>$id,'r'=>$room); //s=>18823, r=>248
}

But if I do the check and the assignment in one step, $id ends up equal to 1 instead of 18823. Why?

if (isset($_GET['supply_id']) && isset($_GET['room'])) {
    if($id=validkey($_GET['supply_id']) && $room=validkey($_GET['room'])) 
        $arr=array('s'=>$id",'r'=>$room); //s=>1, r=>248
}

This is the function I'm using:

function validkey($value){
    if(is_scalar($value)){
        $value=(int)$value;
        return ($value>0) ? $value : false;
    }
    return false;
}

score 7 · Accepted Answer · answered Feb 23 '11 at 14:12

7

You should use parentheses:

if(($id=validkey($_GET['supply_id'])) && ($room=validkey($_GET['room'])))

Otherwise the result of validkey($_GET['supply_id']) && $room=validkey($_GET['room']) is assigned to $id variable because && operator has higher precedence than =

answered Feb 23 '11 at 14:12

meze

14,975
4
47
52

This is the answer. I've run into this problem before and it's really confusing the first time you encounter it - `&&` has a higher precedence than `=`. So you have to wrap the assignment in an extra set of parentheses. – Andrew Feb 23 '11 at 14:16
However I think this sort of syntax is generally a bad idea - it makes code harder to follow, and is more open to errors creeping in. – BrynJ Feb 23 '11 at 14:18
Yup, or use `AND` which i don't like ;) – meze Feb 23 '11 at 14:18
Use it sparingly. I've occasionally done something like `if (($aRow = mysql_fetch_assoc($rQuery)) && $aRow['foo'] == true)`, but having array dereferencing someday will negate the need for that. – Andrew Feb 24 '11 at 14:13

score 3 · Answer 2 · answered Feb 23 '11 at 14:13

3

The && operator binds stronger than the = operator.

So your code basically becomes if ($id = (validkey($_GET['supply_id']) && $room = validkey($_GET['room'])))

--> Add parenthesis around the $foo = $bar expressions in your IF statement.

answered Feb 23 '11 at 14:13

ThiefMaster

310,957
84
592
636

BrynJ · Answer 3 · 2011-02-23T14:28:22.957

2

You seem to have a small error in your second example - a stray doubble quote after $id. Also, your second approach is generally frowned upon (assigning variables in an if construct) as it makes code considerably harder to follow. Clearer would be the following:

if (isset($_GET['supply_id']) && isset($_GET['room'])) {     
    $id=validkey($_GET['supply_id']); //18823     
    $room=validkey($_GET['room']); //248

    if($id && $room) {     
        $arr=array('s'=>$id,'r'=>$room); //s=>18823, r=>248 
    }
}

edited Feb 23 '11 at 14:28

answered Feb 23 '11 at 14:17

BrynJ

8,322
14
65
89

the extra quote was a typo. I've fixed it. I can see where doing the assignment and checking for truth would be clearer if done in 2 steps, but I don't think I would call `validkey` twice. The internal `if` need only by `if($id && $room){}` – dnagirl Feb 23 '11 at 14:23
You're quite right, I just hadn't read the code snippet I cut and pasted from :) I've updated my answer. – BrynJ Feb 23 '11 at 14:28

unexpected cast to boolean?

3 Answers3

Linked