share
Stack OverflowWorst PHP practice found in your experience?
[+63] [44] Joshi Spawnbrood
[2008-10-24 10:37:13]
[ php ]
[ http://stackoverflow.com/questions/233030/worst-php-practice-found-in-your-experience ] [DELETED]

What are the worst practices found in PHP code?

Some examples:

Note: maybe subjective or "fun" if you like.

(6) The answers aren't as helpful unless they state why they're a bad practice. - jgreep
This should be community wiki. - Michael Damatov
(1) the question is ok, there are worst practices in every language. the answers are more like 'language war'. vote to close. - markus
(2) The worst practice of PHP has nothing to do with code, it's the low cost of entry into developing with it. This is the core source of every problem written in that language has. - marr75
+1 @tharkun I would vote if I had the power :) - zaf
Just be careful: many of the headaches of PHP that non-PHP programmers complain about aren't actually problems in the language. They're just problems in perception. - staticsan
[+85] [2008-10-24 10:43:56] Oli [ACCEPTED]

Using and relying on register_globals. Yuck.


Very True ( how could i forgot? ) - Joshi Spawnbrood
(1) That will come to an abrupt and ignominious with the release of 6.0. - Lucas Oman
Only if extract is removed from the core library. - jmucchiello
(1) All the examples in one of the books I have need register_globals turned on -- as a PHP newbie, it wasn't exactly obvious. (It was a Linux box with StartCom Enterprise Linux 5 on it as the server... good times!) - Lucas Jones
(5) Ahh, register_globals, possibly the biggest reason people will not upgrade to PHP 6. Getting people to upgrade from PHP 4 to PHP 5 was a huge effort already, but getting them onto PHP 6... That will take ages. Anyway, if there was ever justification for creating a time machine, this is it. - Michael Stum
1
[+55] [2008-10-24 11:19:04] Joeri Sebrechts
  • Using the dot operator to build query strings from variables, without any form of escaping or variable binding. This still happens a lot.
  • Using extract() liberally to have easy access to variables. We got rid of register_globals only to see it replaced with extract($_POST).

oh boy, extract is a nightmare. - bbxbby
(6) Extract is almost as dangerous as eval() - Lucas Oman
I'm not entirely certain that I understand the dot operator thing, unless you mean a lack of database sanitization, but its not clear from the statement. - Nicholas Flynt
I meant concatenating variables into strings to build queries. Most people writing PHP scripts don't know the meaning of the term "database sanitization". - Joeri Sebrechts
+1 to extract, but it can be useful in combination with EXTR_IF_EXISTS - Ken
(7) wtf? extract is awesome when used properly. Using it for $_POST is just damn sick, but you can't blame extract as such. - dr Hannibal Lecter
(7) I used extract once in a function that took an associative array. The function used output buffering and included a template file, after calling 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
(7) Extract is PHP's equivalent of pointer arithmetic in C. It's quite handy if you know what you're doing, it's a disaster if you don't. - Joeri Sebrechts
Is there a problem with extract if I use it to check whether a desired variable is set to a desired value? For example I want to run script if l1 variable in URL set to homepage. extract($_REQUEST); if($l1=='homepage'){include_once('home.php');} Is it something wrong with it? - Kemal
(1) @xy_: if you use this code you could get into problems: extract($_REQUEST); ... if (is_valid_user()) $authorized = true; if ($authorized) risky_business(); - Joeri Sebrechts
2
[+52] [2008-10-24 11:47:06] Lewis

using @ everywhere because they're too lazy to use isset()


Or just ignoring all the warnings caused by undefined array indexes. This makes being the only guy who actually has all E_ALL on and looks at the log file painful. Though this will be mitigated when we switch to 5.3 - therefromhere
Can't say I agree with this. One of the nicest things about PHP is that you CAN be lazy. Checking for every array index is just stupid: "Yes, I know it might not be set. Just set it to some value because I don't care" - this is what PHP did before it tried to be another Java. </rant> - johndodo
(2) @johndodo, but if you are coding to be lazy, you would just turn PHP notices off - since they are more of a good practise thing. They also has an effect on performance, not to mention could hide other, more serious, bugs. seanmonstar.com/post/909029460/… - Lewis
@Lewis: +1 and a thank you for the link. Interesting find, I didn't know about performance penalties. However the "good practise things" have a habit of turning into requirements so I try to code with notices enabled and disable them when shipping. But it is annoying to have to use all those issets. :) - johndodo
3
[+51] [2008-10-24 10:47:18] Greg

