Stack OverflowTop bad practices in PHP
[+104] [35] Alex
[2011-02-03 20:24:14]
[ php coding-style ]

I've learned from Stack Overflow that some of the code I've been writing so far is considered "bad practice". Like using tons of global variables, create_function() etc. I'm curious to know more about this.

Post examples of bad practices in PHP and why. Please only post one bad practice per answer, this way the worst can get the most votes.

(14) The book Code Complete answers this exact question, down to the most pedantic, mundane detail. It's programming language agnostic, but virtually everything he says there applies. Get it from the library and read it. - eykanal
(9) "But please only one per answer, this way the worst can get the most votes" -- I personally dislike requests like this as they make the list of answers longer than it needs to be. - Shaggy Frog
(11) I've learned more from this site than when I started to learn PHP from a book :s - Alex
(3) Also, is a near-duplicate. - Shaggy Frog
(3) I feel as though this question is going in the "pet peeve" direction, not anything in terms of performance/practicality. ;-\ (The latter I think more worthwhile) - Brad Christie
@Brad Christie: I may have posted a pet peeve... :) You're right, performance and maintainability type things are more useful. - Surreal Dreams
@Brad Christie you're correct. Annoying pet peeve coding standards are still valid as long as the programmer is consistent in style & all devs on the project are on the same page. - Michael
community wiki? - DaNieL
@DaNieL: Just flagged it for mod attention to make it CW. You should do the same ;) - NikiC
(3) Waiting for "Top bad practices in %LANGUAGE_NAME%", like "hidden features of..." series. - Nikita Barsukov
Here's a list of great books that people on Stack Overflow recommend.… - Ev.
(1) @Dan Grossman: and why is closing a subjective question as a subjective question bad? To quote the FAQ: "You should only ask practical, answerable questions based on actual problems that you face. Chatty, open-ended questions diminish the usefulness of our site and push other questions off the front page." How is "what's your opinion on the best/worst/most abused feature of X" not subjective? (not to mention "oooh, I'll just post the same question for Y, Z, and Q!") - Piskvor
(2) I don't like the fact, that this question was closed as subjective and argumentative. Yes, it obviously is so, but given the amount of input and discussion this question has generated, I consider it a wrong decision. New programmers can definitely learn much more from this question, than from yet-another-how-to-escape-sql-question. Even seasoned programmers might have realized some of their habits or ideas were wrong. So, yes, you are right, this is a subjective question. But it is a subjectivity, that encourages discussion, not argument. Voting to reopen. - NikiC
(1) It seems that actually this question has led to several followup question on the things that have been written here. I consider that progress, not "push[ing] other questions off the front page". - NikiC
everyone vote to reopen pls :D - Alex
personally I've learned a lot from the answers here, I think other will too :D - Alex
This question is as much subjective as… which is seen as a proper question. Voting to reopen. - Aron Rotteveel
@Aron: You realize that you linked to a question that is locked and thus we can't delete it? Locked is basically the same as closed other than that. - Jacob Marley
I still think this question should be closed. There are a few good answers but any new answers are not likely to add anything useful. - finnw
(1) @Bemrose yeah I know, but it was locked because of the edit wars. Not voting to reopen again, since this will result in the same, never-ending open/close voting battle. - Aron Rotteveel
$GLOBALS = $_REQUEST (Yes, that's from production code) - Thomas Ahle
[+127] [2011-02-03 20:33:56] Jeff Hubbard [ACCEPTED]

for($i = 0; $i < count($array); $i++)

God kills a kitten every time you call count() inside a loop.

(10) I do that :( Now I understand why I shouldn't ... - Alex
(1) Seriously though, the difference in calculation speed is somewhere around 2-3 orders of magnitude--see: - Jeff Hubbard
(7) This is one of the dead sins, especially in PHP, where you have foreach. - NikiC
@Jeff Hubbard Is this really that bad if you're not iterating more than say 20 times? - Flying Swissman
(49) I'd like to point out, that it is generally a bad practice to recalculate values that don't change inside the loop. Not only in PHP, but in pretty much any language, whose compiler can't optimize that. - Mchl
@nikic : i'll quote @Jeff Hubbard on this and look at the section "foreach() vs. for vs. while(list() = each())" on (I cannot state myself as an example, since I use foreach everytime I can :P) - Jeff D
(9) +1 for kitten-killer god! - Trufa
@Flying Swissman Yes. The difference gets worse as you increase, but why would you willingly recalculate the same value over and over again to no real purpose? Do you enjoy making your processor cry at night? - Jeff Hubbard
@Jeff: I can assure you, that there's something wrong with this benchmark ;) foreach normally is even faster than for loops, because it can make use of PHP's internal implementation of hashes fat better. - NikiC
@Jeff Hubbard Its just that I know I have done this in the past. Thanks to you, this little snippet of bad code will cease to occasionally pop up in my code. :) - Flying Swissman
@nikic I can assure you, I have tested it myself. Just because it can make use of some feature doesn't mean that it does. And foreach only applies when you have an array or otherwise iteratable object, not when you're actually, y'know, counting. - Jeff Hubbard
Poor kittens... - Adam Davis
@Jeff: Well, I assume that you simply have tested it just as wrong as it was tested in the linked benchmark. Using foreach and then utilizing the key to access the array ... that's really ... uhm ... let's call it, inefficient ;) - NikiC
@nikic I tested it independent of their code. But foreach isn't the issue here--count() inside of a loop condition is. - Jeff Hubbard
(6) @Jeff: I just wanted to ensure that people don't believe that foreach is slower. In any reasonable real-life case, it will always be faster. But yes, counting in a loop's bad too, you got my upvote on that ;) - NikiC
(1) does this also count in javascript? like using for(var i = 0; i < somearray.length; ++i) ? - Alex
As Mchl pointed out, it's a bad idea in any language that can't optimize it out (which is the vast majority of them in almost every case). That being said, length is a property in javascript, and it depends entirely on the engine in use. For SpiderMonkey (Firefox's javascript engine), it should be fine, but why take the chance when a local variable is cheap? - Jeff Hubbard
Interestingly, God doesn't kill anybody when doing this in many other programming languages. Oh poor PHP. - Al Kepp
(1) He's too busy smiting kittens from all of the PHP developers. ;) - Jeff Hubbard
Is it such a big deal, since count() is O(1)? - tstenner
If count() was O(1), it wouldn't--but count() isn't O(1). This is easily demonstrate-able with a simple benchmark: $arr = array_fill(0, 1000000, 1); $count = count($arr); $time = microtime(true); for($i = 0; $i < $count; $i++) $arr[$i]; $end = microtime(true) - $time; echo "Using \$count took $end microseconds\n"; $time = microtime(true); for($i = 0; $i < count($arr); $i++) $arr[$i]; $end = microtime(true) - $time; echo "Using count() took $end microseconds\n";. On my system, I get an order of magnitude of difference, and it only increases as I increase the size. - Jeff Hubbard
@Alexandra: for JavaScript, use for (var i=0, ii=somearray.length; i<ii; i++) {} - Mark Eirich
(1) I don't think this qualifies for the worst PHP coding style - galymzhan
I don't think you understand just how bad it is if you think that. - Jeff Hubbard
(2) php is not able to optimize that out? Poor PHP... - ted
[+81] [2011-02-03 20:47:53] Michael

Failing to filter/escape database input or screen output when submitted by users:

mysql_query("INSERT INTO table (name, email) VALUES ({$_POST['name']}, {$_POST['email']})");

This is downright dangerous.

Not that this would even work without the values quoted in the SQL... - Michael
(1) Unless they are numeric/boolean types :D - John Cartwright
@John Cartwright Absolutely, as long as they've been verified as such, coming from $_POST - Michael
@michael it would make super duper dangerous! - gnur
(66) I've seen worse... mysql_query($_GET['query']); ... yes, I have seen it in the wild. - Bill
@Bill: Everything has it's use... Just think of phpMyAdmin. - NikiC
Given the existence of mysql_escape_string() I hope nobody writes like this anymore. Unless of course they googled "PHP MySQL tutorial", instead of reading the docs. (Which there is also no excuse for, because the docs are pretty damn good) - Leigh
I feel this is appropriate: - yuudachi
@yuudachi I figured it wouldn't be long before Bobby Tables showed up here. - Michael
(10) Why does this have so many upvotes? The real problem is that (in 2011 no less!) people are still creating dynamic SQL strings. Can anyone say "placeholders"? Really. - pst
@pst: Because people will fight you to death over this: "but the Internet says it's the right way, and those paramesomethings are too complicated" - and will conveniently neglect that they mean an article from 2001. Note the volume of such questions here on SO. - Piskvor
@pst: Unfortunately MySQL has no native way to do it in another way. It really needs something like [pg_query_params()]( - ThiefMaster
@ThiefMaster you mean, something like mysqli::prepare + mysqli_stmt::bind_param or PDO::prepare + PDOStatement->execute ? - Arkh
That requires a wrapper function (lots of people are too lazy to write one) or multiple function calls in the main code (which is ugly) for a single query... And bind_param where you have to make a function call for every param is just plain annoying. - ThiefMaster
@John Cartwright - you need to escape any data you put into a query, especially if you use a dynamically-typed language like PHP. You could just verify it, but what happens when you accidentally change/delete the validation later on? Good practice dictates that you escape everything. - Chris Kuehl
[+69] [2011-02-03 20:37:44] Michael

Using the @ modifier to suppress warnings & errors, particularly when talking to a database. The proper way is to make sure errors aren't displaying on screen, but are being logged where you address them.


(14) +1: I hate the operator @! - Murilo Vasconcelos
(14) Sometimes though the @ is a valid and good thing to do. - NikiC
(1) thank you, huge sigh to suppressing errors. - twmulloy
(7) @nikic: Can you explain one use where @ is not only valid, but good (as you say it)? I wasn't aware that it's ever good to outright suppress errors... - ircmaxell
@ircmaxell: Sure, I can ;) Imagine you make a HTTP request and you aren't an advocate of accessing pages in 30 lines using curl. Can you imagine that? Now, file_get_contents throws an error in case it gets back a header like 404 or 500 (for I don't know what reason). I obviously don't wont my error log to be cluttered with these messages, so I suppress the error and use $http_response_header to handle each error appropriately. - NikiC
(2) @nikic: one would argue (as I would) that you should be using a class that abstracts that away for you. Either that, or use (as I do) error exceptions and catch the HTTP error so you can gracefully handle it... - ircmaxell
@ircmaxell: Well, I do catch the error and handle it gracefully. In my eyes putting a @ there and using $http_response_header is a much better approach to the problem, than registering an error handler, executing the statement and then unregistering it again. Or is there any hidden benefit of doing this? - NikiC
(2) @nikic: Well, it is better than registering and unregistering a handler, but I still consider suppressing the error bad. I have a handler always installed on all of my code, which lets me treat errors like errors and not blindly ignore them (which is too easy IMHO with the default error handler). - ircmaxell
(1) @ircmaxell: Well, what is your definition of "suppressing an error"? I do handle the error. It's not like I just put the @ there and hope that everything will be fine. Registering a handler that will convert all errors to exception is not an option for a production site (if I correctly understand what you're doing). I don't want my clients to get white pages, just because some silly variable is not initialized. I want that variable logged, so I can fix it, but I don't want an Exception to be thrown. - NikiC
(2) @nikic: I do want the exception thrown, since I can then handle it (and if it bubbles all the way to the top it's caught by the default exception_handler, the code logs the error and displays a 500 error to the user). It ignores notices in production, but throws exceptions for everything else. That way, I can tell what went wrong, and have full information about it (including stacktraces). - ircmaxell
(10) I never thought I'd say this, but here's an actual good use case for @: Ashley at semantic dot org had this to say in the PHP documentation comments for unlink: "you should use @unlink rather than testing with file_exists. The former is atomic, whereas the latter can break if you can't guarantee only one process will try to delete a given file at a time." - Stephen
I agree with @nikic: occasionally, suppressing an error is the sane and elegant way to do something. And then there is the (fortunately very small) subset of functions that return an error code and throw a warning. fopen() is the worst culprit. - staticsan
@Stephen: That's a good one that I will concede to. Learn something new every day... - ircmaxell
(1) @Stephen: There are other ways to ensure that only one process is deleting a file so this is a contrived example at best that can be taken care of in other (better) ways. One way, for instance, is to setup an asynchronous queue. With this, you can better guarantee atomicity. I can't believe people are still trying to validate the use of this monstrosity that is the suppression operator. BTW, just because something "can" be done, doesn't make it "good". - wilmoore
(1) @wilmoore 'One way, for instance, is to setup an asynchronous queue.' ... 'just because something "can" be done, doesn't make it "good".' I think you just answered yourself there. - mgkimsal
(1) @mgkimsal: I disagree. Utilizing an asynchronous queue is a tremendously beneficial practice. I would argue that the suppression operator is well, not as much a beneficial practice :) - wilmoore
@wilmore What if a different process entirely than your PHP code decides to delete the file, like a cron job script for cleaning up temp folders? Not likely, but possible. - Stephen
@wilmoore: Uhm, could you please elaborate on how to set up such an "asynchronous" queue in PHP? I can imagine writing a node.js server for that and communicating with it via PHP, but that is definitely the wrong approach if all you want to do, is invalidate a cache file or something like that. - NikiC
(2) @nikic: Agreed. No need to setup NodeJs in this case. An asynchronous job queue (not to be confused with the evented/asynchronous nature of NodeJs) is as simple as Gearman or can be developed on top of something more general (and more complicated) like AMQP/RabbitMQ. A process (or multiple processes) send a job to the job server, the job server sends a job to one of many workers waiting to handle work. The workers can all be on the same local host or they can be on remote nodes. Quite simple. Check out beanstalkd, resque, and gearman for simple work queues. - wilmoore
(1) @nikic: Also, when invalidating cache on a normal filesystem you will be hard pressed to guarantee 100% atomicity. If you really need that level of guarantee you should be working with something other than the base file-system. Look at redis or mongodb. These are built with asynchronicity in mind. - wilmoore
(1) @Stephen: in that case (some other process touching the file), that is an architectural issue that should be solved, but it shouldn't be a free license to utilize bad coding practices like the suppression operator. - wilmoore
@wilmoore, Now take yourself out of your own sandbox. You've just written a cool MVC framework using PHP. Developers of all levels from intermediate to advanced will deploy your package on everything from shared hosts to dedicated servers. You no longer have control of certain architectural decisions made by the developer. When someone comes to your issue-tracker and asks why they are getting a warning notice when your framework clears the cache, why would you want to terminate that relationship by saying "It's not me. You have architectural flaws." ? - Stephen
I don't like error suppression in the least, as evidenced in my answer, here:… But, @unlink looks like a viable use-case, IMO. - Stephen
(6) @wilmoore: Seriously? Setting up asynchronous queues? To delete a freaking file? Talk about overengineering. That's a far worse practice than suppressing an expected error. - cHao
(1) @cHao: You'd be surpised how many large projects already have this setup, so no, you don't go out and setup an asynchronous queue just to replace error suppression. If you have it, you should use it; however, I would argue also that many projects that aren't using it should be for many other reasons :) - wilmoore
@Stephen: If your framework's goal is to support every possible use-case under the sun including badly architected use cases, then I suppose that is up to the framework's maintainers. It doesn't mean I buy the argument that error suppression is a good thing -- it just means that if you personally feel OK abou it, its up to you :) - wilmoore
(2) I will concede that if you are just building a website (as opposed to a large web application), this entire argument is over the top and one should just do the simplest thing that works. Agreed there because for most websites, it just doesn't really matter. - wilmoore
Using @ with mysql functions is perfectly fine as long as you check for the return value and log the error returned from MySQL (you can trigger_error() it for example). And for mysql_query() you want special handling anyway so you can log the offending query. - ThiefMaster
(1) I disagree. I've used @ several times where the function would return some form of failure code anyway. Why does PHP trigger an error as well as return false? I don't know. :) I'm also kind of better off without try-catch when I'm working with functions, since it's the function result that matters. - Christian Sciberras
[+64] [2011-02-03 20:32:57] Kai Qing

