<

AntiPHPatterns and AntiPHPractices

Published on
45,704 Points
3,804 Views
14 Endorsements
Last Modified:
Awarded

AntiPHPatterns

Design Patterns are independent solutions to problems that occur over and over again.  Usually these are considered at a high level without much detail.  They are independent of core-implementation things like the exact language or data base.  And at the opposite end of things we find AntiPatterns which appear to be solutions, but are actually counterproductive, ineffective or subtly dangerous.  In 2010 Stefan Priebsch coined the phrase AntiPHPatterns, to describe some of these high-level pseudo-solutions that appear in PHP.  His excellent description was presented in London at the UK PHP conference.  His collection fell into four general categories: Constantitis, Globalomania, Singletonitis, and Godclass.


In this article I'll touch on Stefan's excellent analysis, and then I will add a collection of my own AntiPHP examples, showing how novice programmers often over-think or under-think specific problems.  I have seen these things in my work over the years, and on the occasions when I have had the opportunity to refactor, the view in hindsight has helped me to understand why some newer ways of looking at problems can help create a library of effective teaching examples.


Constantitis

In PHP, the define() function creates a constant.  Constants are immutable and global, and they can be defined anywhere in the code base.  As a result, it is entirely possible that two programmers working on the same project may define a constant with the same name, thus causing a collision that results in a run-time failure.  The presence of many global constants is a code smell.  Among other things, it may imply a large measure of configuration over convention, and with many possible configurations, things can become confusing.  In addition, if you want to change the settings that depend on the constants, you have to change the constants in the script.  Yecch!


Globalomania

In PHP, the global keyword creates a global variable, one that appears suddenly and unexpectedly in every namespace and scope.  This is slightly worse than a global constant, since it can be changed, and a change in one area of the script may have unwanted effects in other areas of the script.


Singletonitis

The singleton design pattern tries to solve the problem of having multiple instances of the same class, when only one instance is needed (eg: a data base connection).  But in practice, singletons are often used as if they were global variable containers. Singletons may share same the code smell with other globals (constants and variables).  Singletonitis has the same cure as constantitis and globalomania, which is dependency injection.


Godclass

It is axiomatic in software development that encapsulation is good, both for code and data.  A well-written class should do one thing perfectly (or as close to perfectly as possible).  If you try to describe the purpose of a class and you use any conjunctions, your class is probably defective because it is doing too much work in too many different ways.  When you find that an entire web page can be rendered by calling upon a single class, you've found Godclass.  The couplings and dependencies of the classes that extend Godclass are not readily visible, making it hard to know what changes will have adverse effects.  The cure for Godclass is isolation of data and functionality, so that objects know "all about" themselves, but treat other objects as if they are "black box" vending machines.  In other words, "minimize class responsibilities."


So much for antiPHPatterns.  Now let's look at AntiPHPractices.


AntiPHPractices

AntiPatterns are the high-level concepts gone awry.  More plentiful and finer grained blunders and code smells are called AntiPractices.  If it seems like this article has a negative, "don't do that" tone, it's because the article is intended to steer you away from risky or dangerous practices!  If you find these things in your own programming, get rid of them as soon as you get a chance.


PHP is an old programming language.  There are a lot of things that were designed into the language in the 1990's.  That was before hacking, spam, viruses, and other security problems were even beginning to be understood.  As a result, many of the code smells relate to things that seemed convenient at the time, but turned out to be dangerous or wasteful in retrospect.  Unfortunately, bad code examples do not come with expiration dates, and some of them (well, millions of them) are still published on the internet.  They are easy to find and they get copied over and over by novice programmers, perpetuating the bad practices.  If you find a code sample on the internet ask yourself, "When was this written?"  Also, ask yourself, "Do I completely understand what the author as thinking and how it relates to my problem?"  If you're not 100% sure about both of those answers, skip over the example and go look for one that makes more sense to you!  


These coding horrors are in no particular order, and they may not always cause scripts to fail, data bases to be destroyed, money to be lost, etc.  But they have that potential.


1. Installing the Code Without Understanding the Code

For some reason, people feel that they don't need to learn PHP before they start using it!  Unfortunately, the internet is littered with simply terrible examples of PHP code written long ago by novice programmers who did not understand the principles of computer science or security, and who never took the time to clean up after themselves.  And a particularly frequent offender, DreamWeaver, generates some of the worst PHP code ever written.  Copying code from internet resources or DreamWeaver is a sure way to get yourself in trouble.  So don't do that.  Instead stick to quality learning resources from dependable authors.  Like everything else in life, knowing what you're doing with PHP is really helpful.


Want a good test for understanding the code?  Try to say or write a one-sentence explanation for every line of code in your script.  If you can do that, you understand the code.  If you find yourself unsure or at a loss for words, stop what you're doing and look up the functions and classes on php.net.  A moment invested in learning can save you from days of debugging.  Further guidance on this point comes to us from Alexander Pope in his elegant Essay on Criticism.


2. Not Having a Backup Copy

Really?!  You made a change to a deployed production system?!


3. Not Having a Test Data Set

Really?!  You are about to make a change to a deployed production system?!  You might want to rethink that!


4. Programming Without Error_Reporting(E_ALL)

If you do not tell PHP to show you all the errors, you will miss some of the errors!  PHP will suppress the Notice messages unless you ask PHP to tell you the Notice messages.  Always ask any programming language to tell you all of the errors!


5. Suppressing Messages with @

Whenever there is a reason to suppress a warning message, there is an even greater reason to find out the cause of the message and eliminate it!  If I see the @ prepended to a function call, I expect to see the comments explaining why that is a good practice.  In almost every case it's not a good practice, but was an expedient way for a sloppy programmer to avoid a step in the debugging process.  It's significant to note that if you use @ to suppress messages and a Fatal Error occurs in the function, the script will fail silently.  There will be no message or log, and no indication of where the error occurred.  Imagine trying to debug something like that!