How about Magic Quotes [1]? So evil they've been removed from PHP6.

[1] http://www.php.net/magic_quotes

(1) Second that. Thank GOD magic quotes are gone. - Nicholas Flynt
(53) Yes, it was like a seatbelt...fastened around the neck. - micahwittman
Actually, I stopped using php because of rubbish like magic_quotes, "safe" mode, etc. No second chances. - L̲̳o̲̳̳n̲̳̳g̲̳̳p̲̳o̲̳̳k̲̳̳e̲̳̳
@Longpoke Safe mode will be gone too as of PHP6. Finally no more extremely tied down Shared Hosting.. ugh. (I got a VPS I was so tired of Safe Mode limitations) - Earlz
4
[+45] [2008-10-26 08:03:43] Kevin

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...


(1) Isn't this a fiddler example? Tell me you're kidding about this being in the wild... - marr75
(8) OMFG!!!!! No!!! - Sepehr Lajevardi
(4) :) That's so scary, it makes me laugh... - Irfy
(11) can you post the url of the store? >:) - nimcap
(10) So what if you set it to a negative number.... >;) - Gordon Gustafson
What if you didn't give it a number? - Pacerier
5
[+40] [2008-10-24 11:05:39] Tom Haigh

HTML and PHP and SQL interspersed without any discipline, resulting in an unmaintainable mess.


that's not really specific for PHP.. But is is a really bad thing - Jacco
Maybe not, but it is the classic example that you find causing php projects that are impossible to maintain - Tom Haigh
(4) I tend to "mix" a lot, but I do all my logic and calculation before lines of HTML. That HTML may include lots of PHP to control formatting based on pre-calculated logic, but I'm generally good about keeping it separated. - Nicholas Flynt
(2) You can mix php with HTML if you do it with care. Only ifs, foreach's and echo's. There is no need for yet another language like smarty when PHP itself was designed as a templating language. Just make sure not to do anything unrelated to ouput (like mysql querys or calculations) in the HTML. - Pim Jager
Yes and that's fine, but it is still bad practise to do it badly, which is what I meant - Tom Haigh
Yea, my last project I did I split evenly into two different parts. One is the "templating" where I write out all the HTML code. The other is the "engine" which calls the templating functions to write out HTML and all that. My index.php and postpacks.php files work as the controller. So I made my own simiplistic MVC..ish framework - Earlz
6
[+40] [2008-10-24 13:30:53] Kieran Hall

Programmers thinking that they are 'doing object orientation', when they are actually creating large collections of functions and using hardly any useful OOP practices.


(19) But I use classes!! class Utility { public static function doX(); public static function doY(); } - K. Norbert
(1) To be fair, classes are kinda useful in lieu of namespaces. As long as you know what you're doing of course ;) - DisgruntledGoat
7
[+32] [2008-10-24 11:28:35] madlep

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...


(15) How about HTML+PHP+SQL+JS+CSS, I've seen that... - Dean Rather
(26) Let me guess... WordPress? - Maciej
(1) sounds like pretty much 70% of the php sites out there. - jcoby
(17) Why remain nameless? Bad coders should be shamed into becoming good coders. WORDPRESS SUCKS. - Lucas Oman
Um... I tend to use HTML+PHP+SQL+JS+CSS. The difference is that (even when its all nested together) its not messy. The trick to a project like that is learning how to keep the different languages separated, even when PHP is generating parts of the other languages on the fly. - Nicholas Flynt
(7) I'm all for proper coding practices and whatnot, but from a user perspective, Wordpress is probably the best blogging software out there. At the end of the day, who cares what the code looks like. - nickf
(3) @nickf: I do! Cause I have to read it, fix it, tweak it, use it, etc., etc... - Svish
(That was an In General comment btw, not Wordpress spesific) - Svish
At one point there was an example on php.net of the mysql_get_assoc which did this. I swear I worked on at least 3 sites where every query display was copy pasted from that example. - Nick Van Brunt
8
[+25] [2008-10-24 13:47:03] Lucas Oman

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."


