Forum Moderators: coopster

Message Too Old, No Replies

php coding standard query

         

johnblack

7:14 pm on Jan 29, 2011 (gmt 0)



In a free script I use, when it runs on my hosting no errors are displayed, but when it runs on my local 'test' machine I get Undefined index errors on $_GET and $_SESSION variable because I don't check them using isset()

My question is what is the 'correct' php standard? Should all these types of variables be checked first or is it OK to hide the errors?

The 'clean and elegant' programmer in me says, no check them all, it's the only way you can be sure your code will work.

Whilst the 'quick and dirty' coder whispers in my other ear, nah, don't worry, she'll be right mate.

Just wanted to pick the php wisdom on this forum as to the best practice.

Pointing me in the direction of a good php standards resource would be cool too!

rocknbil

9:32 pm on Jan 30, 2011 (gmt 0)

WebmasterWorld Senior Member 10+ Year Member



LOL . . . love the logic and your way of expressing the two forces of PHP, it's like Luke and Darth. :-)

Here is how I justify this: when you check data first, always error check values, it may seem like more work but in the long run, makes your job easier especially when your apps begin to grow. If you do

do_something() or error("output error, something useful to you")

You may never see the error. It seems to most like wasted effort, code bloat, not needed. But if you don't and you just get a blank page, or if it all seems like it's working but nothing is getting entered into a database, or the wrong values enter, or any number of unexpected results, you will wish you had.

So I'm in the first camp, not out of elitism or snobby or expertise, but because I've learned this the hard way over many years. The basic framework for any good app will always be something like this.

$error = check_any_input();
if ($error) { output_error($error); } // which exits
// get here, input is OK
$error = do_something();
if ($error) { output_error($error); }
output_response();

output_error can be as simple as

output_error($err) {
echo "An error has occurred: $err";
exit;
}

To answer directly, yes, too often I see

$somevar = $_SESSION['blah'];
if (! $somevar) { error(); }

Which will still give you the error because youre setting $somevar without checking the session - additionally, if error output is disabled (which it should be in live environments,) the program may go on and give you unexpected results. Why? I see a lot of this.

if ($somevar=='') { error(); }

Which it's not (blank string.) It's null;

So I'd definitely check not only for the variable but it's expected value/data type as well. In this example, say, a login name allowing only letters and numbers, case insensitive.

if (isset($_SESSION['somevar']) and preg_match('/^[a-z\d]+$/i',$_SESSION['somevar'])) {
do_something($_SESSION['somevar']);
}
else { error("Some var was not set or is the wrong data type"); }

Sometimes it's not an error, for example, if the previous is a login routine you'd send them back to a login screen instead.

johnblack

10:54 pm on Jan 30, 2011 (gmt 0)



Thanks for the feedback rocknbil. Glad you liked the dark and light sides of php!

The session variable I was looking at is an internal value like an admin flag. The script effectively just checks to see if it's 'Y' to see if you are an admin user. Any other value, including null I guess, it assumes you are not.

Not app breaking if null, just on the local version I see error messages. to say it's not set.

I guess it's up to me to decide whether a particular session variable is critical and should always be set with a non-null value or not.

Something I was thinking about was creating a class which exposes the session variables through its properties and forces them to have 'values'. I.e. all the isset code is hidden inside the class and the properties can never be null.

Is that kind of thing worth the effort? Dark and light again!