• Bob the guest (unregistered)

    How old is this code? No one has used mysql_query in years.

  • (nodebb)

    Plot twist: It's within the last few years. You'd be surprised at the number of ignorant "developers" out there, and PHP probably attracts the absolut dregs.

  • (nodebb)

    I know the perpetrators of this code don't care, but it BUGS me when they mix ANSI joins with where-clause joins in the same stinking query.

  • Álvaro González (github)

    I stopped paying attention at the very beginning, where it splits file lines as array elements just to immediately concatenate them into a string.

  • Álvaro González (github) in reply to Bob the guest

    How old is this code? No one has used mysql_query in years.

    People keep saying it's "deprecated" when in fact it was entirely removed from the language years ago, so you can't even use it if you want...

    Unless... 😳

  • Argle (unregistered)

    I got into a short flame war with someone on YouTube who was singing the praises of a recent release of PHP. I pointed out that developers could still pull the same old crappy stunts with PHP that they always could. He rather smugly condemned me for not understanding "backward compatibility." At that point, I decided that backward compatibility isn't truly sacrosanct. Maybe -- just maybe -- vast chunks of PHP should be declared "deprecated" and maybe the programming world would be a better place.

  • (nodebb)

    Is there any way that:

    "' AND p.products_id IN(" . join(",", $_POST['directors1']) . ")"

    isn't an SQL injection waiting to happen ?

  • Álvaro González (github) in reply to Argle

    I pointed out that developers could still pull the same old crappy stunts with PHP that they always could.

    Honest question: how do other languages prevent that? (Aside from being too difficult to use so bad devs don't even care, which I assume isn't what you mean).

  • (nodebb)

    I R Disappoint.

    Remy, you missed the SQL injection vulnerability in this line of code found in the "first major block of code":

    $data_query = mysql_query("SELECT p.products_price, pd.products_name, pd.products_description, p.products_id, s.specials_new_products_price, p.products_image FROM " . TABLE_PRODUCTS . " p LEFT JOIN " . TABLE_SPECIALS . " s ON p.products_id = s.products_id, " . TABLE_PRODUCTS_DESCRIPTION . " pd WHERE p.products_id = pd.products_id AND pd.language_id = '" . $_SESSION['languages_id'] . "' AND p.products_id IN(" . join(",", $_POST['directors1']) . ")");

    Probably the $_SESSION['languages_id'] is OK, depending on exactly where the language ID came from, but the $_POST['directors1'] is (unless I'm being deceived by its name) sourced directly from the body of the POST request.

  • Argle (unregistered) in reply to Álvaro González

    I don't have a good answer to that question. I know that Fortran has evolved enough over the years to deprecate features, such as the arithmetic "if" statement. I just asked people who are more current than I am with Fortran about it. Apparently, deprecation merely results in warnings. And I know from observation that bad programmers tend to ignore warnings. I guess once people start using bad features of a bad language those features are here to stay. I'm in a position now to say that I won't maintain PHP code except under extreme duress and a lot of pay.

  • Seirios (unregistered)

    More disappointments in the SQL part: What is the point of having a variable / global constant for a table name where all other column names are hard coded in the query? So they can 'easily' create a copy of a table whenever a new year / season / catalogue comes out, adjust the data and the constant value and all works?

  • (nodebb)

    It's very 20 years ago PHP code. To some extent that's understandable, as it's sending HTML Email, but in so many other ways it's really not...

  • ismo (unregistered)

    The very first line has check for 'action' not being empty and then that it is 'send'. Why the empty check is needed ? Tells everthing about this developer.

    if(!empty($_GET['action']) && ($_GET['action'] == 'send') )

  • löchlein deluxe (unregistered) in reply to ismo

    I'm pondering my magic 8ball and it says: ops told them to fix the host of warnings about accessing undefined array members but never had time to double-check if they did.

  • Lutz (unregistered) in reply to ismo

    "Tells everthing about this developer."

    Yes - it tells that the developer doesn't want the script to crash if $_GET['action'] doesn't exist. You know? The potential bug that the !empty check prevents from happening...

  • (nodebb) in reply to Steve_The_Cynic

    Came to look for this exact reply.

    I saw it, my mind basically glossed over the rest of the article, I did a quick skim to see if Remy spotted it, he didn't, so didn't bother with the rest of the article and came to the comments instead :)

  • TheMonkey (unregistered) in reply to ismo

    "Why the empty check is needed ?"

    Because PHP will throw a warning if $_GET['action'] is not set and you check if it's equal to 'send'. So checking if it's !empty first will ensure it exists in the $_GET array and also is not null.

  • RLB (unregistered) in reply to Bob the guest

    How old is this code? No one has used mysql_query in years.

    I've used mysql_query not five years ago. In fact, it was I who made it my project to replace it with PDO and properly parameterised queries. My predecessor couldn't be bothered.

  • (nodebb) in reply to ray73864

    Likewise.

    I decided if that wasn't bad enough to be called out, then I really didn't want to the the rest. It's like if there's some sort of massive disaster in a sausage making factory and somebody says "you gotta see the carnage but then walks past a severed arm in the doorway without even mentioning it...

  • Airdrik (unregistered)

    I strongly suspect Remy didn't miss the obvious sql injection. He just left it there for people to find on their own (along with the host of other wtfs in the full chunk). I'd be surprised if any regulars to this site upon seeing that it was passing a string as a sql query didn't immediately scroll to the right expecting to see query parameters being concatenated directly, only to have our well-trained intuitions confirmed. The whole thing reminds me of the "Zipped up ziti with zucchini" in the garbage truck's alphabet soup recipe from my kid's book "I Stink": a bit of a mess, left in the fridge too long where it's been growing mold. Hopefully it's been thrown out with all the other abandoned leftovers.

  • Missing Semicolon (unregistered)

    TRWTF is surely building all this stuff in the front-end to begin with. Is back-end development that hard?

Leave a comment on “Don't Read This Email”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article