(23) Dreamweaver is the source of most PHP evils. It screws up the code formatting, encourages bad css names, and encourages duplicating the layout everywhere thanks to templates and by making it easy to change multiple files. Makes it hell for the rest of us. - jcoby
(3) I agree. I love PHP, don't get me wrong, but any coding project needs to be handled properly. That said, I actually use Dreamweaver, with a couple of rules: It can't type my code for me, and it doesn't get to touch my CSS. So basically, its a CSS highlighting FTP window thing, but that's all I need. - Nicholas Flynt
(1) oooh time to upgrade Nicholas. There's much better software available for you out there. I started learning PHP from the automatically generated code it gave me many moons ago, but thankfully realised that it wasn't a great system for development not long afterwards. - nickf
...so which do you use? :D - Humphrey Bogart
I'm an avid vim fan. Can't imagine using anything else. - Lucas Oman
9
[+24] [2008-11-06 17:55:34] Krzysztof Sikorski

Reimplementing the native functions because one did not bother to check the documentation first.


(3) This has happened to me before... why did I not know basename() existed? - alex
(20) This is also because the naming strategy in PHP is virtually non-existant. There's no way to safely 'guess' a method, let alone their parameters. And the PHP devs are also not really interested in fixing this, I gathered. - Erik van Brakel
(8) I'm forever checking the docs just to make sure I have the parameters in the right order. There's no standard. For search functions, sometimes haystack comes first, sometimes needle. Gah! - seanmonstar
(3) PHP's hideous library doesn't help, but this is a problem common in all languages. Half the stories from dailywtf are snippets of someone poorly re-implementing library functions. I was working on a project once my senior year when one of my developers looked at me and asked if I could review his function to convert hex user input to ascii in C, I said nothing and we instantiated code reviews from then on. - marr75
(3) Hey, at least php.net has the most rockin API documentation of any API out there practically. (I've yet to see a better source of documentation for other languages/frameworks) - Earlz
(1) @seanmonster, that is because you didn't reimplement the native functions with new names / parameters. - Pacerier
@Pacerier +1,000. Sometimes people confuse re-inventing the wheel, with renaming the wheel (considering it was formerly named spinny_go_round_forward_circle_chr1024_rev) - Northborn Design
10
[+22] [2008-10-24 17:53:51] Jim Puls

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.


hehe well put. It seems these days that frameworks that over-complicate things are all the rage. It seems there's a natural tendency for people to crave something which is too complicated to understand. - thomasrutter
11
[+15] [2008-10-25 21:33:37] PeterV

Trying to emulate the strenghts of other languages while forgetting about the elegant simplicity that makes PHP powerful. And overengineering frameworks.


(4) I wouldn't use word 'elegant' and PHP in the same sentence. I believe the right word is 'pragmatic'. - Kornel
(3) The elegance comes from what you do with it. Well that's what she said. - thomasrutter
12
[+15] [2010-12-31 17:01:24] Jacek

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); 

(2) lol. this one here is hilarious! - maraspin
13
[+14] [2008-10-24 17:47:15] Patryk Kordylewski

"Enterprise error suppression" ;-)

if (some_condition) {
   return false;
   error_log('some error message');
}

Wow, just wow!!! - AntonioCS
That'll be a pretty empty log file yeh? - alex
(5) Behind every one of these there'll be a phone call from the boss asking if you could "fix this error that keeps coming up in the logs" - thomasrutter
14
[+14] [2008-10-26 01:41:30] VirtuosiMedia

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.


