What are the worst practices found in PHP code?
Some examples:
Note: maybe subjective or "fun" if you like.
Overuse of globally scoped variables in general. And, erm... using PHP ;-)
HTML and PHP and SQL interspersed without any discipline, resulting in an unmaintainable mess.
HTML + PHP + SQL all nested and messed up together.
Add in a bunch of global functions relying on global variables, and you've got a right mess.
And this was just in the first 30 lines of the index.php file of a major open source project that shall remain nameless...
Setting the error reporting to 0 - error_reporting(0);
Good in production mode - very bad in development mode.
Setting up a system that uses an id field from $_GET to access a particular data record, then not checking group-permissions when the user just starts incrementing the id in the url. Very bad when the page displays information like SS#'s, addresses, etc.
ex: user_record.php?user_id=1
Either obfuscate the user_id in the url with some kind of salted hash, or check that the user has permission to be viewing the page. Better yet, do both.
Programmers thinking that they are 'doing object orientation', when they are actually creating large collections of functions and using hardly any useful OOP practices.
Web designers thinking that because it is easy to learn PHP syntax that it must be easy to learn to code. PHP is just... a more powerful CSS, right?
They're fans of using Dreamweaver to manage their PHP code, which often means having the same code pasted at the top of every single page on the site. Includes, abstraction, DRY, generalization are all beyond their programming "knowledge."
register_globals sibling in evil extract($_REQUEST)
magic_quotes_*
DIRECTORY_SEPARATOR when opening files ('/' is usable on all systems).
require_once(SF_ROOT_DIR.DIRECTORY_SEPARATOR.'apps'.DIRECTORY_SEPARATOR.SF_APP.DIRECTORY_SEPARATOR.'config'.DIRECTORY_SEPARATOR.'config.php');
Yeah.. that's readable.
Bad use of the magic method __call(); resulting is some ugly things like the automagically creation of the setter and getter methods via reflection.
By far the worst practice I have ever seen is suggesting that eval() should ever be used under any circumstance what-so-ever. eval() is evil. Never ever use eval(). If you feel like you have no other choice, there's a 99.99% chance you've done something horribly, horribly wrong.
There are also particularly egregious examples of system function misuse (backtick operators, system(), passthru())
@jcoby, I have to disagree with you on DIRECTORY_SEPARATOR. It's a matter of personal preference. I personally prefer the constant as it is more precise. If paths are exported to logs or other displays, I know they are valid without the need for paranoid transformations and can be used by out-of-band scripts. As for readability, the example you gave could easily have been rewritten as:
// Load configuration settings.
require_once(implode(DIRECTORY_SEPARATOR, array(
SF_ROOT_DIR,
'apps',
SF_APP,
'config',
'config.php'
));
Which is readable and arguably easier to modify than scanning through a string. I'm not suggesting that using the constant is a better practice, I'm just suggesting that it's a matter of personal preference and consistency should be followed whatever the choice may be.
"Enterprise error suppression" ;-)
if (some_condition) {
return false;
error_log('some error message');
}
Take a look at the design of CakePHP. It uses classes without being object-oriented. It duplicates classes within an app layout. It defines methods that take input that doesn't match the method prototype at all. It dumps things in to $this->params, which is a more complicated version of $_REQUEST.
It takes everything that is convenient, wonderful, and fast about PHP and makes it complicated, disturbing, and slow.
Trying to emulate the strenghts of other languages while forgetting about the elegant simplicity that makes PHP powerful. And overengineering frameworks.
I've come across
if ($condition) {}
else {
// something
}
and
if ($condition) {
$var = 'foo';
} else {
$var = 'foo';
}
And the following is actually code written by someone who took Hungarian Notation to the extreme. These are just snippets from a class for manipulating a DOMXML object.
function getAttributes(&$a_oNode)
{
$l_aAttributes = $a_oNode->attributes();
if ($l_aAttributes === NULL)
return array();
$l_iCount = count($l_aAttributes);
$l_i = 0;
for ($l_i = 0; $l_i < $l_iCount; $l_i++)
{
$l_aReturn[$l_aAttributes[$l_i]->name()] = $l_aAttributes[$l_i]->value();
}
return $l_aReturn;
}
function getText(&$a_oNode)
{
$l_aChildren = $a_oNode->child_nodes();
if ($l_aChildren === NULL)
return NULL;
$l_szReturn = "";
$l_iCount = count($l_aChildren);
for ($l_i = 0; $l_i < $l_iCount; $l_i++)
{
if ($l_aChildren[$l_i]->node_type() == XML_TEXT_NODE)
$l_szReturn .= $l_aChildren[$l_i]->node_value();
}
if (strlen($l_szReturn) > 0)
return $l_szReturn;
return NULL;
}
Another really bad issue that I have to deal with is the complete lack of consistent indentation, and also inconsistent use of braces, such that you get code like this:
if ($condition)
foo();
Which lends itself open to a bug because when it's written without indentation, and foo() is inline with the 'if', another developer could miss the condition and add a bar() before foo(), not realising that they've completely changed the flow of logic (it has happened...).
Not validating input.
mysql_query('SELECT * FROM users WHERE user = '.$_POST['user']);
Or:
$_REQUEST['anything']
How often are you not going to know what kind of request you're getting? If anyone has a legitimate use case, please share.
Yeah, found a new one! The future "\ as namespace separator". The irc discussion show how stupid the PHP core developper are. They don't just have a clue about what context sensitive operators are.
Damn, that's a shame such an horrible language gains some popularity!
One company I worked for had written their own shopping cart; on the checkout page the purchase total was calculated and saved to a hidden form field... When you submitted your order it conducted the transaction with the price from that hidden field...
I recently rewrote some code written by an employee who obviously didn't know what he was doing.. I ran into a few gems like this..
$sql = "SELECT * FROM attendance_exceptions";
$found = false;
...
$appendSql = " AND YEAR(event_time) = '$_POST['year']' AND MONTH(event_time) = '$_POST['month']' AND DAY(event_time) = '$_POST['day']'";
$temp = array($sql, $appendSql);
$sql = join($temp);
$found=true;
Creating an array, then join()ing it simply to concatenate a string? I won't get started on the query itself, nor the unsanitized $_POST data.
function verifyQuery($sql, $con) {
if (!mysql_query($sql, $con)) {
echo "Error occured in verifyQuery() in sqlfunctions.php <br>";
echo "SQL sent : ";
echo $sql;
echo "<br>Database report: <br>";
die('Error: ' . mysql_error());
}
return mysql_query($sql);
}
Executing mysql_query twice?
Sometimes I wonder if the guy was just trying to make his code look more difficult.
The use of short tags <? ?> rather than <?php ?> because other languages also use the <? syntax.
Edit: As of PHP 5.3, short tags will throw an error.
Reimplementing the native functions because one did not bother to check the documentation first.
my favourite so far is in some code I was given when they had skipped the idea of Mod_rewrite and instead used the 404 page to return "200 OK" and then process all 404 urls (which were being used as real pages) into parts of a query and build the page from that
$url_parts = explode("/",ereg_replace("(.php|.html|.htm)", "", $_SERVER['PHP_SELF']));
This is just wrong on so many levels.
I know this is an old question, but what I perceive as the worst practice hasn't been posted yet.
The worst practice in PHP is having the language's behavior change based on a settings file. Particularly the settings you can't alter at runtime. It severely affects portability.
PHP6 will be great if not just because it removes the magic quotes settings.
As a side note, PHP5 has two ini files. From memory, here are some of the differences between them:
php.ini-dist
php.ini-recommended
There is a lot of things which can be done badly in PHP. However, I find it very bad when people do not use functions, classes, mix php and html, use echo for creating pages and still try to support pre php5 versions.
There are a few. And they usually come together :)
They are not necessarily php-specific, however those practices tend to show up more frequently in this language.
These two are not PHP specifics, but please please please please:
$apáñalo are not funny (and I've even seen wrong behaviours using them)$what_a_long_variable is not descriptive, and people's mood won't get highWhere to begin, the use of variables that are not defined unless the if(something) is triggered, thus producing lots of errors in the error log.
The use of an array value that does not exists unless it was created prior in if(something), thus spawning more errors.
Not closing a persistent connection to MySQL for a single query in the page...
Including files that have no purpose in the script other than to say give more errors with variables that are not defined.
Running multiple queries to insert data into a table that could have been done in 1 line.
Templating engines. Why implement templating languages in a templating language?
This article says it better than I can.
Hmm, what to say, most PHP projects I've ever seen are just plain horrible. I can't remember much, here goes:
// WARNING: Don't try this at home!
mysql_query("SELECT * FROM tblSomething WHERE id = " . $_GET['x'] . " AND tblSomethingElse = " . $someval ) or die(mysql_error());
(note, there is no return in this, SO wrapped it)
die(mysql_error()) yay! let's spam the user with something that is completely useless to him instead of a standard not found page"foo" . $bar . "baz" ugly as...$_GET['x'] Unsanitized, however I blame PHP for not forcing you to use parameterized queries.or die() wtf, maybe for a 10-line project it's okay to die in the middle of nowhereIf you're going to do something insecure, at least make the code look nice..
mysql_query("SELECT * FROM tblSomething WHERE id = {$_GET['x']}".
"AND tblSomethingElse = $someval") or die(mysql_error());
What seems to be an idiom in php:
include($_GET['x']);
This is just plain bad security-wise, and bad programming style anyways.
Writing 500 function calls with the @ error suppressor.
Global variables galore. Setting $_GET['x'] = 123, then calling a function with no parameters that uses $_GET['x'] which also calls another function with no parameters that uses $_GET['x'].
People calling sprintf where they could have just used string interpolation.. uggh.
This:
<?php
if ($_GET['ad'] == 'ud') {
include('includes/ud.inc.php');
} else if ($_GET['ad'] == 'dd') {
include('includes/dd.inc.php');
} else if ($_GET['ad'] == 'wd') {
include('includes/wd.inc.php');
} else if ($_GET['ad'] == 'fd') {
include('includes/fd.inc.php');
} else if ($_GET['ad'] == 'cd') {
include('includes/cd.inc.php');
} else if ($_GET['ad'] == 'od') {
include('includes/od.inc.php');
} else if ($_GET['ad'] == 'ld') {
include('includes/ld.inc.php');
} else if ($_GET['ad'] == 'ldu') {
include('includes/ldu.inc.php');
} else if ($_GET['ad'] == 'ldc') {
include('includes/ldc.inc.php');
} else if ($_GET['ad'] == 'lde') {
include('includes/lde.inc.php');
} else if ($_GET['ad'] == 'pd') {
include('includes/pd.inc.php');
} else if ($_GET['ad'] == 'nd') {
include('includes/nd.inc.php');
} else if ($_GET['ad'] == 'cu') {
include('includes/cu.inc.php');
} else if ($_GET['ad'] == 'kg') {
include('includes/kg.inc.php');
} else if ($_GET['ad'] == 'st') {
include('includes/st.inc.php');
} else if ($_GET['ad'] == 'cm') {
include('includes/cm.inc.php');
} else {
include('includes/home.inc.php');
}
?>
And the winner is...
<table cellpadding="4" cellspacing="0" border="0">
<?php do { ?>
<?php if ($userDetails['name'] == 'adLinkTitle') { ?>
<tr>
<td>
<label for="wTitle">Your Website Title:</label>
</td>
<td>
<input name="wd_title" type="text" id="wTitle" class="adminInput" value="<?php echo $userDetails['value'];?>" />
</td>
</tr>
<?php } ?>
<?php if ($userDetails['name'] == 'adLinkURL') { ?>
<tr>
<td>
<label for="wAddress">Your Backlink URL:</label>
</td>
<td>
<input name="wd_url" type="text" id="wAddress" class="adminInput" value="<?php echo $userDetails['value'];?>" />
</td>
</tr>
<?php } ?>
<?php if ($userDetails['name'] == 'adLinkDescription') { ?>
<tr>
<td>
<label for="wAddress">Your Website Description:</label>
</td>
<td>
<textarea name="wd_description" id="wDescription" class="adminInput" rows="6"><?php echo $userDetails['value'];?></textarea>
</td>
</tr>
<?php } ?>
<?php } while ($userDetails = mysql_fetch_assoc($getUserDetails)); ?>
</table>
<input type="hidden" name="wd_updated" value="yes" />
<input value="Update Your Link Details" type="submit" class="padSubmit" />
</form>
<br />
Okay, the answers here seem pretty tame compared to my experience. I once worked on a project where the author had avoided using arrays by using eval instead. Example:
$qty = eval("\$qty_" . $product)
$cost = eval("\$cost_" . $product)
This was to access variables that should have been accessed through $POST or whatever.
How about this (snipped from http://thedailywtf.com/ and modified a bit) :
<?php
function get_signature($signature) {
return $signature;
}
function get_date($date) {
return $date;
}
<?
And,
if (!false) {...
The over usage of duck-tape, er I mean arrays. I'm horribly guilty of this though because PHPs arrays are so powerful!
things like this
myobject=array("field1" => 10, "field2" => "abc");
and yes. I do this because I still think PHPs OOP implementation is half-baked. I haven't tried using it in so long though that I'm probably wrong now though
I know this might even sound too wicked to be real, but the worst piece of "software" I've ever had the chance (or disgrace) to put my hands on was a single nK line php file which, I remember, was handling the whole logic for an e-commerce site and was structured like this (including tons of echoed HTML, obviously not properly indented):
<?php
// Obviosly relying on register globals!
switch($page) {
case cart:
echo '<html><head>...</head><body>'.
some random application logic, including SQL, of course! .
'</body></html>';
break;
case checkout:
echo '<html><head>...</head><body>'.
even more random application logic, including SQL, of course! .
'<form><input type="hidden" name="total" value="X"></body></html>';
break;
case complete:
echo '<html><head>...</head><body>'.
and again, some random application logic, including SQL, of course! .
'</body></html>';
break;
case error:
echo '<html><head>...</head><body>'.
some random application logic, including SQL, of course! .
'</body></html>';
break;
// Show the home page
default:
echo '<html><head>...</head><body>'.
show the home page, if you really don't know what else to do.....
'</body></html>';
break;
?>
A nightmare, believe me!!! ...and thus the worst coding practice I could think of... If I think about it now, of course it makes me smile though :-)
One programmer in our office did this to add space:
<?php echo ' '?>
I was dumbstruck!
Walking past one of the 'developers' desks and seeing this code.
for ($i=1; $i <= 5; $i++) {
echo '<br>';
}
shudders