Not indenting, in my opinion.

function awesome($someone){
if($someone == 'you')
echo 'it is you';
case 'him':
echo 'that guy';
case 'her':
echo 'that girl';
echo 'someone else';

It's just impossible to read if you have to update someone's old code as I do all the time in my horrible job.

I think not closing a parenthesis is worse ;) - gnur
(136) Must resist the urge to edit your post and fix your indents :) - Surreal Dreams
@gnur: isn't that just a syntax error? Not a good practice either, but not quite what the question talks about. - Surreal Dreams
Also, missing single quote in line 2 - makes the whole thing even less readable, as syntax highlighting is totally messed up. - delnan
That's why I use Notepad++ whenever I'm working on someone else's PHP code. I can just hit TextFX --> Edit --> Reindent C++ Code and suddenly everything is rainbows and sunshine. Except when people embed HTML. They just need to be taken out back and shot. :| - Jeff Hubbard
(13) Applies to pretty much any language, not just PHP... with the exception of BrainFuck - Mark Baker
(1) @jeff - yeah I can hit the fixer but I have to highlight the section of code to do so since the garbage I have to fix is often so badly written that the editor gets confused with all the php / html mixed in. It's not that it is hard to control + f and find the code I need to clean or fix, but it is just annoying when I get there and it is always a pile of trash. No indents, or indents with spaces and not tabs. wow... Makes me want to quit and get a job mixing paint at home depot. - Kai Qing
ctrl-meta-\ ftw - flies
(5) I don't think this is so much of a bad PHP practice, particularly given that fixing it is so easy in pretty much every halfway decent PHP editor (In PHPStorm it's Code -> Reformat Code). - Billy ONeal
Mis-indented code is worse when the original coder likes tabs at 2 and tended to use spaces, but didn't indent all the time. - staticsan
(6) And then they complain about semantic indentation in Python. - ulidtko
(2) break without semicolon? - Benoit
what horrible job do you do - yes123
@BillyONeal Yeah, unless you get one of those "Real Programmers" that use Notepad to code. Which, IMO, is totally unprofessional. It seems like they're more interested in sending the message that they're "old school" than writing good clean code. Or the ones who use two spaces (might as well not even indent). - mmmshuddup
@mmmshuddup: The choice of editor does not make a programmer more or less professional. Of course if (s)he is not willing to modify things for future readers of the code, that is unprofessional, but the tools one uses to accomplish that have nothing to do with professionalism. - Billy ONeal
You have a great point but I believe if an IDE exists (and is good) it should be used. If a programmer can seriously be more productive and still have clean code using Notepad, then I guess they are the exception and more power to 'em. But as I've seen time and time again, that is almost never the case. - mmmshuddup
[+55] [2011-02-03 20:37:02] Mchl