(3) I built a REST API and at time the requests were in POST and at other times they were GET so I used $_REQUEST - UnkwnTech
(1) Try two different PHP files. - jmucchiello
This answer was practically bait for someone to jump in and volunteer a time they had done it, and then be told why they were wrong. Can someone start PHP over from scratch? If I wish hard enough can it happen? - marr75
I use $_REQUEST to get error messages and notifications and display them in my templates. That way if you need to notify the user of something you can just pass it whichever way is the easiest. I also check in $_SESSION for the same thing too, which makes it even more transparent. - tj111
No problems with that. Just wrap your $_REQUEST into objects, where input can only ever be accessed via filter functions. So mysql_query("SELECT * WHERE user = $_GET->int[id]"); Only way to enforce validation. - mario
For trivial things like sort=namedesc, sometimes it's handy to allow this either via the query string or a post variable as there might be a few actions you can take on that page. Ok so if @marr75 is right someone's going to come in now and tell me doing that was wrong. ;) - thomasrutter
15
[+13] [2009-05-06 14:01:21] Powerlord

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

  • magic_quotes_gpc = On
  • display_errors = On
  • arg_separator.input = "&"

php.ini-recommended

  • magic_quotes_gpc = Off
  • display_errors = Off
  • arg_separator.input = "&;"

Off the top of my head I can't remember the exact version numbers (4.1.3/4.1.4/4.1.5 something), but there exists at least one instance in which the default setting for magic quotes was changed from on to off, and then back to on again, all in a sequential set of minor version updates. - David
It doesn't help that PHP comes with two different inis: magic quotes gpc is off in dist and on in recommended. There are other minor differences between the two as well. I should probably add these to my answer. - Powerlord
Default error settings fall foul of that, too. I actually filed a bug because the default .ini file couldn't decide if it was intended for a dev or a prod environment (e.g. Notices were suppressed). - staticsan
16
[+12] [2008-10-24 13:08:19] Mike

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.


it gets worse, i've seen not only values, but column names taken directly from the url like that. - Kris
(4) There's been stuff on dailywtf about entire queries in the URL. - chaos
It also sucks when the way to delete a user or whatever from the database is via GET variable :( - AntonioCS
Or: drop the obfuscation, and do the proper way twice! - L̲̳o̲̳̳n̲̳̳g̲̳̳p̲̳o̲̳̳k̲̳̳e̲̳̳
haha @chaos: I've seen sites that base64-encode SQL queries into a GET var and execute it. - L̲̳o̲̳̳n̲̳̳g̲̳̳p̲̳o̲̳̳k̲̳̳e̲̳̳
(1) Why obfuscate? What's so bad about going through news section of a website by incrementing id in the url. saves a lot of time... - Ivan
I did put columns freely searchable, I even made a parser parsing search input to SQL (every parameter is sent seperately, parameterized) - sinni800
@Mike, if the user has permission to view the page, then why obfuscate? - Pacerier
17
[+11] [2008-10-26 09:07:19] daniels

renaming plain .html files to .php just to look "cool"


(2) Or the opposite, adding a php handler for .html files just to hide the fact they use php ;p - Kevin
(3) @Kevin, what's wrong with that? Of course, the mime type has no real place in a good URI but it still beats exposing implementation details by a laaarge margin. So yes, if your server doesn't allow/support something like mod_rewrite, do use PHP handlers for html files instead (+ DirectoryIndex!). - Konrad Rudolph
(1) @Konrad, there's nothing wrong with it besides the fact that PHP has to parse every HTML file called from the server which increases server load. Also, PHP headers usually include "X-Powered-By PHP" which defeats the whole purpose of giving HTML a PHP mimetype handler. - Kevin
@Kevin, that's simply a lousy server configuration problem. Pushing the blame to PHP is like blaming the typewriter for libel. - Pacerier
18
[+11] [2008-10-27 18:58:27] VirtuosiMedia

The use of short tags <? ?> rather than <?php ?> because other languages also use the <? syntax.


(6) I don't really agree with that - Joshi Spawnbrood
(11) What don't you agree with? It can conflict with XML and not all hosts support the use of short tags. It's a deprecated practice. - VirtuosiMedia
(3) I agree20% of the time I see code that someone is having problems with they are using <? how much work is just to type the extra 3 characters - UnkwnTech
(14) It's kind of handy when you're not using something like smarty as a template engine. <?=$var?> is a lot easier than <?php echo $var; ?> - I.devries
(2) Agreed with Unkwntech. Those 3 bytes prevent a number of unnecessary headaches. Short tags solve a problem that doesn't exist. - Legion
@Vatos: Indeed the <?=$var?> is a lot simpler than <?php echo $var?> but I always use the <?php.. ?> because I had problems with short tags in the past and just using <?php instead of <? fixed everything - AntonioCS
Is that edit true? Short tags will throw an error in 5.3? Is this on E_STRICT only? - alex
Just use short tags for development, and a simple oneliner for code distribution with long tags: perl -pie 's/<?=/<?php echo /; s/<?\s/<?php /' $(find templates) - mario
(2) because other languages also use the <? syntax. So let's change these other languages. I'm not ready to give up on <?=$var?> - Ivan
19
[+11] [2009-06-19 15:35:58] ya23

There are a few. And they usually come together :)

  • No functions or few functions 300+ lines long. Classes? Yeah, right...
  • When code is mixed with HTML the indentation is just random.
  • "select count(*) from users where login = '{$_GET['login']}'...
  • Mixing English with other languages within code
  • Copy & Paste methodology - why bother using functions when you can copy the blob of code, tweak it a bit and it all works?

