What are the worst practices found in PHP code?
Some examples:
Note: maybe subjective or "fun" if you like.
Using and relying on register_globals
. Yuck.
extract
on the array passed to it. That way, the template was loaded into a context containing simple variables rather than having to reference them from the array. Isn't this fine usage? - Carson Myers
extract($_REQUEST); if($l1=='homepage'){include_once('home.php');}
Is it something wrong with it? - Kemal
extract($_REQUEST); ... if (is_valid_user()) $authorized = true; if ($authorized) risky_business();
- Joeri Sebrechts
using @ everywhere because they're too lazy to use isset()
isset
s. :) - johndodo
How about Magic Quotes [1]? So evil they've been removed from PHP6.
[1] http://www.php.net/magic_quotesOne 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...
HTML and PHP and SQL interspersed without any discipline, resulting in an unmaintainable mess.
Programmers thinking that they are 'doing object orientation', when they are actually creating large collections of functions and using hardly any useful OOP practices.
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...
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."
Reimplementing the native functions because one did not bother to check the documentation first.
spinny_go_round_forward_circle_chr1024_rev
) - Northborn Design
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.
Spoted this little snippet on a forum I am a member of, made me unhappy:
$str = file_get_contents("db-info.php");
$str = str_replace("<?", "", $str);
$str = str_replace("?>", "", $str);
eval($str);
"Enterprise error suppression" ;-)
if (some_condition) {
return false;
error_log('some error message');
}
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.
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
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.
renaming plain .html files to .php just to look "cool"
The use of short tags <? ?>
rather than <?php ?>
because other languages also use the <?
syntax.
E_STRICT
only? - alex
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.
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...).
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.
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!
Overuse of globally scoped variables in general. And, erm... using PHP ;-)
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.
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.
Bad use of the magic method __call(); resulting is some ugly things like the automagically creation of the setter and getter methods via reflection.
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.
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.
Walking past one of the 'developers' desks and seeing this code.
for ($i=1; $i <= 5; $i++) {
echo '<br>';
}
shudders
Yeah, found a new one! The future " \ as namespace separator [1]". The irc discussion [2] 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!
[1] http://news.php.net/php.internals/41374Hmm, 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 />
There's quite a nasty little gotcha in the Zend Framework in the code that generates checkboxes.
It's not mentioned anywhere in the documentation, but if you use Zend_Form_Element_Checkbox to insert a checkbox into a form, it will also insert a hidden field with the same name, but a different value.
I don't know about you, but I'm used to the PHP behaviour with checkboxes, where the checkbox value is included in the $_GET or $_POST if it was checked, but no evidence of the checkbox exists in the request data if it wasn't. As this is the behaviour I'm used to, I do
if (isset ($_POST ['mycheckbox'])) {}
all over the place. In fact it's an easy way to validate checkboxes, as you don't have to worry about their value, only if they're set or not.
$validatedPost ['myCheckbox'] = isset ($_POST ['myCheckbox']);
Now try doing that if you're working with Zend forms and using checkbox element objects. Because there's a hidden field with the same name as the visible checkbox, the posted data (I'm just going to use $_POST from now on) always contains a value for myCheckbox. If your code was checking for the presence or absence of myCheckbox then your code will now not function properly anymore.
One developer I know who had gotten a little overdependent on the framework ran into this issue and ended up wasing half a day of development time trying to figure out what happened. When he finally did twig the air turned rather blue, I can tell you.
You might say that if you use Zend you should be aware of such things, and you might have a point. However, it a) violates the Principle of Least Surprise, and b) isn't properly documented. By trying to be helpful, the Zend framework actually makes checkboxes harder to work with for people who are used to them not existing if they're not set. The ZF guys should have known better.
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.
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.
Templating engines. Why implement templating languages in a templating language?
This article [1] says it better than I can.
[1] http://www.massassi.com/php/articles/template%5Fengines/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) {...
What about using the e
flag in preg_replace_all
and the like?
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
The common adoption of generalised front controller MVC frameworks. Do you really think that this helps if you need to scale unique requests ?
RewriteCond %{REQUEST_FILENAME} !-f
RewriteCond %{REQUEST_FILENAME} !-d
RewriteRule ^(.*)$ index.php [L]
use
huh? nice. - jcoby
Setting the error reporting to 0 - error_reporting(0);
Good in production mode - very bad in development mode.