6. Register Globals

This takes the danger of Globalmania and sprays it randomly and universally into your script.  Disable Register Globals!  If you're at a current level of PHP, it should be disabled already.  Maybe you want to use phpinfo() to verify this.


7. Sloppy Syntax and Formatting

This is a programmatic shibboleth that screams, "I don't know what I'm doing!"  PHP allows you to write ugly code, very ugly code, and PHP will still interpret it, and if the code does not fail for a parse error, PHP will try to run it.  This is unfortunate, because it leads novice PHP programmers to think that coding standards do not matter, that they can use silly or meaningless variable names, that alignment and readability of the code isn't important.  Nothing could be further from the truth.  Neatness counts, and the reason for neatness and tidy organization is so that humans can make sense of your script.  Those humans include you, if you revisit the programming after a day or two away from the task.  Adopt a coding standard and adhere to it rigidly.  You probably want to adhere to PSR-1 and PSR-2.  With self-discipline comes self-confidence.


7a. Misusing Quotes and Apostrophes

Quote marks and apostrophes have special meanings in most programming languages and PHP is no exception, providing the novice with an almost endless number of ways to create parse errors with fiddly punctuation.  What do you need to know, when and how to use or avoid them?  See this article about Quotes in PHP Programing


8. Poorly Chosen Variable Names

What is the likely meaning of a variable named $x?  It's hard to guess!  If it's expected to be today's date, why not use $today instead?  And if you're retrieving rows from several different queries, beware of using $row.  It's just too easy to get confused.  Instead, strive for meaningful variable names that clearly identify your thought processes.  And in related matters, choose data base column names that you can readily understand and that do not conflict with SQL functions or reserved words.


9. Compound Statements

We've all seen them... Someone wants to look smart, so they nest a collection of function calls into a single-line statement.  The problem with doing this is pretty simple.  If you get an unexpected return value, you have to take the code apart to find out what screwed up.  Why not just start with the code on separate lines and save yourself a debugging cycle?  PHP does not run any faster when there are compound statements in the script!  PHP has to compile the script before it can be executed, and the compiled output is exactly the same, whether the statements were written legibly or all jammed up together into an unreadable mess.  PHP does not care, so take the easy approach -- the one humans are most likely to understand.  These two blocks of code do the same work.  The signature of compound statements are adjacent closing parentheses.

 

// NORMALIZE, CAPITALIZE, ESCAPE THE COLOR
$rgb = $mysqli->real_escape_string(ucfirst(strtolower(trim($color))));

// A CLEANER WAY TO NORMALIZE, CAPITALIZE, ESCAPE THE COLOR
$rgb = trim($color);        // REMOVE WHITESPACE
$rgb = strtolower($rgb);    // MAKE ALL LOWER CASE
$rgb = ucfirst($rgb);       // CAPITALIZE FIRST LETTER
$rgb = $mysqli->real_escape_string($rgb);


9.a Compound Arguments

These are closely related to compound statements, and they create a similar problem, to wit, you cannot readily see what data was passed in the argument string.  Consider this example:

 

$res= $mysqli->query($sql2  . $start . ', ' . $limit); 

What does the query string contain?  Who knows?  You would have to print out three variables to begin to figure it out.  A better ways to write the code would look something like this:

 

$sql = $sql2  . $start . ', ' . $limit;
print_r($sql);
$res = $mysqli->query($sql); 


10. Using REGEX to Do Things That PHP Should Do for You

I came across this statement in a client's code recently:

 

$regexp="/^[a-z0-9]+([_\\.-][a-z0-9]+)*@([a-z0-9]+([\.-][a-z0-9]+)*)+\\.[a-z]{2,}$/i";

Maybe that made sense (to validate an email address) many years ago, but we've had PHP filter_var() since PHP 5.2.  Regular expressions are very hard to get right!  And they are even harder to get right if they're strung together in a long, uncommented string.  Experienced programmers have a joke that goes, "I had 99 problems, so I used a Regular Expression.  Now I have 100 problems!"


11. Writing Code to Do Things That PHP Should Do for You

You wouldn't believe how many novice programmers do not understand the way date() and strtotime() work together.  Instead of using the built-in functionality, they try to write their own date computation algorithms, and the resulting code is almost always overly complicated and usually wrong in the edge cases, like leap year, daylight savings time, changing time zones, etc.  This is not the only place that PHP built-ins get ignored, but it's fairly common.  For another example, see the list of array functions for a collection of things that novices overlook or misunderstand.


12. Using the Wrong Tools

"Please bring me a wrench," said the carpenter.  


"What kind of wrench" said the apprentice, "Pipe wrench, lock wrench, spanner wrench, monkey wrench, Stillsons wrench...?


"Doesn't matter -- I'm just going to use it to pound nails."


File systems and data bases are different kinds of storage, and understanding the differences is an important part of application development.  Have you ever seen anyone store image files in a data base BLOB?  You won't see them doing it for very long, because as the image collection grows the data base will become slower and slower, impossible to back up, and generally an impediment to application deployment.  If you start down this path, expect to find yourself refactoring (or maybe someone else will be refactoring because you will get fired).  Image files (and by extension, sound or multimedia files) belong in the server file system.  The associated data base information should contain the URL of the image, not the image file.


The flip side of this misstep is seen in applications where some well-intentioned but misinformed designer created a flat text file or an XML document so he could store his files without using a data base.  Usually this happens when the designer is trying to avoid learning what a data base does.  The problem with this approach is not performance, but functionality.  XML is by its nature hierarchical.  A data base is relational.  The first time you have to find the relationships in an XML document, you will wish you had learned about data bases instead!


