Smoking toooooo much PHP

Fixing someone else's code is frequently the most annoying job that anyone with some intellegence has to do. It's one of those, pay the rent, scream and run around jobs..

I'm sure other languages suffer as well, but thanks to PHP's learning curve, software written in the language often falls into 3 categories

• Half a clue (HAC)
  
• Reasonably worked out
• Over engineered

This week, unfortunatly, I had to attack both the HAC and the Over engineered, (not on the same project thankfully).

The problem with the HAC project was the end users had found a few minor bugs. (this was a PHP application that had been paid for by a client - kind of like a shareware type thing.). But luckly (or perhaps less luckly for me) they got the source code.

Bugs are a fact of life for all software, whether you find them when developing, or as an end user. There's bugs in all my software or features depending on how you look at it (see the bug list on pear.). But the difference between average and good programmers (IMO) is the ability to write code that someone else has a chance of understanding.  (and sending in bug fixes...hint hint....)

Untraceable code paths.
The first gripe I had with the HAC was the horrific use of random functions included from a wealth of files, making following the codepath next to impossible. (which file will this function come from? = grep again...)

The bug I had to solve was supposed to be quite simple the host name for an img src= tag, was pointing at the wrong server. (the application lives on a private development server running behind a reverse proxy, and hence the wrong host name)

Same code repeated in global scope
Like alot of PHP applications, this one had one script to respond to each url request (rather than a bootstrapper and class loader). The script then did the usual crap of loading config, libraries (bad joke there) etc. It then went down and had a big if block for each action.. (all in global scope). Apart from making the file rather large, it also would have made managing global changes to the site more complex. This is where this concept makes a world of difference.

class page_action extends applicaton_base { } 

Mixing PHP and HTML badly
Unfortunatly, no-where within that code was it immediatly clear where the image url or HTML was comming from. Yes another of those deadly sins, splattering HTML all over the PHP code,  resulting in some very large php scripts, which you have to look really hard to determine the what on earth is going on due to the excessive noise from the HTML..

HTML + Databases + $variables +  eval = trouble waiting to happen
When I finally tracked it down, (after grepping for a few functions that seemed possible candidates) I found that the tag was contained in a small snippet of HTML stored in the database. The HTML in the database went something like src="$site_base_url/$the_image_url". The author had decided that the wonderfully secure eval() function made a great dynamic layout tool.

Every variable should start somewhere
Now I knew what the variable was. The next task was to work out where it was comming from. A grep of the source indicated lots of instances of it being used, but not a single one creating it.

This is my pet gripe with $smarty->assignVal(), (and quite a few other template engines) you have another instance of data getting used with a great disconnect with the source of the original data.  Making backtracking code and locating where something occurred considerably more difficult. Let alone making security auditing next to impossible.

Global is almost as evil as eval()
Almost all the functions in the applications had a list of 20 or more global variables. This instantly made all the function completely un-re-usable. (it hard coded the implementation to the application). and goes back to the problem of tracking down where variables where created.

How may evil functions does PHP have - extract()
In the end, the culprit of this design was the wonderful function extract(), It had been used to convert the return array from a database query, and filled those wonderfull global variables. By making the result set array available globally the could would have been so much more readable, and it would only involve a small amount of extra typing (Copy and paste anyone). So after all that digging, It turned out you had to use the application interface to change that variable......

I guess in the end, this all goes back to the Microsoft philosopy, if we all made perfect software, then there would be no work for everyone fixing it. Although some of us would like to doing more with our lives ;)