They are not necessarily php-specific, however those practices tend to show up more frequently in this language.


20
[+8] [2008-10-26 01:08:05] JamShady

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...).


(9) This is bad code, and has nothing whatsoever to do with PHP, besides the fact that the bad code happened to be written IN PHP. - MattBelanger
(1) The first example reminds me of something I saw in my boss's code: if ($condition) { $yeah = 'y' } else { ...actual pointful code... }. - thesunneversets
21
[+7] [2008-10-24 15:26:55] Rabbit

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.


(5) eval is not evil. It is evil when you use it with data from the user - AntonioCS
22
[+6] [2010-05-12 00:13:47] maraspin

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 :-)


23
[+6] [2010-05-12 01:15:40] Thorpe Obazee

One programmer in our office did this to add space:

<?php echo '&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;'?>

I was dumbstruck!


Isn't it dumbstruck-en? - Pacerier
24
[+4] [2008-10-24 10:48:14] Dominic Rodger

Overuse of globally scoped variables in general. And, erm... using PHP ;-)


(1) Hey, I wanted to write that. :-) - Tomalak
(7) Agree with overuse of scoped variables. Disagree STRONGLY with the bash of PHP, I don't think I would use anything else if given a choice as the backend for my websites. - Nicholas Flynt
PHP is a procedural language (really) so global variables are less of an evil. Without encapsulation, sometimes you just have to use a global. - jmucchiello
It could be a necessary evil, just not when they are littered through out the whole code base of 500k lines of codes. - fred
25
[+4] [2008-10-26 15:28:11] mikedub

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.


(1) actually, joining arrays is faster than string concatenation in most languages, not sure about PHP, though. - I.devries
(2) in PHP, strings are mutable, so it's not really worth the WTFery of using arrays. - nickf
(1) judging from that code sample, i don't imagine the guy was using join() for purposes of speed. - ithcy
26
[+4] [2009-06-19 15:50:08] DaniBaeyens