13. Failure to Put Comments Into the Code

It doesn't always have to be a doc-block, but there should be something to tell people what you're thinking and what you're trying to do!  And a doc-block never hurts.  It can provide tips and code hinting in some IDEs.


14. Using the ?> Close-PHP Tag

This tag is almost invariably followed by an invisible whitespace end-of-line character, which is browser output.  It can and will cause more trouble than it's worth.  Especially because it is invisible!  Unless you absolutely require the close PHP tag, omit it.  Really, PHP knows when it's finished!  Even if DreamWeaver doesn't!


15. Using the Global Keyword

This is really only needed when the application designer hasn't given enough thought to the organization of the data and classes.  It's a signature of disorganized thinking.  Avoid global variables.  Pass parameters and return values instead.


16.$_REQUEST

Avoid using the $_REQUEST variable.  The reason for this is simple: you do not know where the request data came from!  Was it from your HTML form?  Or did a malicious person simply type the request variables into the URL?  If you choose $_POST and $_GET you at least know the request method.  It does not protect you from hack attacks, but it makes them a little harder to perpetrate.


Another reason to avoid $_REQUEST is because of the request_order directive.  This (positively stupid) configuration variable can cause the same script to produce different outputs if the directive is changed.  Why did PHP introduce this stumbling block at 5.3?  Your guess is as good as mine!


17.Extract()

Like the use of register_globals, any programming that injects unnecessary variables into the symbol table has a bad code smell.  The wise programmer avoids variable proliferation.


18. Writing Your Own Version of Extract()

You may have seen something like this:

 

$u = $_POST["username"];
$p = $_POST["password"];

This accomplishes nothing.  It just copies one variable to another, creating two versions of the same data.  Perhaps the programmer forgot about filter_var()?  It also introduces a subtle psychological element of trust that is completely misplaced.


$u = $_POST['username'] only copies the contents of $_POST['username'] into the variable named $u.  Whatever was present, good or bad, in the original POST request variable is now "conveniently" addressable inside the shorthand $u.  The act of including the unfiltered attack vector in a more convenient variable name is one big part of this anti-practice, because it seems to encourage the use of shorthand $u instead of the longhand and more explicit $_POST['username'].  It seems to me that $_POST is a highly recognizable external variable, and therefore readily identified as a tainted data source.  Not so, for $u, and it requires us to read the code and follow $u back to its origin before we can know whether to trust $u.  So if you're going to create a shorthand notation, filter and sanitize the data before assigning it to the shorthand variable. 


This anti-practice is an example of failure to sanitize external data, as shown below.


19. Failure to Filter/Sanitize External Data

Here is the classic screwup:

 

$sql = 'DELETE FROM myTable WHERE id = ' . $_GET['id'];

Do you have any idea how many rows will be deleted?  No, you do not.  Maybe you think it is only one row?  But there is no LIMIT clause.  Want to know how common this blunder is?  In November, 2013, Michelangelo Van Dam gave a presentation to the Washington DC PHP Users Group in which he searched GitHub for the intersection of mysql_query() and $_GET.  This is what we saw.GitHub SearchWhat if $_GET['id'] came from a URL that said: url.php?id=0+OR+1+=+1

The resulting query would say DELETE FROM myTable WHERE id = 0 OR 1 = 1 and the OR condition would apply to every row of myTable. Bang! You're Fired!  This anti-practice has been an object of ridicule for years.  It's as dangerous as driving on the wrong side of the road.  Yet for some odd reason, there are still programmers who have not heard of the issue.  See http://xkcd.com/327/


19.a Relying on JavaScript to Filter/Sanitize External Data

A variant on failing to filter external data is relying on client-side technologies to do the work that must be done on the server-side (see the relationship here).  JavaScript and jQuery are used to make a nice experience for your human client, but they provide no protection at all against an attack because the attackers will simply bypass the client-side niceties and will post toxic data directly into your application.  This is easily accomplished with cURL or fsockopen() and the attack can even be tailored to look like it's coming from a browser that was referred by the HTML form on your web site.  All external data is, by definition, tainted, full stop.  You must filter it with server-side tools, after it has been received on the server.


20. Failure to Escape External Data

The query says, "INSERT INTO myTable (name) VALUES ({$_POST['name']}" and that should work, right?  Sure, until you find someone named O'Reilly.  Then you have a stray apostrophe in your query string, and that's a recipe for failure.  Apostrophes have a special meaning in query strings, unless you escape them.


21. Double Escape of External Data

Consider the PHP environment is set up with Magic Quotes, and the programmer, not sure about this, uses addslashes() or similar functions to escape the data.  As a result the data base is cluttered with double backslashes.  A common symptom of this misstep is code that uses stripslashes() on the fields that are drawn from the data base.


22. Failure to Escape Client Output

Envision a script reads an input field and stores it in the data base, then later it echoes the data out to a client browser.  What could possibly go wrong?  Well, the input field could have been poisoned with toxic JavaScript that will cause the client browser to begin an action that is harmful to the client computer and eventually to the client human.  When you see echo statements without htmlspecialchars(), you have to wonder...


23. Uninitialized Variables

http://xkcd.com/1193/ shows what can go wrong if you don't know what a variable contains.  Unfortunately PHP suppresses Notice level messages by default.  So novice programmers think it is OK to use uninitialized variables to mean FALSE or zero or an empty string, because that level of offense only rises to the level of a PHP Notice.  You can get away with this right up until the time that your manager wants to make a small change to your script and she chooses the variable name you were assuming would be empty or set with a value you expected.  If you raise the error_reporting() level to show the Notice messages, PHP will tell you about this pitfall.