Mixing business logic with presentation logic. This has its roots in PHP being in the beginning pretty much a templating system for HTML. Unfortunately you still see a lot of code like:

<?php session_start(); ?>
<?php connectToDataBase() ?>
<?php doSomeStuffNotRelatedToDisplayingHtmlAtAll() ?>
 <?php $result = mysql_query()
  while($row = mysql_fetch_assoc($result)) {
   if($row['condition']) {?> <td></td> <?php }


(42) I just saw some code like this today. And I'm very sorry to say I wrote it six years ago. :( - Michael
I'm reading a book that does this now, it drives me nuts. I'm a rookie but i know better than to use that! - Drewdin
(5) Oh MAN I hate this! Some of our biggest clients had sites made by amateurs. Like $300k plus sites and when they ask for a simple change you check the files and you get THIS! PHP barfed all over the inner html for no good reason at all. Sometimes even templated in such a way that all the POST handling is done AFTER headers are sent! I could kill myself, really. - Kai Qing
(2) This is a context thing -- if the entirety of your script is to display the correct year in the footer copyright notice, then it's less necessary. But then again, you're also killing an ant with a sledgehammer by using PHP for that purpose. Generally, I agree completely. - aendrew
@Kai I feel you... ugh I remember adding some feature to a site coded like this... sucks... I want it done the soonest possible time... can't stand it. - Woppi
Extremely guilty of this when I first started learning PHP. - yuudachi
I've seen worse in php. - Lime
@Lime: Well, I wasn't really trying to invent something horrible :P - Mchl
Sorry :P I meant to say ASP. not php. Sorry I'm not sure what happened there. %) In old ASP apps they often mix model, view, controller and spaghetti. Yummy spaghetti! - Lime
Well I guess each language has it's own share of such stuff :D - Mchl
[+51] [2011-02-03 20:36:01] Murilo Vasconcelos

Using register_globals = On. This results in big headache.

(4) I would ammend this that /depending on/ register_globals = On for your code to function. - Michael
(23) How are things over there in 2003? - Roger Halliburton
@Michael its bad even if you don't depend on its functioning, since an attacker could alter variables dynamically without you even using bad practice. - yahelc
@yc That's correct - perhaps we should add uninitialized vars to this bad practice list, as that's the main cause of register_globals' problems. - Michael
(3) I had a techie at Adobe of all places tell me that he was absolutely shocked, SHOCKED, that I wouldn't turn this on for my client on a shared web server... until I sent him this page: or something like it, saying essentially that it was considered a security threat, and the authors of PHP DISCOURAGED USING IT. What do you know, about a week later, our customer gets an updated version of his web application from Adobe. :-) - SplinterReality
@Charles according to that page: This *feature* has been DEPRECATED - Josh
@Charles Chappell Considering Adobe is the #1 source of security & malware problems in our organization, despite rigorous patching, this kinda doesn't surprise me. - Michael
@Josh This was a good 5+ years ago, when the awareness of Register Globals being a bad thing was just starting to become really prevalent, so that page was a little different than it is today. - SplinterReality
@Charles So, this goes into the, rather large, category of support for legacy systems causing dangerous side-effects? (I only started using PHP a couple years ago.) Regardless, at least support for this is going away. - Josh
@Josh I'd say that's a fair assessment, but I'd lay a fair amount of the blame on the PHP developers for not thinking the Register globals idea through in the first place. Deprecation of features always causes problems, so it's really a Catch-22 at this point. - SplinterReality
[+42] [2011-02-03 20:30:03] gnur

Worst practice ever?

echo "$var";

It really hurts me when I see it somewhere.

(5) Maybe worse: echo "this " . $var . " that"; - Surreal Dreams
Are you against placing $vars in strings, or just the "extra mile" for a one-variable print-out? - Brad Christie
(2) I use something like echo "Blah: {$var}"; I think it makes the code prettier :) - Alex
(2) It's an explicit type conversion. Since echo itself expects a string, the $var would be morphed anyway. (And there are actual use cases, like SplString or other __toString implementors.) - mario
(1) @surreal Worse then echo "this $var that";? - gnur
(1) echo "this " . $var . " that"; makes syntax highlighting easier to read for humans. If anyone didn't get this comment - echo "this $var that" would work cause the double quotes allow var interpretation where single would print the literal. - Kai Qing
(1) @surreal why is echo "this $var that"; bad? - Kenny Cason
(1) @Kenny Cason: It's not. Breaking up a double-quoted string to insert a simple variable doesn't make any sense, though - it's parsed for variables, so stick 'em in there. @gnur said that, anyhow. My comment was the very first one. - Surreal Dreams
(1) Encapsulating $vars within double quoted strings is slower then doing: echo 'this ', $var, ' that'; - gnur
(4) Um... you're all doing it wrong. echo "this " . htmlspecialchars($var, ENT_QUOTES) . " that"; ;) - MiffTheFox
(1) @gnur: Slower by how much exactly? Once you learn the answer to that, you stop caring. @MiffTheFox: can we assume for the moment, that $var has already been preapred to use within HTML? Or that the string will not actually be used as HTML output at all? ;) - Mchl
@mchl Anything that makes my code even slightly faster at all is enough for me. I'd rather code something knowing it is slightly faster then the alternative then the other way around. - gnur
(1) Yeah, why denormalize the database or reduce queries when you can achieve minuscle speed gains by using single quotes? - mario
(1) @gnur: Knowing that my typing ' . . ' will take more time than it will save during entire lifetime of that code is pretty convincing argument for me not to do it. - Mchl
@mchl I learned actionscript as my first language and regularly code in javascript, python and java. So it is basically just habit. - gnur
@mario I always think that doing something knowing that it is slower just sucks. What to do after DB optimization, reducing queries, reducing http requests: look for the small things :) - gnur
I'm not sure of the validity of it but @mario in his answer (below), the linked document states that using only single quotes can increase format by 13.8% :P if true, that would be a considerable improvement - Kenny Cason
(1) @gnur: Regarding 'what to do after you optimize obvious things'. The answer is: 'do the profiling to find the non-obvious, don't waste your time on trivialities' - Mchl
print "Here it gets {$slightly} worse"; - twmulloy
(2) print("$var"); takes the cake - dogmatic69
(13) @dogmatic69 how about print("{$var}");? - twmulloy
@twmulloy, i did not think it could be worse... +1 - dogmatic69
I have had occasions to do "$var", always to make sure it was a string (though I prefer (string)$var). I always left a comment on why I did this. - staticsan
Oh, and although this isn't really true in PHP 5, in PHP 4, concatenating a string was far faster than an embedded reference. - staticsan
@twmulloy: Wow. Putting that in a code would probably make me a murderer ;) print + braces + interpolated variable + pointless interpolated variable... - NikiC
(2) Turns out I was wrong, I tested it with several methods of displaying a certain string. As it seems echo "$var is blabla and $ho is something else"; is just as fast as echo $var . ' is blabla and ' . $ho . ' is something else'; See the results at - gnur
-1 There's nothing wrong with this. Why do 30 programmers think there is? - Django Reinhardt
(1) @Django Because echo $var; works just as well and looks much better. - gnur
This is hardly "bad practice" IMO, but I take your point. - Django Reinhardt
There are many more pressing things that are likely wrong in the code...this is hardly a big deal; however, I do agree that it is a bit silly to do it this way :) - wilmoore
[+40] [2011-02-03 22:02:30] ircmaxell

