views:

623

answers:

7

I have several older applications that throw a lot of "xyz is undefined" and "undefined offset" messages when running on the E_NOTICE error level, because the existence of variables is not explicitly checked using isset() and consorts.

I am considering working through them to make them E_NOTICE compatible, as notices about missing variables or offsets can be lifesavers, there may be some minor performance improvements to be gained, and it's overall the cleaner way.

However, I don't like what inflicting hundreds of isset() empty() and array_key_exists() s does to my code. It gets bloated, becomes less readable, without gaining anything in terms of value or meaning.

Is there anybody out there who feels the same way? What do you do about it?

I am looking, and hoping, for workarounds, magic functions, and/or best practices for minimizing the impact of strict variable checking on code readability.

+10  A: 

Just write a function for that. Something like:

function get_string($array, $index, $default = null) {
    if (isset($array[$index]) && strlen($value = trim($array[$index])) > 0) {
        return get_magic_quotes_gpc() ? stripslashes($value) : $value;
    } else {
        return $default;
    }
}

which you can use as

$username = get_string($_POST, 'username');

Do the same for trivial stuff like get_number(), get_boolean(), get_array() and so on.

BalusC
This looks good, and does magic_quotes checking as well. Nice!
Pekka
Great function! Thanks a lot for sharing.
letseatfood
+16  A: 

IMHO you should think about not just making the app "E_NOTICE compatible", but restructuring the whole thing. Having hundreds of points in your code that regularly try to use non-existent variables sounds like a rather badly structured program. Trying to access non-existent variables should never ever happen, other languages balk at this at compile time. The fact that PHP allows you to do it doesn't mean you should.

These warnings are there to help you, not to annoy you. If you get a warning "You're trying to work with something that doesn't exist!", your reaction should be "Oops, my bad, let me fix that ASAP." How else are you going to tell the difference between "variables that work just fine undefined" and honestly wrong code that may lead to serious errors? This is also the reason why you always, always, develop with error reporting turned to 11 and keep plugging away at your code until not a single NOTICE is issued. Turning error reporting off is for production environments only, to avoid information leakage and provide a better user experience even in the face of buggy code.


To elaborate:

You will always need isset or empty somewhere in your code, the only way to reduce their occurrence is to initialize your variables properly. Depending on the situation there are different ways to do that:

Function arguments:

function foo ($bar, $baz = null) { ... }

There's no need to check whether $bar or $baz are set inside the function because you just set them, all you need to worry about is if their value evaluates to true or false (or whatever else).

Regular variables anywhere:

$foo = null;
$bar = $baz = 'default value';

Initialize your variables at the top of a block of code in which you're going to use them. This solves the !isset problem, ensures that your variables always have a known default value, gives the reader an idea of what the following code will work on and thereby also serves as a sort of self-documentation.

Arrays:

$defaults = array('foo' => false, 'bar' => true, 'baz' => 'default value');
$values = array_merge($defaults, $incoming_array);

The same thing as above, you're initializing the array with default values and overwrite them with actual values.

In the remaining cases, let's say a template where you're outputting values that may or may not be set by a controller, you'll just have to check:

<table>
    <?php if (!empty($foo) && is_array($foo)) : ?>
        <?php foreach ($foo as $bar) : ?>
            <tr>...</tr>
        <?php endforeach; ?>
    <?php else : ?>
        <tr><td>No Foo!</td></tr>
    <?php endif; ?>
</table>

If you find yourself regularly using array_key_exists, you should evaluate what you're using it for. The only time it makes a difference is here:

$array = array('key' => null);
isset($array['key']); // false
array_key_exists('key', $array); // true