24. Failure to Test the Return Values from PHP Functions

You wouldn't believe how many times the same programming error occurs, over and over, because something like this gets copied and perpetrated again and again.

 

$res = mysql_query($sql);
while ($row = mysql_fetch_assoc($res)) { ...

What's wrong here is the assumption that the return value in $res is a query result resource.  It might not be!  It might be FALSE.  If the program does not test for this condition, the script will eventually fail silently or with data damage.  MySQL is not a black box.  It can and will fail for reasons that are outside of the PHP programmers' control.  If your script cannot control something, it must test for the uncontrollable condition (in this case, query failure) and handle the conditions as they arise.


25. Wasting Time Not Knowing What Happened

Script failed, no output, what's wrong?  Well, the first thing to do is start creating output!  Use var_dump() to print out the inputs your script is receiving.  And to print out the intermediate variables.  And to visualize the data before the output phase.


25.a Wasting Other People's Time Not Knowing What Happened

It's amazing how many questions are posted here at EE with a code example and the question, "what's wrong?"  The code is useless; we already know it doesn't work.  What we really need to see instead is the data.  If your data takes the form of a return object (for example, from an API) and you post the output of print_r() or var_dump() we can read it, but it's unwieldy when we want to write some code that works with the data.  PHP has a perfect tool for solving this problem.  It's a built-in function, var_export().  Look it up, learn it now, test it out, and please use it when you want to show a colleague your test data.  And since PHP is still a "living language" with some things that are only half-baked, you may also want to read this note.  Var_export() may need help to work with StdClass.

http://www.php.net/manual/en/reserved.classes.php#113971


26. Wasting Time on Meaningless Optimization

Not limited to PHP by any means, but PHP programmers seem particularly interested in minutiae like whether it is faster to iterate over an array or an object.  The answer is, "it doesn't matter."  These kinds of pursuits are like milking a mouse.  No matter how much effort you put into the process, you will not get much.

http://xkcd.com/1205/


27. Overlooking Meaningful Optimization

Data transfers via disk I/O operations are several orders of magnitude slower than in-memory data transfers, and when there is a performance problem in a web application it is usually found in the data base.*  You need to know how your database works and how to optimize the data base and script file structure.  Here are some topical questions to ask yourself.


Are your scripts using something like MySQL_Fetch_Array()?  Perhaps you copied that from one of the many bad PHP examples that litter the internet?  Change that function to a variant of Fetch_Assoc() or better yet, Fetch_Object().  The Fetch_Array() variant retrieves twice as much data as is needed, making it the least efficient way to retrieve your data.


Do you have a SELECT * query?  If so, eliminate the * and put in the names of the columns you want to retrieve.  SELECT * retrieves all of the columns, even the ones you do not need, making it the least efficient way to retrieve data.


Do you have a SELECT query that is expected to retrieve one row (such as a user-id lookup or a single inventory item)?  If so, be sure you have LIMIT 1 in the query.  Failure to LIMIT the query will result in a table scan, making it the least efficient way to find the information.


Does your code loop around to add values as it retrieves the results set from the MySQL server? Maybe you should be using server side aggregations with a GROUP BY clause.


Are your data base tables appropriately indexed?  You may want to have an index on every column used in WHERE, ORDER, GROUP, JOIN and HAVING clauses.


Are your queries sargable?


Have you used EXPLAIN SELECT to optimize queries that touch more than one table?


If you recognize some of these issues in your queries, or more importantly, don't understand some of them, you might want to read this excellent article by our EE colleague, gr8gonzo.  Entire books are devoted to MySQL and optimization, especially at large scale, is a highly technical and detailed endeavor.  But every programmer should have an understanding of the basics.


* Or sometimes in the API calls to other applications with slow data base queries!


28. Skipping Best Practices

You really want to just get it done fast, right?  See http://xkcd.com/292/  The cartoon is a joke, but the best practices are not.  They are the way experienced programmers have learned to get the best and fastest results.


29. Manipulative Programming

(Added in 2016) Undoubtedly you've been in a conversation with a technical manager who has no sense of marketing, and something like this has come up: "How can we force the users to ___ (fill in the blank)?"  The answer is not found in technical details; it's found in human nature and common sense.  If your clients feel like they are being coerced, they will go away, and that is that.  One obvious manifestation of this anti-practice is demanding that clients fill out a registration form before they can see free information, or refusing to accept forms until they are complete, or making a web site that fails if it cannot set HTTP cookies.  Don't do things that drive customers away!  Instead, be as open and informative and inviting as you can be.  You can bet your competitors are trying to be nice to your clients.  Don't lose your customers over a design flaw.


Summary

Knowing what to do is important, and so is knowing what not to do.  Do you have some examples of wrongheaded designs and practices?  Please share them in the article comments below.  If we're smart about things, we can learn to avoid the AntiPHPractices, choosing best practices instead.


Please give us your feedback!

If you found this article helpful, please click the "thumb's up" button below. Doing so lets the E-E community know what is valuable for E-E members and helps provide direction for future articles.  If you have questions or comments, please add them.  Thanks!

 

14
Comment
Author:Ray Paseur
[X]
Welcome to Experts Exchange

Add your voice to the tech community where 5M+ people just like you are talking about what matters.

  • Help others & share knowledge
  • Earn cash & points
  • Learn & ask questions
  • 6
  • 4
  • 4
  • +6
20 Comments
 
LVL 38

Expert Comment

by:lherrou
Great tips! I voted Yes above - this IS a helpful article.
0
 
LVL 110

Author Comment

by:Ray Paseur
Code smells are the appearance of something about the code that indicates the probability of underlying flaws.  While reading about code smells this morning, I came across these very well-written commentaries.

Guidance:
http://www.codinghorror.com/blog/2006/05/code-smells.html
http://www.soberit.hut.fi/mmantyla/BadCodeSmellsTaxonomy.htm
http://c2.com/xp/CodeSmell.html
https://wiki.engr.illinois.edu/display/cs242sp13/Code+Smells

Humor:
http://c2.com/cgi/wiki?CodeStench
http://c2.com/cgi/wiki?CodeDeodorant
http://thc.org/root/phun/unmaintain.html
0
 
LVL 53

Expert Comment

by:Scott Fell, EE MVE
Very Helpful Ray and takeaways for any language.
0
PeopleSoft Has Never Been Easier

PeopleSoft Adoption Made Smooth & Simple!

On-The-Job Training Is made Intuitive & Easy With WalkMe's On-Screen Guidance Tool.  Claim Your Free WalkMe Account Now

 
LVL 1

Expert Comment

by:tjyoung
Awesome notes to follow. Have bookmarked for closer review... over and over.
Thanks Ray, appreciate your help over the past couple years.
tj
0
 
LVL 27

Expert Comment

by:skullnobrains
hi ray. quite an interesting article.

though i don't agree with everything, you have good arguments for each of your claim and i guess very few people would not gain something from this reading.

i just wanted to point out one thing :


Do you have a SELECT query that is expected to retrieve one row (such as a user+password lookup or a single inventory item)?  If so, be sure you have LIMIT 1 in the query.  Failure to LIMIT the query will result in a table scan, making it the least efficient way to find the information.

actually, i see this as a "code smell" for two reasons
- if it does make a difference in performance, then you most likely did not create a necessary index.
- the database should be the one that determines that only 1 row should be retrieved. (using a primary key or unique index). your code's job in this situation would be to either trigger an error if more than one row is retrieved or handle multiple rows.
0
 
LVL 31

Expert Comment

by:Marco Gasi
Great article, Ray, and nice too! I'll keep it at a glance.
0
 
LVL 36

Expert Comment

by:Loganathan Natarajan
Good Article and get to know things.. Thanks Ray!!!
0
 
LVL 83

Expert Comment

by:Dave Baldwin
On items 9 and 9a you might want to make a note that compound statements and arguments do not make the code faster or better.  The PHP interpreter 'pre-compiles' those statements into something that PHP can use no matter whether you write them simply or in a compound statement.  You should write these things for People to understand because PHP already knows what to do with them.
0
 
LVL 110

Author Comment

by:Ray Paseur
@DaveBaldwin: Where's that "like" button when we need it?!
0
 
LVL 83

Expert Comment

by:Dave Baldwin
It's still in that Other place...
0
 
LVL 70

Expert Comment

by:Jason C. Levine
Alright, Mr. Dreamweaver hater :)