Simple: magic_quotes [1] being turned on on.

Why? Because escaping cannot be done automatically. People think they are safe since it's on, and wind up with \ in their database, or major vulnerabilities because it doesn't work how they think it does...


+1 It's not a bad coding practice, more bad management. But it's nevertheless the worst. (Sidenote: magic_quotes wasn't originally intended as security feature. That's a recent misattribution. The sole purpose was to make SQL concatenation work for noobs. A pure convenience feature. Accidentially it was also secure anno 1997, because msql didn't support multibyte charsets...) - mario
It's bad. It's the worst thing that ever happened to PHP. Even register globals left a less impact. The magic_quotes disease is so big, modern systems even emulate it, such as Wordpress. And if you disable it? Bam - hacked. I'd give 20 points for this if I could. - Christian Sciberras
[+36] [2011-02-03 21:10:02] twmulloy

commenting the obvious

// return false

return false;

using eval()

too many arguments, just pass in an object or array

// too many arguments
public function doSomething($arg1, $arg2, $arg3, $arg4, $arg5, $arg6)

// better, also put type and maybe some comments on usage for others.
public function doSomething(array $args)

not quoting array keys

echo $yourArray[someKey];

echo $yourArray['someKey'];

functions that do too much, "Swiss Army function (?)"

public function someFunction(string $arg)
      case 'update':

      case 'delete':


// good, separate functions-- keep it clean and simple
public function update(){...}

public function delete(){...}

public function someFunction(){...}

(4) Since when does your last example throw an error? It simply won't iterate ^^ - NikiC
maybe i'm mistaken-- but it'll attempt to iterate the first index before the iterator realizes it has no index - twmulloy
(2) empty array will just not foreach. you will get errors if it was not an array though. some methods/classes return false on fail or the array of data - dogmatic69
fair enough, i've removed that example. i think it appears like that for me in most cases because the array was never created in the process before the foreach instead of it being empty. i have to clean up a lot of the php notices for work and that check generally resolves those types of notices. - twmulloy
Haha, swiss army function. Or miracle function. - yuudachi
(3) Commenting the obvious is the very worst.... makes me want to hurt someone - Webnet
The doSomething method with a lot of arguments is actually (arguably) better than the alternative with the arrays, as the arguments are self-documenting and will throw errors if no values (or default values) are passed. The array variant needs documentation to inform the caller which values are mandatory and what they should be indexed under. Example: doSomething(array(1, 2, 3)); and doSomething(array('some str!1')); - which one is valid? I would not recommend replacing method params with an array. - fwielstra
@cthulhu i agree that many arguments is not bad, but getting carried away and then reaching a point where you're doing things like doSomething($arg1, $arg2, $arg3 = true, $arg4 = null). where you're having to set defaults to the argument to not break any reference to the function when it had 2 arguments. in this case, should really revisit the function and consider maybe passing certain arguments as an array. the point i would like to make is that when you're reaching the point where you have many arguments that you should reconsider rewriting your function, ~6+ arguments is my threshold. - twmulloy
@twmulloy: An array could work, but even then you'd have to think which is more obvious and maintainable. Usually, in hardcore OO-programmed applications, functions with many parameters are replaced with an object containing the arguments (see also: Edit: Actually, that entry also speaks about Magic Container, which is effectively what passing an array to a function is. - fwielstra
[+34] [2011-02-03 22:49:06] Webnet

I can't stand it when people abbreviate variables.... avoid abbreviating to make your code more readable.

$ml = 123;
for($x = 0; $x < $ml; $x++) {
    //code here

You have NO IDEA what $ml is... why not just called it $minimumLines or something where a developer can look at your code and know what that value is.

(19) Actually, when I read $ml I immediately though of minimum lines :D But generally, you raise a valid issue. Though one should not exaggerate. $i definitely is a far better variable name than $iterationKeyStorage_integer. +1 - NikiC
LoL, yes, $i is definitely better... but you get the idea :) - Webnet
(1) +1 I've worked on something like this before... and what's worse is it's an accounting-related website with the $tpacc, and more like that as parameters... all I understand is that acc = account... I mean How the hell can I understand the meaning of these variables! UGH! - Woppi
[+33] [2011-02-03 20:40:33] mario

Some of the things you have been told to be "bad practices" are just memes gone awry and overgeneralized on Stackoverflow. Here are a few antithesis:

Effective PHP Cargo Cult Programming [1]

(I'm so going to get downvoted.)


+1 I don't agree with all what is being said there, but some points are valid. - NikiC
@nikic: It's not a coherent list. Some entries just describe sloppy coding. And some idioms are actually good practices and contribute to quality, if used consciously or in moderation. - mario
(2) Now that's an incendiary link! :-D Though some points are actually good: avoiding code that generates E_NOTICE, for example. - staticsan
(2) enterprisey! I like it. - Dan Burton
@mario hilarious, but when discussing methodName() you failed to mention that METHODname() calls the same MeThOd in PHP, so camelCase really doesn't make sense (though neither does strtolower coupled with str_replace). - Josh
@josh. Ah, nice tip. Forgot that even though it was irking me quite often too. I shall add that (needs a rewriting anyway.) - mario
half of those points are baloney, but the other half is good... :) - StasM
@StasM: I wonder if your halves are same as mine (probably not) :D - Mchl
That link should 1. be on a webpage instead of a PDF, and 2. written better, right now it doesn't quite get the point across due to it err. not really getting the point across. - fwielstra
@Mario - looks like a dead link now. - Tim Post
@TimPost: Yep. I'm dissatisfiedly aware. Thanks for the note! Applied a workaround url for now. (Just had enough time to salvage my openid) - mario
[+26] [2011-02-03 20:44:40] David W

Failing to escape user submitted data:

echo $_POST['key'];

(3) This creates a security hole if the variable is <script... Use htmlescapechars() - David W
(16) It's hardly a security hole, when the user is attacking himself. A good point in general though. - Mchl
It's a good point that in this example the user would probably only be attacking himself but I can think of a scenario where this could be exploited in a DDoS attack. @Michael 's case is more typical. The point is to treat all user data as suspect. Also, if a user submitted < then they expects to see <. - David W
(19) This is a complete security hole. If I can trick you into following a link to that page with parameters I set, I can insert javascript to do anything I want. Like maybe steal your cookies, and then log in as you. Mwuhahahaha! @Mchl is wrong. Always use htmlescapechars() or similar. - James
Hmm... I guess you're right. Didn't think about that possibility. - Mchl
(1) @David @James It's htmlspecialchars(). - Nyuszika7H
@James a link that can set POST parameters? How? - Gerry
(3) @Gerry. Its called a form. You can even submit them automatically with JS. - James
@James: I would have though their would be a protection on the form submit event so that it can't be launched from JS unless the action domain was the same. But looking now it appears not. +1 and thanks. - Gerry
It can also lead to a notice displayed and/or logged - Martin Dimitrov
[+23] [2011-02-05 00:28:50] DaNieL

Believe the user is your friend and he'll never try to hack you.


You HAVE to assume your visitor is a maniac serial killer, out to destroy your application. And you have to prevent it

hhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhh - Qchmqs
[+22] [2011-02-03 22:11:22] ircmaxell

Debugging and writing code without E_NOTICE [1] active.

Why: A fair number of developers (including those who should know better) think that notices are not important and hence can be ignored. However, a fair number of bugs sneak through because of miss-spellings or typos. Undefined variables are errors and should be treated as such.

Better: Always write and debug code with all errors levels running (E_ALL). Only on production servers should you limit the error level as appropriate.

Mario's edit to add a counter-argument: That's plain wrong. PHP is a dynamic language and dynamically typed. Handling undefined variables is one of the core features, as it's optimized for receiving forms. Notices are debug messages and not errors (potential problems != faults). They should be turned on in the development stage. They should neither be ignored, nor habitually suppressed, but evaluated in each specific case (there are syntax notices and informational notices, should not be lumped together). Using isset($_GET["var"]) ? $_GET["var"] : NULL is functionally eqivalent to @$_GET["var"] - everything else is a lie. Or a micro optimization (which are only ever appropriate in loops).


(4) The official PHP distribution package is, unfortunately, confusing on that score. The default php.ini has E_NOTICE disabled and has so for a long time. This leads to a lot of PHP code that throws many E_NOTICE errors because the developers never turned it on. - staticsan
(2) @mario: While I understand the sentiment behind the counter-argument, I still think it smells of code-rot. While the language may handle it, that doesn't mean it's a good idea (goto is supported by the language as a core-feature). Plus, I'd argue you shouldn't be doing $foo = isset($_GET... or accessing request variables directly in any case. Instead, you should use some kind of request class/object that abstracts away things like magic_quotes handling and character set conversion of the input stream... - ircmaxell
(4) >>> Using isset($_GET["var"]) ? $_GET["var"] : NULL is functionally eqivalent to @$_GET["var"] - everything else is a lie <<< a) It has global sideeffects b) It can trigger function calls and c) It's not NULL its "" (empty string) --- iirc :) - edorian
just for the sake of pedantic clarification, undefined variables are not errors, per se. - Stephen
(1) @Stephen: That's up for debate. I do consider them errors. And I do consider them an indicator of code quality both from a readability and testability standpoint, and a functionality standpoint. Since any undefined variable by definition introduces another potential code-path aside from the normal ones. - ircmaxell
Like I said, just being a pedant. :) The statement should be " Using undefined variables creates errors." Take JavaScript for example if (variable === undefined). It's practically a datatype. - Stephen
@edorian: Well c) is wrong, a) is fictional, and b) could be true for isset($_GET) too. I'm for example using an ArrayObject $_GET wrapper, which captures isset calls via ::offsetExists. It could implement tragic global side effects (much like a custom error handler could); but that's entirely unprobable and seems a red herring like "all notices are errors". - mario
(1) @mario you are right on the null part. For the rest i don't see a further need discussing this since i feel you don't care for it. Thanks for the correction :) - edorian
[+21] [2011-02-03 20:44:59] Radoslav Georgiev