As stated above though, if you're properly initializing your variables, you don't need to check if the key exists or not, because you know it does. If you're getting the array from an external source, the value will most likely not be null but '', 0, '0', false or something like it, i.e. a value you can evaluate with isset or empty, depending on your intent. If you regularly set an array key to null and want it to mean anything but false, i.e. if in the above example the differing results of isset and array_key_exists make a difference to your program logic, you should ask yourself why. The mere existence of a variable shouldn't be important, only its value should be of consequence. If the key is a true/false flag, then use true or false, not null. The only exception to this would be 3rd party libraries that want null to mean something, but since null is so hard to detect in PHP I have yet to find any library that does this.

deceze
True, but most failing access attempts are along the lines of `if ($array["xyz"])` instead of `isset()` or `array_key_exists()` which I find somewhat legitimate, certainly not structural problems (correct me if I'm mistaken). Adding `array_key_exists()` just looks like a terrible waste to me.
Pekka
I can't think of any case where I'd use `array_key_exists` instead of a simple `isset($array['key'])` or `!empty($array['key'])`. Sure, both add 7 or 8 characters to your code, but I'd hardly call that a problem. It also helps to clarify your code: `if (isset($array['key']))` means this variable is indeed optional and may be absent, whereas `if ($array['key'])` just means "if true". If you get a notice for the latter one, you know your logic is screwed somewhere.
deceze
I believe the difference between isset() and array_key_exists() is that the latter will return true if the value is NULL. isset() won't.
Htbaa
True, but I couldn't think of a sane use case where I need to differenciate between a non-existent variable and a set key whos value is null. If the value evaluates to FALSE the distinction should be without a difference. :)
deceze
+1  A: 

I'm here with you. But PHP designers has made a lot more worse mistakes than that. Short of defining custom function for any value reading, there's no way around it.

vava
worse mistakes that *what*?
just somebody
isset() stuff. Making everything null by default would save a lot of troubles.
vava
And what is this 'everything'? It would seem like a waste for PHP to have to imagine every conceivable variable name and set each to NULL just so a lazy developer can avoid typing 5 characters.
Lotus Notes
@Byron, look, it is really simple, a lot of other languages do that, Ruby and Perl as few examples. VM knows if variable was used before or not, doesn't it? It always can return null instead of failing with or without error message. And this is not about lousy 5 characters, it's about writing `params["width"] = params["width"] || 5` to set defaults instead of all that nonsense with `isset()` calls.
vava
A: 

I'm not sure what your definition of readability is, but proper use of empty(), isset() and try/throw/catch blocks, is pretty important to the whole process. If your E_NOTICE is coming from $_GET or $_POST, then they should be checked against empty() right along with all the other security checks that that data should have to pass. If it's coming from external feeds or libraries, it should be wrapped in try/catch. If it's coming from the database, $db_num_rows() or it's equivalent should be checked. If it's coming from internal variables, they should be properly initialized. Often, these types of notices come from assigning a new variable to the return of a function that returns FALSE on a failure, those should be wrapped in a test that, in the event of a failure, can either assign the variable an acceptable default value that the code can handle, or throwing an exception that the code can handle. These things make the code longer, add extra blocks, and add extra tests, but I disagree with you in that I think they most definitely add extra value.

Mlutz
This is all good and true (with the exception of some practices some might do otherwise), but I don't see how it's related to my question, and I don't think you can imply from a usage of `if($varname)` instead of `if(isset($varname))` that an application is insecure. My question is related to 1. older projects needing an overhaul and my unwillingness to riddle each if statement with one or more functions; and 2. my looking for ways how to make the `necessary` `empty()` and `isset()` calls without cluttering the code to the extent I think these functions do.
Pekka
+1  A: 

I don't mind using the array_key_exists(), in fact I prefer using this specific function rather than relying on *hack* functions which may change their behavior in the future like empty and isset (strikedthrough to avoid susceptibilities).


I do however, use a simple function that comes handy in this, and some other situations in dealing with array indexes:

function Value($array, $key, $default = false)
{
    if (is_array($array) === true)
    {
     settype($key, 'array');

     foreach ($key as $value)
     {
      if (array_key_exists($value, $array) === false)
      {
       return $default;
      }

      $array = $array[$value];
     }

     return $array;
    }

    return $default;
}

Let's say you've the following arrays:

$arr1 = array
(
    'xyz' => 'value'
);

$arr2 = array
(
    'x' => array
    (
     'y' => array
     (
      'z' => 'value',
     ),
    ),
);

How do you get the "value" out of the arrays? Simple:

Value($arr1, 'xyz', 'returns this if the index does not exist');
Value($arr2, array('x', 'y', 'z'), 'returns this if the index does not exist');

We already have uni and multi-dimensional arrays covered, what else can we possibly do?


Take the following piece of code for instance:

$url = 'http://stackoverflow.com/questions/1960509';
$domain = parse_url($url);

if (is_array($domain) === true)
{
    if (array_key_exists('host', $domain) === true)
    {
     $domain = $domain['host'];
    }

    else
    {
     $domain = 'N/A';
    }
}

else
{
    $domain = 'N/A';
}

Pretty boring isn't it? Here is another approach using the Value() function:

$url = 'http://stackoverflow.com/questions/1960509';
$domain = Value(parse_url($url), 'host', 'N/A');

As an additional example, take the RealIP() function for a test:

$ip = Value($_SERVER, 'HTTP_CLIENT_IP', Value($_SERVER, 'HTTP_X_FORWARDED_FOR', Value($_SERVER, 'REMOTE_ADDR')));

Neat, huh? ;)