As the ranking DW Expert, I feel oddly compelled to attempt to defend DW's honor.

<?php

require_once('Connections/connSOMETHING.php'); 

if (!function_exists("GetSQLValueString")) {
function GetSQLValueString($theValue, $theType, $theDefinedValue = "", $theNotDefinedValue = "") 
{
  if (PHP_VERSION < 6) {
    $theValue = get_magic_quotes_gpc() ? stripslashes($theValue) : $theValue;
  }

  $theValue = function_exists("mysql_real_escape_string") ? mysql_real_escape_string($theValue) : mysql_escape_string($theValue);

  switch ($theType) {
    case "text":
      $theValue = ($theValue != "") ? "'" . $theValue . "'" : "NULL";
      break;    
    case "long":
    case "int":
      $theValue = ($theValue != "") ? intval($theValue) : "NULL";
      break;
    case "double":
      $theValue = ($theValue != "") ? doubleval($theValue) : "NULL";
      break;
    case "date":
      $theValue = ($theValue != "") ? "'" . $theValue . "'" : "NULL";
      break;
    case "defined":
      $theValue = ($theValue != "") ? $theDefinedValue : $theNotDefinedValue;
      break;
  }
  return $theValue;
}
}

$editFormAction = $_SERVER['PHP_SELF'];
if (isset($_SERVER['QUERY_STRING'])) {
  $editFormAction .= "?" . htmlentities($_SERVER['QUERY_STRING']);
}

if ((isset($_POST["MM_insert"])) && ($_POST["MM_insert"] == "form1")) {
  $insertSQL = sprintf("INSERT INTO some_table (something, somethingelse, something3, somethingfinal) VALUES (%s, %s, %s, %s)",
                       GetSQLValueString($_POST['apply_id'], "int"),
                       GetSQLValueString($_POST['app_id'], "int"),
                       GetSQLValueString($_POST['program_id'], "int"),
                       GetSQLValueString($_POST['dateapplied'], "text"));

  mysql_select_db($database_connSOMETHING, $connSOMETHING);
  $Result1 = mysql_query($insertSQL, $connSOMETHING) or die(mysql_error());
}
?>

Open in new window


The above is a plain vanilla, wizard-generated, Dreamweaver PHP Insert operation.  The only thing I did was to edit the connection, table, and field names to make it wholly generic.  

Why is the above so bad/evil?
0
 
LVL 83

Expert Comment

by:Dave Baldwin
It is generic 'clever' code.  Too much generic stuff that the programmer should know.  This is what I would do for the same thing:
<?php

require_once('Connections/connSOMETHING.php'); 
mysql_select_db($database_connSOMETHING, $connSOMETHING);

