Skip to content

Prevent booleans from being encoded (converted to strings) by h() function #724

Merged
merged 1 commit into from Jul 26, 2012

3 participants

@SimonEast

This patch prevents booleans from being encoded (converted to strings) by the global h() function. New tests included.

Background

If booleans are encoded, they become strings which can easily cause fatal PHP errors in PHP versions around 5.2.9 and possibly 5.3 also.

$a = $this->Article->findById(99);  // potentially returns false
$a = h($a);                         // translates false to empty string
echo $a['Article']['title'];        // Fatal error on PHP 5.2.9 rather than warning

PHP 5.4.4 displays a warning when this is attempted (thankfully), but I'm not sure how many other PHP versions are affected by this.

Simon East Prevent booleans from being encoded (converted to strings) by h() fun…
…ction, helps prevent accidental fatal errors in some PHP versions around 5.2.9
1ea457e
@AD7six
CakePHP member
AD7six commented Jul 20, 2012

why would you do that:

$a = h($a); 

Moreover, why would you do that before testing if there is a result at all?

@markstory
CakePHP member

Won't $a['Article']['title'] still fail with a boolean value?

@SimonEast

@AD7six, it's a simplified code-snippet of course, but basically we encode all our variables before sending them to the view for safety/security (as Rails now does, and perhaps Cake should too). Sometimes the view will test whether a result exists, but by this point the variables are already encoded.

And @markstory, yes, PHP still gives a warning when you try and access array elements on a boolean, but the problem with PHP 5.2.9 (and possibly 5.3) is that when the variable is a string it produces a fatal error, which is really painful, and completely breaks a page. See my tests here:
http://paste2.org/p/2077941

So yes... With extra if()'s all through the controllers/views the issue could be worked around, but in my opinion this is the best place to fix it.

Simon.

@markstory
CakePHP member

Well given that 5.2 explodes on array offsets on strings, I think this is worth merging in. Any objections @AD7six?

@AD7six
CakePHP member
AD7six commented Jul 25, 2012

I guess less asplode is good with me.

@markstory markstory merged commit 4dd532d into cakephp:master Jul 26, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.