When i see this i wish i never started using PHP :

function something ($optional_var1 = 1, $optional_var2 = 2, $must_provide_var) {

Or another thing is when people abuse dynamic typing : $a = "1" + "2"; and expect integer

(7) WTF? This is possible?! - NikiC
how works this definition? OMG! - raultm
Yes this is possible. I think E_STRICT barfs on that though. - Mchl
(10) A good IDE will yell at you for listing optional function params before mandatory params. - Stephen
Ugh... I remember that one... it was a nightmare when someone did a function like that... - allenskd
(5) Hm, I wonder whether one can somehow, through some very dirty hack, actually pass only the third parameter.... - NikiC
I don't think it's possible. I would be shocked to find out how! - Zachary Burt
Maybe through call_user_func() ? - Zachary Burt
Stupid as it is, this wouldn't be a real issue if PHP had keyword args (sigh, why can't my work use Python). - Josh
@Josh: it wouldn't be a real issue if php developers were using array parameters - at least if they expect both stable & extensible api - Kamil Tomšík
I do not wish to smoke the same that the author of the function did... - andre matos
(2) @NikiC How about something(,,$someVar)? - mc10
(2) @mc10 this leads to a parse error Parse error: parse error, expecting ')' - Martin Dimitrov
@Zachary Burt call_user_func accepts a list of params. how can you skip the first 2? Not possible. call_user_func_array expects the parameters to be passed to the function, as an indexed array. So we might think we can do call_user_func_array("something", array(2 => 3)); to provide just the third argument but actually the keys of the array are ignored and reordered starting from index 0. - Martin Dimitrov
@Martin Let's give this another shot: How about something(null, null, $someVar)? - mc10
@mc10 null is an absolutely valid value, so basically your code is calling the function something with three arguments. This means that $optional_var1 and $optional_var2 will end up with value null and not use their default values :) - Martin Dimitrov
[+16] [2011-02-03 21:07:09] PJK