if (!isset($_POST['apply_id']))  $apply_id = ''; else $apply_id = $_POST['apply_id'];
if (!isset($_POST['app_id']))  $app_id = ''; else $app_id = $_POST['app_id'];
if (!isset($_POST['program_id']))  $program_id = ''; else $program_id = $_POST['program_id'];
if (!isset($_POST['dateapplied']))  $dateapplied = ''; else $dateapplied = $_POST['dateapplied'];

$apply_id2 = mysql_real_escape_string($apply_id);
$app_id2 = mysql_real_escape_string($app_id);
$program_id2 = mysql_real_escape_string($program_id);
$dateapplied2 = mysql_real_escape_string($dateapplied);

$insertSQL = "INSERT INTO some_table (apply_id, app_id, program_id, dateapplied) VALUES ('$apply_id2', '$app_id2', '$program_id2', '$dateapplied2')";

$Result1 = mysql_query($insertSQL, $connSOMETHING) or die(mysql_error());
}

?>

Open in new window

0
 
LVL 110

Author Comment

by:Ray Paseur
Jason, great question.  In a vacuum, it's not bad or evil, and undoubtedly the generated code worked correctly.  How much longer it will work is anybody's guess.  It seems to me that PHP is evolving and DreamWeaver is stuck in a back-level compatibility time warp.  Let's deconstruct the code it generated and then try to see what will happen to a PHP novice who believes that this code is a good learning example.  Here are my comments, interspersed into the code.
<?php

require_once('Connections/connSOMETHING.php'); 

// ALMOST CERTAINLY THIS FUNCTION DOES NOT EXIST THE FIRST TIME, AND DOES EXIST ALL THE OTHER TIMES
// IF THE FUNCTION DEFINITION WAS INCLUDED IN A "COMMON" SCRIPT THERE WOULD NOT BE A NEED FOR A
// RUN-TIME TEST TO SEE IF THE FUNCTION EXISTS
if (!function_exists("GetSQLValueString")) {

// MOST CODING STANDARDS WOULD RECOMMEND AN INDENT HERE
function GetSQLValueString($theValue, $theType, $theDefinedValue = "", $theNotDefinedValue = "") 
{
  // THE PHP VERSION WILL ALWAYS BE LESS THAN 6 SINCE PHP6 IS NOT BEING BUILT
  if (PHP_VERSION < 6) {
  
    // ALWAYS RETURNS FALSE BECAUSE MAGIC QUOTES FEATURE WAS REMOVED AT PHP5.4.
    $theValue = get_magic_quotes_gpc() ? stripslashes($theValue) : $theValue;
  }

  // MYSQL_ESCAPE_STRING() WAS DEPRECATED AT PHP5.3
  // MYSQL_REAL_ESCAPE_STRING() WAS DEPRECATED (ALONG WITH THE ENTIRE MYSQL EXTENSION) AT PHP5.5
  $theValue = function_exists("mysql_real_escape_string") ? mysql_real_escape_string($theValue) : mysql_escape_string($theValue);

  switch ($theType) {
    case "text":
      $theValue = ($theValue != "") ? "'" . $theValue . "'" : "NULL";
      break;    
    case "long":
    case "int":
      $theValue = ($theValue != "") ? intval($theValue) : "NULL";
      break;
    case "double":
      $theValue = ($theValue != "") ? doubleval($theValue) : "NULL";
      break;
    case "date":
      $theValue = ($theValue != "") ? "'" . $theValue . "'" : "NULL";
      break;
    case "defined":
      $theValue = ($theValue != "") ? $theDefinedValue : $theNotDefinedValue;
      break;
  }
  return $theValue;
}
// MOST CODING STANDARDS WOULD RECOMMEND AN INDENT HERE
}

// THIS IS SETTABLE BY THE REQUEST, AND IS THEREFORE TAINTED DATA
$editFormAction = $_SERVER['PHP_SELF'];

// WHY NOT USE 'REQUEST_URI' -- IF 'PHP_SELF' IS DEEMED DEPENDABLE, SO IS 'REQUEST_URI'
if (isset($_SERVER['QUERY_STRING'])) {
  $editFormAction .= "?" . htmlentities($_SERVER['QUERY_STRING']);
}

// WHERE IS THE "ELSE" CLAUSE FOR THIS CONDITIONAL?
if ((isset($_POST["MM_insert"])) && ($_POST["MM_insert"] == "form1")) {
  $insertSQL = sprintf("INSERT INTO some_table (something, somethingelse, something3, somethingfinal) VALUES (%s, %s, %s, %s)",
                       // FOUR FUNCTION CALLS EMBEDDED INTO THE CREATION OF THE QUERY STRING?
                       GetSQLValueString($_POST['apply_id'], "int"),
                       GetSQLValueString($_POST['app_id'], "int"),
                       GetSQLValueString($_POST['program_id'], "int"),
                       GetSQLValueString($_POST['dateapplied'], "text"));

  // IT'S VERY RARE TO SEE THIS NEEDED MORE THAN ONCE - ABSTRACT TO COMMON SCRIPT
  mysql_select_db($database_connSOMETHING, $connSOMETHING);
  
  // MYSQL EXTENSION IS DEPRECATED
  $Result1 = mysql_query($insertSQL, $connSOMETHING) 
  
  // USING DIE() INSTEAD OF TRIGGER_ERROR() MEANS THERE CAN NEVER BE AN ERROR HANDLER
  or die(mysql_error());
}
?>

Open in new window

What if you did not know PHP and took this code as a learning example?  Here are some things you might be led to believe:

It's OK to still be using MySQL many years after PHP announced its removal
Every script must test to see if GetSQLValueString() exists
We need to test the environment with get_magic_quotes_gpc()
Unit testing is unimportant (this design renders it impossible)
You have to select the database before every query
It's OK to ignore the return values from PHP functions
PHP if() statements without else clauses are normal and acceptable
PHP mySQL_Error() contains enough information to diagnose a failing query
PHP die() is an acceptable end to a program

