• po (unregistered)

    not that i agree with the way he builds those queries but simply executing queries without taking care of the errors shouldn't be that big of a problem, maybe the data isn't that important...

    although the code says "UPDATE software_install" so that code must be quite important :)

  • Bob (unregistered)

    echo mysql_error();
    header("Location: index.php");

    Okay, yeah, this is a WTF? (Beyond what is already described) If you look at the function header you see a line:
    Remember that header() must be called before any actual output is sent, either by normal HTML tags, blank lines in a file, or from PHP.

    So this code may error out every time, depending on the size of buffers and how echo with nothing is handled.

  • po (unregistered)

    and why the hell isn't he using the join-function :)

    ....set menus = '".join( ",", $_POST[ "menu" ], )."' where id = .....

  • Ron (unregistered)

    Seems like there's a security hole here.


    If $_POST is coming from a form post, he's trusting the contents too much.

    It's hard for me to find anything else wrong with the code because I'm so mesmerized by the beautiful colors.

  • Stephen Caldwell (unregistered)

    It is this kind of stuff that makes me just want to give up programming all together. I code in php primarily at the moment ( save me please ) and I for whatever masochistic reason help people out with php in #php on freenode. And some of the people who run through there are more than capable of coding monstrosities like the above. I squarely blame the "ease" of php to be it's downfall.

  • Dan (unregistered)

    Of course, PHP is designed for this sort of WTFage...
    The stupid "magic quotes" feature tries to work
    around morons who pass completely unvalidated user
    data to sql database routines

  • Jon (unregistered)

    Blame the developer, not the language..

  • Joshua (unregistered)

    Jon: Precisely.

    "Man this car is stupid. It let me drive straight into a tree!"

  • Charles (unregistered)

    > Blame the developer, not the language..

    But with PHP, it's so easy to blame the language. It is stupid by default, out of the box. It's also horribly inconsistent in a variety of ways.

    First, the language designers can't make up their friggin' minds. function_name, functionname, or FunctionName? You'll find BUILT IN FUNCTIONS following all three.

    Second, they've constantly added and removed critical functionality in both minor and major releases. We're having to tell our clients now that the next release of our product will only run on PHP 4.3 JUST so that we can avoid having to think about the twenty thousand previous releases in which SOMETHING is SLIGHTLY different that completely breaks our code.


    Third, every single web hosting provider uses a slightly different configuration. Some have magic quotes on. Some have safe mode on. Some have a basedir restriction. Some still keep register_globals enabled... and our code has to run EVERYWHERE, in every configuration, without thinking or complaining. Which is why there are two hundred lines of boilerplate code at the top of the main library, JUST TO CLEAN UP THE ENVIRONMENT.

    That, ladies and gentlemen, is a WTF.

    I was originally a Perl developer, and I still am when I can. The Perl version of this very same application has very few of these issues, because I know, for fact, that I'm writing for Perl 5.00503 and higher, that the code I write will work perfectly and flawlessly in Perl 5.8.3, in any configuration, on any web server, on any platform.


    All that being said... Yes, the code in today's example very clearly was produced by someone that had no clue what he was doing... unvalidated input, no error checking until it's too late, then discarding the error for all intents and purposes. Not to mention the fact that if he didn't turn on output buffering above, the header() function will do nothing other than complain about headers already being sent.

  • James Nobis (unregistered)

    Seeing sizeof drives me nuts. In php sizeof is an alias of count. For those that know c...sizeof returns the number of bytes a given structure takes NOT the length of an array. Though, in this code that is the least of concerns.


    1) header after an echo...I think the coder here wants to redirect to index after this code runs? A meta refresh or a javascript document.location is probably called for

    2) No user validation. Egads! Sql injection anyone? Maybe this is some code from phpnuke... ;)

    3) The logic of executing the same sql query twice (perhaps a different one each time)...This one has me completely stumped, what were they possibly trying for ?

  • Guayo (unregistered)

    It seems the code' author hasn't realized (or doesn't understand/know) he is updating the same row.
    So he could set the menu and complete fields in just one update.
    Of course it could be that a DB logic (trigger) needs a deferred update of menu and then of complete fields (but I think is doubtful).
    Anyway, the last call to mysql_query to re run the last update is just nuts.
    Not need to talk about layered architecture <flame type=”just jocking, the right tool for the job, yadah, yadah...”>after all, it's PHP</flame> but that's a little piece of code emphatically asking for a SQL injection attack.

  • Robert Brown (unregistered)

    I've never worked with PHP before, but won't that first SQL fail because it build an SQL string with a comma at the end?

  • Guayo (unregistered)

    @R.Brown
    Same background with PHP here.
    Do you see that little substr($sql, 0, -1)?... I think that's what makes the trick

  • Guayo (unregistered)

    ... not to mention the comma separated values are just set to a single field like:
    UPDATE software_install SET menus = ‘menu1, menu2, menu3'
    so there isn't a SQL syntax error here if there is a comma at the end of menu3 and before the single quote.

  • JoeJoe (unregistered)

    That code is also an SQL INJECTION just waiting to happen...

  • bje (unregistered)

    Isn't there somewhat of a WTF deserved for using an image to convey text, in this very article?

  • Dave Mays (unregistered)

    Nah, that just looks like a screenshot from EditPlus, so he could have the nice syntax hilighting.

    Ooh, the pretty colors. :-)

  • Thomas (unregistered)

    That for loop would be faster and cleaner if it was a foreach loop.

  • Marco (unregistered)

    Come on, it looks like an quick'n'dirty script.
    The two queries are UPDATE statement, if they are executed twice there will be no problem (since it's an installation speed is not a concern).
    And if one of those will produce an error, most probably, they will reproduce the error the second time they will be executed and that code will display that error without redirecting to another page.
    If everything goes well, no error will be showed and the page will be redirected to the index page.

    Now, I don't know php but this piece of code seems fairly reasonable for a quick hack.

    Regards,

    - mn

  • Dominic (unregistered)

    Charles: Agreed - You always need to factor in a surprising amount of time in your estimate with PHP for deployment & test issues. Those varying server configs are highly irritating (and any host that still uses register_globals=on should be disconnected. permanently.)

    The naming issue is a pain too - it's not just the function names but the argument order too, esp. when working with strings: is it foo( $needle, $haystack ) or the other way round?

    That said, it is possible to write excellent applications in PHP, as PHP 5 makes this a much more mature platform with decent OOP and XML support. As Jon said - it's the developer, not the language.

    Tip: Google for php design patterns - www.phppatterns.com

  • zeriod (unregistered)

    The header after the output is perfectly fine if there is some output buffering going on that we can't see

  • Jon (unregistered)

    "I was originally a Perl developer"

    I guessed that from the second paragraph =)

    Have you never installed anything on a nix system? 8)

  • Joni (unregistered)

    I have to say, this guys writing much worse code than I am. No commenting, no error handling. Nada. Zip. I hope he actully RTFMs from time to time :p

  • Ryan (unregistered)

    I think he just want's to see errors. Does a mysql_error without even checking the return value of mysql_query, before trying to issue a header (WTF doesn't apply if he is using Output Buffering, except considering it is pointless for that reason and lacks logic).

    It looks like a quick-hack-use-once script to me, otherwise, big WTF. Sadly, there is many PHP code like this (because it attracts lazy and unskilled developers). However, it is important to note that Perl programmers (and all programmers) are capable of producing equally obscure programs, especially when they try to golf every line. Charles makes valid points, but 200 lines of valid code IMHO shows a weakness on your side, so I will assume you are slightly over exaggerating.

Leave a comment on “$_PHP["logic"]”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article