Whoa, this list could grow really really long as ways of screwing things up are vast.

From those commonly encountered:

  • Mixing functionality (typically db queries) with HTML
  • Not sanitizing inputs (including type and range checking, which is commonly ignored)
  • Extremely poor design decisions due to poor knowledge of both PHP and OOP principles
  • Reinventing the wheel instead of searching in the common library
  • Nesting DB queries (made me punch a guy once)
  • Poor naming conventions
  • Misleading documentation
  • Not taking advantage of subtle nuances of PHP (for example single and double quoted strings)
  • Relying on multiple switches instead of using introspection and metaprogramming
  • Abuse of poorly understood libraries
  • Obfuscation of simple code by overusing design patterns and best practises
  • Not adhering to corporate/project coding standard, mixing various styles
  • Wasting of memory by not taking advantage of references
  • Crippling code with unbecoming references
  • Using deprecated features
  • Not choosing appropriate tools (frameworks, ORMs)
  • Mixing exceptions based code with return values indications (awwww)
  • Ignoring exceptions or handling them sluggishly (leading to unpredictable behaviour and difficult debugging)
  • Improper use of testing tools

... I could go on and on. I've seen terrible things :/. Just use your brain and get to know your tools and you are going to be OK

What do you mean by "Nesting DB queries"? I do while ($row = mysql_fetch_assoc($query)): [do another query here] all the time, when logical. Or am I misunderstanding? - Andrew
Why are subqueries a bad thing? Subqueries are sometimes actually faster than joins and in that case I use them. - NikiC
(3) @nikic: Subqueries are a thing of SQL not of PHP, so I doubt PJK writes about them. As far as running queries within loops go, it generally should be avoided. As with any general rule, there are valid exceptions. - Mchl
To clarify: I don't mean subqueries, I mean nesting queries as described by Andrew. These can be avoided by JOINs and other techniques. As long as resulting SQL query is reasonably readable, this practice should be avoided. - PJK
[+14] [2011-02-03 21:15:53] Trufa

Using eval() [1].

Always remember: eval() is evil().

Just like this guy:

enter image description here


(19) Mine PHP says that Function evil() doesn't exist so I guess you're wrong about this alias. - Mchl
(1) @Mchl If it was a joke I didn't get it, sorry! and if it was serious, well... neither! - Trufa
(4) Yes it was a joke. You say 'eval() is evil()' which could be understood as 'eval() is an alias for evil()' (just like sizeof() is an alias for count()) Apparently to see it as funny, one needs to have his brain as screwed up as I do) - Mchl
(2) @Mchl Im laughing now :) I was going to strange places of my brain, places I had never been before trying to decipher that that but nope, not a chance in the world! - Trufa
oh my god this tatoo is epic - yes123
@Mchl, I wrote my own function that I include in all of my projects: function evil($code_str) { return eval($code_str); } - Nemoden
@Nemoden: That sounds like a great idea! - Trufa
[+12] [2011-02-03 20:50:40] DaNieL

Passing every input inside a 'sanitizer' function, without care about what that input mean.

$an_aspected_integer_id = my_super_mega_iper_sanitizer_function($_GET['id']);
// (int)$_GET['id']; looks bad!?

Using a lot of if-else short declaration

 $myvar = $x == $y ? $y <= $z ? $y : $x != $j ? $k >= $j ? $k : $j : $x : "omg_i_did_again" ;

You see this a lot? I've never really seen something like your first example. (the second one, all the time.) - Stephen
(1) I used to to that when the first time i approached php (and programming, in general) XD - DaNieL
[+12] [2011-02-04 06:10:48] Michael Trausch

Writing a 30,000 line Web application, with:

  • Zero defined functions (only functions used are built-ins)
  • Direct variable insertion into SQL strings without quoting
  • Circular includes
  • Includes-as-a-function-call
  • Recursive Includes

I could go on and on and on.

Maybe I could sum it up simply and generically: Writing it ugly, bad. Not bothering to incrementally improve: just plain offensive.

(1) Zero defined functions is probably why it's 30K - RobertPitt
Indeed. I never imagined that it could be possible for someone to write and maintain an application that was literally nothing more than a pile of gunk... - Michael Trausch
[+12] [2011-02-03 22:32:38] allenskd

People should stop using die() like, a typical example die("MySQL error:".mysql_error()) use PDO and try/catch exceptions

(4) I'd argue against using trigger_error(), since it's too easy to ignore and doesn't actually interrupt the execution flow. Instead, use Exceptions or another inline error paradigm... - ircmaxell
Throwing an exception is always better than trigger_error(). It's recoverable and you get to see the stack trace (in case it bubbles all the way up). - Tomik
Edited, to be honest I also use exceptions than trigger_error or die, it kinda becomes a habit - allenskd
Question: If you catch an exception and wish to halt execution, what do you do? - Stephen
Nothing, once the exception hits the execution of the script will halt - allenskd
(2) @Stephen: rethrow it and let it bubble up... - ircmaxell
[+9] [2011-02-03 22:51:27] ircmaxell

Writing all your strings like:


Because ' quoted strings are faster* than " quoted strings.

The bottom line, write readable code. Write appropriate code. Don't worry about premature optimizations and micro-optimizations.

* Note: They aren't really faster. The benchmarks between them are quite inconsistent and both are well within the margin of error of each other.