Executive summary: DreamWeaver was a great solution many years ago for someone who did not understand computer science and was unwilling or unable to learn the principles of computer science.  But the whole industry has evolved rapidly since 1999, and even PHP has gotten an object model.  This is just a 20th-Century way of programming and we don't do it that way any more.  All DreamWeaver-generated PHP scripts will begin throwing errors soon, as hosting companies are forced to upgrade their PHP versions or face the loss of support.  PHP5.3 is not supported any more, not even for security fixes, and the pressure to keep your software platform current is a fairly big issue.

My thinking is that if you're going to write PHP code, you should try to understand PHP code.  This used to be quite a challenge - you would have been forced to read the online documentation, and it was technically dense reading.  Today there are many good learning resources to supplement the manual - books, courses, videos, seminars, conferences, etc.  There are some good learning resources available in this article for those new to PHP.
0
 
LVL 70

Expert Comment

by:Jason C. Levine
It is generic 'clever' code.  Too much generic stuff that the programmer should know.
My thinking is that if you're going to write PHP code, you should try to understand PHP code.

These are variations on many programmer's objections to DW and they almost completely miss the point.  Dreamweaver's whole selling point was "you don't need a programmer to deliver a web site with dynamic capabilities" and attempted to return control of the web stack to the designers and non-technical folks.  Whether or not it succeeded (it did for a long time) or if the code is overly generic (it has to be...DW doesn't know what version of PHP is running or if it is Apache or IIS or what extensions are present...it just has to work or Adobe would lose money) or overly clever isn't relevant. The people using Dreamweaver are not high-end programmers nor would a sizable percentage probably want to be referred to as programmers.  They sure as hell wouldn't refer to themselves as computer scientists and despite probably being willing and able to learn computer science...they won't for other, more significant reasons (time and money, of course).

Instead, these are the graphic artists, the slightly above-average technical employees of a small business, and all of the others who just needed a simple way to build a site and add some basic functionality to it.  They did not have the time to learn the "proper" way to program and they didn't have the budget to hire a "proper" programmer to do it for them the "right" way.  This is a trap that I often see professional programmer's fall into...they frequently let the perfect become the enemy of the good.  In my mind, any program that writes wizard-generated code has to fulfill three basic requirements:

1) The code has to do the job it is supposed to do
2) The code has to be reasonably secure
3) The generation of the code should be able to be done by a non-professional with little to no intervention from the professional

Dreamweaver code does this for Insert, Update, and Delete actions[1].

Whether or not the code is perfectly indented or written elegantly or even written to current best standards and practices (incidentally, this code is from DW CS 5.5 and is probably a good 5 years old at this point.  I stopped buying it before 6 came out and refuse to move to CC) is simply not relevant to the masses who plunk down their hard-earned money to use the software.  Thankfully (or not) the hoi polloi has been moving away from Dreamweaver due to the rise in and massive improvements of online web site builders (Wix, SquareSpace, etc) and the quantum leaps made by WordPress, Joomla, Drupal, et al on the CMS side of things.  Yes, this has replaced DW wizard-generated code with random mash-ups of themes, plugins, snippets and widgets and we all have seen the results of this here on EE.  But the flip side is that it is easier than ever for a non-technical person to get their content on the web and have the chance at the same level of exposure as a professionally-designed, programmed, and optimized site.  That's a hell of a trade-off and I'm happy that people can do this and willing to put up with the dumb or overreach-of-ability questions as a result.

On our side of things, we're absolutely evolving as our "bedrock" tools continue to get better and better.  Can you remember what life was like before backbone.js?  Or JQuery?  Hell, now that node.js is coming along nicely, maybe PHP itself will be on the way out in favor of the next great language or library.  I know several people wouldn't shed any tears. Well, maybe some would.

On to some rebuttals

It's OK to still be using MySQL many years after PHP announced its removal

Kind of my bad (and done to generate the argument).  I used a version of Dreamweaver at LEAST 2 versions and 5 years out of date from current because lazy.  Dreamweaver is actually removing the server behaviors from base installs as of current DW CC versions and if you want to use it to write PHP code, you're going have to download and install an extension.  The extensions should be up to date with the current stuff.

Every script must test to see if GetSQLValueString() exists

Well, yeah.  DW doesn't know where all of the different behaviors are being applied.  It has a page open in the editor, that page has a server behavior and we need to sanitize inputs.  You do want to sanitize inputs, right?

We need to test the environment with get_magic_quotes_gpc()

See first point re: versions.  At the time, definitely a mixed bag of PHP versions was in play.

Unit testing is unimportant (this design renders it impossible)

See point above about who is using DW and why.  My guess is less than 1% of them know what unit testing is and those who do?  They ain't using the built-in stuff...they're just writing code.

You have to select the database before every query

Fair point.  I didn't post the contents of connSomething.php but it uses mysql_pconnect() to reduce the load on the MySQL server.  Again, old and bad...I agree but Mom's Local Sandwich Shoppe generating 100 views a day isn't straining here.

It's OK to ignore the return values from PHP functions
PHP if() statements without else clauses are normal and acceptable
PHP mySQL_Error() contains enough information to diagnose a failing query
PHP die() is an acceptable end to a program

So this is the stuff that is much less defensible when we talk as peer to peer but again, most of the target audience using this code rarely, if ever, switched to code view to look at it.  When we (Experts) answer questions from people about PHP coding we should keep this in mind.  Yes, we need to point out when the code is bad or insecure or has a odor to it, but the people who are asking the questions are most likely not interested in being told to go learn how to program better.  They're doing the best they can with what they have.  We can guide them to the Golden Path of Truth and Righteousness or we scold them for being bad little programmers :)