Alix Axel
"Relying on hack functions that may change their behavior in the future"?! Sorry, but that's about the most ridiculous thing I've heard all week. First of all, `isset` and `empty` are *language constructs*, not functions. Secondly, if **any** core library functions/language constructs change their behavior, you may or may not be screwed. What if `array_key_exists` changes its behavior? The answer is it won't, as long as you're using it as documented. And `isset` is documented to be used exactly so. Worst case functions are deprecated over a major release version or two. NIH syndrome is bad!
deceze
I'm sorry deceze, but first of all **hack** is in **italics** in case you didn't noticed. =) Secondly, you mean that one shouldn't rely on `array_key_exists()` to check if a **key exists** in an **array**?! `array_key_exists()` was **created exactly for that**, I rather rely on it for this purpose than `isset()` and specially `empty()` whose official description is: "determine whether a variable is empty", doesn't mentions anything if it actually exists. Your comment and down-vote is one of the most ridiculous I've witnessed all **month**.
Alix Axel
Also, take `$arr = array('xyz' => null)`; `isset($arr['xyz'])`? **Oops!** `empty($arr['xyz'])`? **Oops!**, `array_key_exists('xyz', $arr)` **Finally**, it works **like it should**.
Alix Axel
I'm saying `isset` and `empty` are no more or less reliable than `array_key_exists` and can do the exact same job. Your second, long-winded example can be written as `$domain = isset($domain['host']) ? $domain['host'] : 'N/A';` with just core language features, no extra function calls or declarations necessary (note that I do not necessarily advocate use of the ternary operator though ;o)). For ordinary scalar variables you'll still need to use `isset` or `empty`, and you can use them for arrays the exact same way. "Reliability" is a bad reason for not doing so.
deceze
Re: `isset` and `null`: In the limited case where you're just interested in the array key being set, go ahead and use `array_key_exists`. In 90%+ of all cases I dare say you're usually interested in the value not equalling `false` though, and `isset` is shorter in these cases. Also note that you'll need to take the extra step of confirming that the array is actually an array before you use `array_key_exists` on it, which makes it way more complicated for most cases.
deceze
@deceze: My answer was aimed at dealing with array indexes, not scalar values. I also think reliability isn't such a bad reason to be worried about (hell, this whole question is about reliability): `empty(0)` didn't become deprecated "over a major release version or two", it simple stopped working the way it did before in PHP 4. Same goes for `empty(new stdClass())` in PHP 5.
Alix Axel
You made your point, although I don't agree with most of the stuff you said. I think you got it way wrong on the 90%+ cases, for instance I use the value of "0" in hidden fields in forms all the time. Still I believe the solution I provided doesn't deserve the down-vote **and may well be of some use** to Pekka.
Alix Axel
If you're writing `empty(0)` you have bigger problems, and the behavior of `$x = 0; empty($x);` did not change AFAIA. `empty` on "empty" objects is another topic, but then again loads of things changed between PHP 4 and 5 with regard to objects. Furthermore the question targeted `isset`, `empty` and `array_key_exists`, not array indexes. My point being, creating long custom functions to deal with simple issues that are already solved in the language (if you'd take the time to understand how) is bad practice.
deceze
You seem to take it way too personally when I skip the basics, I know that `empty` is a language construct (irrelevant to this matter BTW) and therefore cannot accept `0`, `new stdClass()` or any other value other than a variable for that matter, I'm sorry if I assumed you would figure that out by yourself for the sake of the comment length. If you're not aware of the changes implied you should research (http://google.com/search?q=empty(0)) a little before you affirm that "isset and empty are no more or less reliable than array_key_exists".
Alix Axel
"creating functions to deal with issues that are solved": I can't access an index of an array returned by a function directly. I can't return a default value if any of the indexes doesn't exist in an array, or if the variable isn't an array, etc...
Alix Axel
"the question targeted isset, empty and array_key_exists, not array indexes" - I never said I had a one size fits all solution, what I said before was that "my answer was aimed at dealing with array indexes" mostly because I saw Pekka comment on BalusC similar answer and it seemed useful to him.
Alix Axel
I'm sorry if I'm trying to be very precise here, which I understood to be the important part of this exchange. `$x = 0; empty($x);` always worked the same, what changed was `$x = "0"; empty($x);`, and it changed between PHP 3 and 4, not 4 and 5. Sorry if the downvote bothers you, but I'm calling it as I see it: Long custom functions for basics like variable handling is bad practice.
deceze
**A word to the wise is sufficient.** The down-vote doesn't bothers me a bit, what bothers me is your stubbornness... Both `$x = 0; empty($x);` and `$x = '0'; empty($x);` changed their behavior in PHP 4: "stopped working the way it did before in **a comma is missing - OHH!!** PHP 4", otherwise (**IF I meant from PHP 4 to 5**) I would've joined both (I'm explicitly referring to the "`empty(new stdClass())` in PHP 5") statements. Again, pretty obvious IMO.
Alix Axel
While @deceze has a point with the custom functions - I usually take the same stance - the value() approach looks interesting enough that I will be taking a look at it. I think the answer and the followup will enable everybody who stumbles upon it later to make up their own mind. +1.
Pekka
A: 

software does not magically run by the grace of god, if you are expecting something that is missing you need to properly handle it. if you ignore it you are probably creating security holes in your applications. on static languages accessing a non-defined variable is just not possible, it won't simply compile or crash your application if it's null. furthermore makes your application unmaintainable, and you are going to go mad when unexpected things happen. language strictness is a must and php, by design, is wrong in so many aspects. it will make you a bad programmer if you are not aware.

knoopx
I am well aware of the shortfalls of PHP. As I pointed out in the question, I am talking about the overhauling of older projects.
Pekka
+4  A: 

I believe one of the best ways of coping with this problem is by accessing values of GET and POST (COOKIE, SESSION, etc) arrays through a class. Create a class for each of those arrays and declare __get and __set methods (overloading). __get method accepts one argument which will be the name of a value. This method should check this value in the corresponding global array either using isset() or empty() and return the value if it exists or null (or some other default value) otherwise. After that you can confidently access array values in this manner: $POST->username and do any validation if needed without using any isset()s or empty()s. If username does not exist in the corresponding global array then null will be returned, so no warnings or notices will be generated.

presario
This is a great idea, and something I'm ready to restructure code for. +1
Pekka
Unfortunately you won't be able to make those instances superglobal unless you assign them to $_GET or $_POST which would be pretty ugly.But you could use static classes of course...
ThiefMaster