(8) for readability on strings like that i opt for sprintf(), might not be as fast when you calling functions but makes the code so much nicer. sprintf('foo%sbaz%sbuz%s', $bar, $biz, "\n"); - dogmatic69
(1) In PHP 4, embedded variables were definitely slower. - staticsan
And this kind of "optimization" is usually a sign of an ... inexperienced developer ... benchmark it in context. Does it matter? Likely not. Move on. Get stuff done. (I think that everyone should benchmark and profile code -- to see that if it doesn't matter, it doesn't matter. A very good lesson to keep in mind.) - pst
(4) I do that not for the performance benefit (because if you, like any reasonable PHP developer, use APC (or a similar cache) that doesn't matter at all anymore). I simply think that '<a href="' . $url . '" rel="nofollow">' . $text . '</a>' is "cleaner" than writing "<a href=\"$url\" rel=\"nofollow\">$text</a>". Though it is already very rare that I need to write something like this. I use a Template Engine and that normally makes any manual work with strings unnecessary (well, apart from extensions for the TE maybe...) - NikiC
@nikic: If you write it because you think it's cleaner, that's 100% fine. But I see it quite often that people only do it because of the performance benefit (which is a fallacy in modern PHP implementations). And that's why I posted it here... - ircmaxell
i have to admit i do this all the time - yes123
[+8] [2011-02-04 04:48:51] wilmoore

Really, I'm not making this up...

if($_SESSION['username']=='name-of-lead-developer')echo "<a href='".$URlBuildString."' target='_blank'>Here it is</a>";

This wasn't git-stashed, this was live, in production, code.

[+8] [2011-02-03 20:36:28] NikiC
  • Using echo, return, ... with braces, like return(5);
  • Adhering to a coding standard over then Zend or PEAR or a close derivate
  • Using a non-English language in code (well, strings excluded)
  • Using eval instead of PHP dynamic language features, like $function() or new $class (eval is okay to use, but no if there is a native possibility to do it.)
  • if ($a == $b) { return true } else { return false }

(19) Disagree with the third point... you're effectively forbidding anybody that doesn't speak English from coding in PHP - Mark Baker
(1) @Mark: i agree with you, but if someday you want to share your code, you'll need to rewrite all. Lets say that at least the variables/function/classes names should be in english - DaNieL
@Mark Baker: In that case, one can obviously. I just don't like to always argue with people, whether or not to use English variable names, instead of German ones. - NikiC
@DaNieL: With "code" I'm referring to variables, functions, classes and constants. Strings obviously may be localized. - NikiC
@nikic: ok i misreaded, you have my +1 - DaNieL
I would argue that $funcion and $$variable are just as bad as eval. - Billy ONeal
(1) @Billy: Then do so, argue ;) I see nothing bad in using callbacks or instantiating a class using a dynamic name, instead of having a big switch/case list. - NikiC
(1) @nikic: Sorry -- $function is okay if you're using it as a callback (and nowhere else!). But $$variable almost always needs to go. Never said anything about new being bad. $$variable is bad simply because you're encoding the name of a variable as a requirement of your program logic. Therefore instead of being able to use a simple rename tool (or even find and replace), if you pick a bad name you're stuck with it. - Billy ONeal
@Billy: Well, PHP's dynamic language constructs should be used with care. I'm not suggesting that one should use ${'_'.!$_=getArray()}[0] as the shortest way to dereference a function. Admittedly I can't even remember when I used $$variable the last time, but I'm very confident that I have used it several times and did so for a reason. - NikiC
@nikic: Just because you used it doesn't mean it was good to do so. Really, if there are enough variables accessible from a single scope that needing $$ ever crosses one's mind, that's a code smell in and of itself. - Billy ONeal
@billy: What about $function where the variable is a generic object that implements __invoke()? - ircmaxell
@Billy: Didn't mean it like that. That wasn't supposed to sound like "I use it, so it's correct." I just meant to say, that I currently can't remember what $$variable is good for, but am confident that there are good use cases. - NikiC
@Billy: Hm, I just coded up a quick script to check my entire codebase for uses of $, but apart from some edge-case php-preprocessing unit tests I found only two usages in Doctrine. And they didn't seem very legitimate. I will remove the $$variable example from my post. - NikiC
@ircmaxell: (*Bill goes and looks up what __invoke is) Only place I see that making sense as a callback, but I can't say I've ever used __invoke (no PHP 5.3 features in anything I've done as of yet). - Billy ONeal
(1) @nikic: Fair enough. +1 :) - Billy ONeal
@nikic - lingusitically, I have enough problems trying to use the American spelling $color instead of the English $colour... but definitely agree with your last if ($a == $b)... instead of simply return ($a == $b); - Mark Baker
(1) @mark: someone who doens't speak english can't actually code - yes123
[+7] [2011-02-03 22:30:46] ircmaxell

Not understanding the different ways PHP handles references [1] and blindly using & for creating references with objects.

Reason: passing around variable references can get quite messy and lead to very hard-to-debug errors.

Instead, just let PHP internally handle the reference for you. It's far cleaner, and far easier to debug when something goes wrong...


In addition, it should be noted and coached that objects are automatically passed by reference. So not only is "&" adding noise and confusion, but it is also 100% useless in this situation. - wilmoore
@wilmoore: objects are not passed by reference (well, not the same mechanism that & provides anyway). So no, it's not useless at all. But most uses in the wild aren't necessary. - ircmaxell
That is true only to the extent that object "references" are not "symbol table aliases"; instead they are object pointers. That is a very technical difference which matters little in my opinion. And, my use of the word "useless" was meant as "not necessary" just as you put it. - wilmoore
[+6] [2011-02-03 22:07:48] ircmaxell

Not PHP specific, but uninitialized variables:

