Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.
Popular code excerpts may be moved to "Code Snippets" by the moderators.
I cobled together a simple user validation/login script using a mySQL database and PHP sessions. I seem to work OK but I'm not sure that it is as secure/safe or fast as it can be. It seems a bit slower in Firefox than IE. Comments welcome.
I am using multiple values of the $_REQUEST array.
I am also scrubbing all global values. I'm not 100% sure what all this does but I found it in a best practises presentation.
$_GET = array_map('mysql_real_escape_string', $_GET);
$_GET = array_map('stripslashes', $_GET);
$_GET = array_map('mysql_real_escape_string', $_GET);
Also what constants should I use for error_reporting() to flush out as many errors and warnings as possible?
Is there any risk in leaving error_reporting() in a production server?
I think the two thing that stand out for me are that the code is not well structured and you do not filter input properly. What kind of comments were you looking for?
I am using multiple values of the $_REQUEST array.
I am also scrubbing all global values. I'm not 100% sure what all this does but I found it in a best practises presentation.
$_GET = array_map('mysql_real_escape_string', $_GET);
$_GET = array_map('stripslashes', $_GET);
$_GET = array_map('mysql_real_escape_string', $_GET);
Also what constants should I use for error_reporting() to flush out as many errors and warnings as possible?
Is there any risk in leaving error_reporting() in a production server?
The problem is that you are running everything through mysql_real_escape_string no matter what it is or where the variable will be used. Sometimes you don't want to escape stuff if you are going to use it in something other than a query.
Also, what if an array gets passed in the post? Your script will bomb. This is what feyd is pointing out.
As for error_reporting, take a look at the comments in php.ini or the documentation on php.net. For development you want to turn on pretty much everything including notices, for a production server you generally do not want any errors/warnings at all.
These are exactly the type of comments that I was looking for. I want to write code
that more than just works. I want well structured, maintainable, scalable
and secure code as well. These things that are not always obvious at first pass.
I'd have all your variables that you set at the top be boolean FALSE if the required variable is not found. It seems a bit cleaner, both code-wise and semantically. By doing so, your first if condition can be changed to:
Why do you have 2 echo statements one right after the other in fnGelogin(). heredocs can interpret variables, so you can put $message in your heredocs. Strictly speaking, fnGetlogin() should be named fnGetLogin() to be proper camel case.
Include2.php: is it just for this application? If so, what are $LF, $quote, $q, $dq, $comma for? They're not used anywhere. Also, you connect to a database in the include2 file, as well as in your main file.
If you want to keep the db connection in include2.php, you can get rid of the superfluous if...die condition.
~feyd is correct in that your assumption that $_GET, $_POST, and $_COOKIE are one dimensional. I also personally consider it bad form to overwrite super globals. I treat superglobals as unmodifiable & escape them as necessary.
Real programmers don't comment their code. If it was hard to write, it should be hard to understand.