These two are not PHP specifics, but please please please please:

  • If you are not english speaker, please do not use your language not english characters on the variable names. Variables named $apáñalo are not funny (and I've even seen wrong behaviours using them)
  • Also, do not use stupid names for variables $what_a_long_variable is not descriptive, and people's mood won't get high

(6) Using overly short variables are bad also, such as $x, $b, $c, $dcx. - alexy13
(1) Uhmm... I think this is only half true. For cycles or small code blocks short variable names are handy, and definitely not harmful. Moreover code blocks should be ideally kept small enough so that they do not generate too much complexity by themselves. IE if you have a $a global variable (or its scope is too broad, even within a class or file), your problem is most likely not the variable name itself. At least in PHP or in situation where speed of execution is not your main concern. I second dexem thoughts in full though. - maraspin
(1) @alexy13 there is nothing bad in using $i in a for loop. Short variable names are not necessary evil, long ones are. - Ivan
If your entire php script is filled with short variables, then it could be an issue. - alexy13
27
[+4] [2009-06-19 17:04:10] Aduljr

Where 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.


28
[+3] [2008-10-24 14:23:54] Davide Gualano

Bad use of the magic method __call(); resulting is some ugly things like the automagically creation of the setter and getter methods via reflection.


29
[+3] [2009-03-30 12:10:02] Tristan Bailey

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 was seriously looking at that idea on an older IIS/PHP setup a few years ago. It was a hosted system with no mod_rewrite and installing a module for it was no option. Luckily, there was PATHINFO. - Pekka 웃
LOL, what an idea! - Pacerier
30
[+3] [2010-01-23 02:16:30] Dietrich Epp

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.


31
[+3] [2010-08-27 00:41:49] Stoosh

Walking past one of the 'developers' desks and seeing this code.

for ($i=1; $i <= 5; $i++) { 
    echo '<br>';
}

shudders


32
[+2] [2008-10-26 06:58:09] gizmo

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/41374
[2] http://wiki.php.net/_media/rfc/php.ns.txt?id=rfc%3Anamespaceseparator&cache=cache

seriously can you explain what is so horrible about the the backslash namespace operator? What would have you preferred? - nickf
(1) Well, the :: was a more decent choice (or any other commonly used separator). The main reason they did go for "\" is their incapacity to creator a correct context dependent operator. Now, you can have a string with "\n" in there that can be used as namespace, but when you try to debug using logging, you end up with a new line in you debug information. Great... - gizmo
I don't see anything horrible in it. Not perfect, but not horrible either. PHP is not that kind of academically pure language for that to be important. - Romme
33
[+2] [2010-01-23 01:42:23] L̲̳o̲̳̳n̲̳̳g̲̳̳p̲̳o̲̳̳k̲̳̳e̲̳̳

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)

  • Breaking 80 columns isn't cool anymore, let's break 120! Who knew that Moore's law applies to source line length?
  • 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 nowhere

If 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 />

34
[+2] [2012-03-18 22:05:05] GordonM

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.


35
[+1] [2008-10-24 14:07:25] jcoby

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.


DIRECTORY_SEPARATOR is good practice, as it is making your code more robust to future platform support. I agree it is a long constant, and in my applications I usually do a define('DS', DIRECTORY_SEPARATOR) in my bootstrap or wherever necessary. - Kieran Hall
also, symfony's front controller almost never needs to be edited, so using it as an example is unfair. - Nathan Strong
it was only an example. not bashing on sf at all. - jcoby
36
[+1] [2009-05-06 15:55:13] Niran

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.


37
[+1] [2009-06-19 17:28:09] ykaganovich

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/

Unfortunately this debate is more like a religious war, kinda like the vi vs emacs one. - K. Norbert
(2) Because a template engine limits the options, it leaves less options for 'junior programmers' to create a complete mess. Then again, I never fail to be amazed by the new and creative ways people come up with to create an even bigger mess when you limit their options. - Jacco
38
[+1] [2010-04-12 23:50:32] alexy13

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) {...

39
[+1] [2010-04-13 13:14:22] alex

What about using the e flag in preg_replace_all and the like?


40
[0] [2010-04-13 00:04:44] Earlz

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


41
[0] [2010-11-17 20:18:00] Pete

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]

42
[-2] [2008-10-24 14:00:00] gizmo

Planning on using the WTF crap php guys call " closure [1]"

[1] http://wiki.php.net/rfc/closures

use huh? nice. - jcoby
43
[-4] [2008-10-24 12:57:36] Binny V A

Setting the error reporting to 0 - error_reporting(0);

Good in production mode - very bad in development mode.


(10) Bad in production mode, too. You should LOG errors, but don't display them to the user. - Bob Somers
Agree with Bob Somers. - thomasrutter
I also agree with tomasrutter - alexy13
He's saying bad in development, good in production. - K. Norbert
Bad in development also. Case in point, our company system prints out XML templates to render the view and any error messages would serve as big fat monkey wrenches. - fred
44