if ($foo) {
    $bar = $foo;
echo $bar;

Why: it makes debugging much harder while making the code less clear.

Instead, always pre-declare your variables (where you're going to conditionally assign them):

$bar = '';
if ($foo) {
    $bar = $foo;
echo $bar;

(1) Um, why is that a bad practice? I do this quite often, especially for function local variables. IMO, this style is waste of time and space. What about a short comment? - PJK
[+6] [2011-02-03 20:58:49] Tomik

Writing PHP4 style classes - no visibility modifiers so all member variables and functions are public.

Not writing unit (or other types of) tests. That usually also means little refactoring (it's hard to refactor code without tests).

Overusing magic methods - e.g. __get(), __call(). These methods can do cool tricks but also obfuscate class.

Premature sanitization of input values:

  • escaping all $_GET, $_POST, $_REQUEST arrays with mysql_real_escape_string()
  • storing htmlspecialchars() escaped string to database

Yeah, I tend to avoid magic methods. It's not like Perl, but PHP can still descend into write-only quicker than most languages. - staticsan
[+6] [2011-02-04 13:21:59] Benoit
$i = $_POST['key']
if($i == '') $i = $_GET['key']

c'est qoi ça au nom de l'enfer ??? you found this on a real word code ??? - Qchmqs
[+5] [2011-02-04 06:21:54] pst

I would have to say ... almost anything on (the "documentation section in particular).

While some people claim it's a great source for information (and there is lots of information) -- the problem is there is lots of bad information mixed in with the good -- but unlike SO, where the same applies, there is no real moderation. This site has done as much to pollute and destroy new developers -- if not more -- as it has done to encourage good programming. None of the "competing" languages (Perl, PHP, Ruby, etc.) can claim the same.

Reading the "accepted" user comments (often complete with code to avoid) makes me cringe.

The PHP3/4 era documentation was compact and useful and was the reason for widespread adoption. But it does lack in good programming advises (mentions lots of deprecated things, but no word in mysql_query). The comments are sometimes misleading (no wiki, no editing). Yet I think it's the minor problem. There's a gargantuan pile of really awful tutorials on the web which are indoctrinating developers with bad memes, coding advise or lack thereof. - mario
(4) I don't agree with this answer. I think that PHP has a very high quality documentation, when compared to other programming languages. If you find a faulty statement in the documentation or see a way to improve it, just file a Documentation Bug at and it will be dealt with. - NikiC
(9) The documentation itself is fine. The "user contributed notes" sections contain some absolutely horrible code which people purport as "cool" or "useful" or somehow clever. - QmunkE
[+5] [2011-02-03 23:53:48] Mark Baker

Open Source libraries without PHPDoc (or equivalent) tags, so there's no API documentation and you have to wade through the code itself to find out what it actually does... only marginally worse is PHPDoc tags that don't match the actual code.

I know it's hard work keeping tags up to date, and boring as well; but it's a necessary part of maintaining any open source library.

(3) +1 Furthermore there should always be a "real" documentation on Open Source projects. Not just PHPDoc. - NikiC
(1) @nikic - <rant annoyanceLevel="high">though a personal peeve is people who don't read either the real documentation or the API docs when they are provided, and then come to places like SO asking questions that are explicitly answered in that documentation... it's hard enough getting the right level of docs in a format that people want, and then users don't even read it</rant> - Mark Baker
Those are hopeless cases and don't deserve to be ranted about. - NikiC
[+5] [2011-02-03 22:05:35] ircmaxell

Using variable variables [1] in all cases:

Why? You should use an array instead:

for ($i = 0; $i < 10; $i++) {
    $var = "foo" . $i;
    $$var = $i;

That's a good example of how using them is wrong. However, there are some introspective techniques that are only possible because of variable variables. The only one I use at all regularly is to initialize as set of variables from a HTML request, defaulting missing values. - staticsan
@staticsan: can you provide an example that an array or object wouldn't be better suited for? - ircmaxell
@staticsan: Why not do that with an array? - Billy ONeal
@ircmacell: Yes. Putting the request variables into an array groups them together according to their source. However, the code uses them according to their purpose, and sets others in the same purposes along the way. Since the variables it sets are first class variables, it makes sense to initialize them suitable out of the request parameter. (Note I didn't say it was a good example.) - staticsan
[+4] [2011-02-03 20:28:21] Surreal Dreams

It's not the worst thing ever, but I really do hate it when people put the "then" part of an if statement without braces. Let me just show an example.

if($this == $that)

I hate this. It bugs me. Please use a set of braces...

if($this == $that)

It's small, but it really makes a difference to me. I go hunting for a closing brace that's not there. Put an else in the mix and it's actually worse.

if($this == $that)
//this drives me nuts

(23) -1. This is nowhere near "bad practice"; this is just a matter of style. I strongly prefer the brace-less version because the braces are plain unnecessary, and so do many others. - larsmans
I only permit myself to do it in if($condition) continue;. - Mchl
@Mchl: If you must do it, I prefer your way. @larsmans: style or not, it drives me crazy. - Surreal Dreams
(1) I hate it when no brackets are used, or if the "then" part is on the same line, it's just so annoying (for me) to read hehe - Kenny Cason
@Surreal Dreams: and it drives me crazy to see your style :) Extra braces are clutter, even more so in the if-else version where they hide the else from sight. - larsmans
(3) @larsmans: I'll admit, I used to do all kinds of things to cram code closer together, to fit it all in on fewer lines. Probably comes from my formative years coding on a Commodore PET. Now I take extra time to format the brackets out onto separate lines. I've only done it for a couple of years. I find a little whitespace makes it easier to read. I've obviously spent WAY too much time with graphic designers :) - Surreal Dreams
I'm guessing you don't like python, hehe. I actually prefer no braces in a one-liner. - Flying Swissman
@Surreal Dreams no, I agree 100%, a little whitespace makes the code much easier to look at :) i will say that for open bracket { i put it on the same line as the if statement, but other than that i still like the brackets. And plus, when you want to debug, it's easier to add extra code to the "then" block if the brackets are already there - Kenny Cason
@Surreal Dreams: I agree about the whitespace, but braces aren't whitespace. Braces combine multiple statements into a compound statement. "Compounding" a single statement is like adding zero to a number. But in any case, it's not bad practice, it's preference. - larsmans
(1) I can't stand this either, but @larsmans is right. - Stephen
I've never posted something so controversial, this answer has +8 and -6 so far. I have to admit, if I had it to do again, I'd just call this a style preference and move on. Thanks for keeping it civil, though - I realize this is a touchy subject for many. - Surreal Dreams
:) I would break those votes down like this: 8 people agree with you and can't stand it, 3 people disagree and like it, and 3 people either agree or disagree but don't think it's relevant. - Stephen
[+3] [2011-02-03 22:14:34] dogmatic69
  • if(true == $var)
  • not using unset() enough
  • doing things like select * from table when only need id
  • $object =& new Something() in 5.x
  • <div><?php $rows = $DB->query('....'); foreach.... ?></div> data handling in the view
  • output in model layers
  • wrapping lots of code in ifs like

    if($true){..code...}else{error(); return;}

could be

if(!$true){something(); return;} ...code...
  • loads of nested if/foreach/etc
  • etc

the list of bad practices is 100x bigger that best practice :)

(3) Why on earth do you typically have to worry about unset? - Billy ONeal
free up memory so your app is not a hog. you can quickly use up 20mb on a big request, which will slowly start reducing the number of concurrent requests you can run - dogmatic69
(1) unset() slows down your execution time. it's more a convenience for removing a key=>value pair from an existing array where array keys or values matter... like when using Zend Framework database $this->update($data, $id); where $data is an array and the key=>value must precisely match your db schema - twmulloy
if unset() is so slow why do you not create a new array then, copying only the keys you want... - dogmatic69
the select * is a good thing to consider, but that's more an SQL consideration than PHP. in its defense though, select * is acceptable depending on its usage (mostly if you're return all fields anyway). Definitely do want to restrict the results of a query for frontend queries (for obvious performance and security reasons) but for backend (eg. content management system) it's not always convenient to list out some large number of table fields to return, granted more secure but it's generally the case you need all field's data in a backend type system. - twmulloy
@twmulloy it is more sql than php, but sql is a huge part of most websites :) as for the backend, sure doing * is fine when you generally using all the fields, but there are still times when you just need one or two - dogmatic69
@dogmatic69 unset() isn't a huge hit on performance, but if you have an array with some x (large) number of post variables and you only need to remove certain keys, it's surely more convenient to unset($post['action'], $post['id']); than to essentially dupe the array with x - 2 keys. - twmulloy
@twmulloy seems we are talking about different things now, yes its easy to format the array, and yes its not a big performance hit. and yes it can free up loads of memory thus making the site faster when its busy (not waiting for memory to free to run the next request) - dogmatic69
(2) @dogmatic69: The problem with that argument is that the unset is implicit whenever: 1. you return from a function (local variables get unset), 2. your script terminates. Unless you have long running PHP scripts somewhere you're going to hurt performance using unset, not help it. - Billy ONeal
[+2] [2011-02-05 06:03:59] rabidmachine9

Using a CMS with deprecated functions in it...

[-1] [2011-02-04 04:42:48] wilmoore

I feel a little bad about submitting this as it is a real example that I found in a project; however, hopefully this will help someone else...

// accept DateTime or String as date
$this->birthDate = is_object($birthDate) ? $birthDate : (!empty($birthDate) ? new \DateTime($birthDate) : null);

Problems with the above code:

  1. It tries to do too much. It should delegate to two different methods. One that takes a string, the other that takes a DateTime. This way, the API is clear and easier to to test. Also, a user of this API might want to directly call one of the specific methods rather than go through the proxy method.
  2. It checks that $birthDate is an object rather than check that it is a DateTime object. This can cause some subtle bugs.
  3. Two levels of nested ternary statements...not so hot.
  4. Doesn't really check the string correctly. It assumes anything that is not empty (could be #1) is OK. It should at least wrap "new DateTime(...)" in a try/catch or even "date_create === false".
  5. Not a single unit test was written.

Just curious why someone would downvote this. When would it ever make sense to check "is_object" on something that should be DateTime? - wilmoore
[-1] [2011-02-03 20:32:26] SergeS

the & before variable in function parameters list is one of those - when you call it without variable it produces error

(3) Uhm, you do know that the & is for references and it is sometimes desired to pass a variable by reference? Normally people don't use it, just to suppress error messages. - NikiC
@nikic - my bad, I was confusing the & with the @ operator. - eykanal
Uh. & is the reference operator. Usage of these depends from case to case. As you are passing a reference, not a value, it can't accept a value. That just makes sense. Why is that a bad practice???!!! Haven't you messed it up with call-time passing by reference? - PJK
Why use reference when you can use object ( in PHP 5 sent as reference ) ? and if you don't want to pass anything - you don't get error. - SergeS
foreach($myArray as &$value){...} is better than the alternative-- creating another array. - twmulloy
But pretty laggy - alternative is to completly avoid such assignement - SergeS
@sergeS Because PHP has segregated objects and data types (primitives), thats why! - PJK