[1] Curiously, DW never shipped a behavior to go from form submission to email.  If you wanted that, you would have to buy a premium extension from a third party (at $99 or so!), pay to have a developer come in and add the functionality to the behavior (which "breaks" Dreamweaver in an annoying, non-fatal way), or learn to do it the right way the first time.  Arguably, it's the anger over paying $500 for DW and then being extorted for at least another $100 or even way more to get the only real form function that most people need is what drove some of the move towards the CMS' and drove others to learn how to do it.  I'm certainly in the latter camp...my core competency prior to learning how to build web sites was databases.  I had to retrain myself from the ground up...first on HTML, then Perl, then CSS, then PHP (first with then without DW) and I still know enough to know that I mostly suck at this :)

 In closing, Ray...Dave...I respect the heck out of both of you because you know your stuff and prove that time and time again.  I  just wanted to throw a little love at DW because it doesn't really deserve all of the scorn it gets.  Peace, brothers.
0
 
LVL 83

Expert Comment

by:Dave Baldwin
I understand your point, I just can't recommend using that code even to a non-programmer.  DW lets non-programmers create questionable HTML code as well.  I spend a fair amount of time fixing the messes my main client generates with DW.  Sometimes it's just kind of nasty code that won't validate and occasionally they manage to actually break the site.  Worse is they use 'apDiv's all over the place which breaks even a few HTML things.  You can't put anchors in between 'apDiv's.  Since 'apDiv' are absolutely positioned 'div', the browser has No idea what the anchor is referring to.

Part of the site is a PHP shopping cart that was not done in DW.  They tried to edit one of those pages a few months ago and broke that page because they don't understand code, they just move pictures around on the screen.  Any page that requires any active code like javascript or PHP, I have to edit and sometimes re-edit to keep it working.
0
 
LVL 83

Expert Comment

by:Dave Baldwin
One of the funnier things (to me) is the blocks DW will put in a page that prevent a DW user from editing that section.  I have been hired to edit those sections because the DW user couldn't figure out how to do it.  I would imagine that it's not hard to get around those rules but when I'm using a code editor instead of WYSIWYG editor, I can ignore them completely.

Of course, DW has never been as bad as Microsoft Front Page.  I got one site to redo and maintain because the code had gotten so out of whack that it could no longer be fixed in Front Page.
0
 
LVL 110

Author Comment

by:Ray Paseur
DW has never been as bad as Microsoft Front Page
Totally understand expedient, and DW was a good, expedient, tool for its time.  In the age of "brochurewear" when web publishing was a push-only mechanism, DW made sense.  It gave the non-professional a way to get semi-professional results.  I don't have any problem with that.  But I stand by my original statement, "Copying code [that you don't understand] from internet resources or DreamWeaver is a sure way to get yourself in trouble.  So don't do that."

The reason I advise against DreamWeaver as a source of PHP knowledge is simply that I have not seen good outcomes when people try to learn PHP by reading Dreamweaver-generated PHP code.  They learn too little.  And they are usually in a hurry, so they get stuck in this kind of situation.  Sometimes the right answer is "be respectful of your time and hire a professional."  Or as the great fire fighter Red Adair famously put it, "If you think it's expensive to hire a professional to do the job, wait until you hire an amateur."

DreamWeaver is by no means the only culprit.  The internet is littered with bad code examples.  They are equally troublesome when they make their way into a deployment (how is it that Joomla extensions seem so prone to hacking?)

Technology is always advancing.  Some future day we will probably be having this conversation about the shortcomings of Laravel  or Node ;-)
0
 
LVL 70

Expert Comment

by:Jason C. Levine
Dave,

Dreamweaver Templates are to blame for "non-editable" code but that only stops Dreamweaver from editing it, and even then only for about three clicks (Edit | Modify Templates | Detach from Template).  The idea behind it was to give a designer/dev the ability to lock down all but the content area of a site so the real non-technical folks could make edits without blowing away headers/footers/menus etc.  

What got people into trouble was that when an exception to the rule popped up, they didn't know to create a new child template and instead tried to edit the non-editable areas and the whole thing rapidly went to heck.

As far as apDiv and poor code goes, DW wrote perfectly good code but anything that writes code can be manipulated into writing bad code (bringing us back to Ray's overall point) because power without knowledge is a dangerous thing.
0
 
LVL 83

Expert Comment

by:Dave Baldwin
You're right, Jason.  And most of the code I see is written by people who are working by themselves and do not have 'designer/dev' to help them do any better.  They often don't understand what they have done.  Especially when it breaks.  Guess I shouldn't complain... it keeps me in business.
0
 
LVL 70

Expert Comment

by:Jason C. Levine
Guess I shouldn't complain... it keeps me in business.

Me, too.

Ray, great article and thanks for humoring me on the debate.  +1
0

Featured Post

Secure Your WordPress Site: 5 Essential Approaches

WordPress is the web's most popular CMS, but its dominance also makes it a target for attackers. Our eBook will show you how to:

Prevent costly exploits of core and plugin vulnerabilities
Repel automated attacks
Lock down your dashboard, secure your code, and protect your users

Join & Write a Comment

Learn how to match and substitute tagged data using PHP regular expressions. Demonstrated on Windows 7, but also applies to other operating systems. Demonstrated technique applies to PHP (all versions) and Firefox, but very similar techniques will w…
The viewer will learn how to create and use a small PHP class to apply a watermark to an image. This video shows the viewer the setup for the PHP watermark as well as important coding language. Continue to Part 2 to learn the core code used in creat…

Keep in touch with Experts Exchange

Tech news and trends delivered to your inbox every month