• Zatapatique (unregistered)

    Replace $req000 with $reqFRIST and $req001 with $reqSECNOD.

    Problem solved.

  • (nodebb)

    ISTM that Brian got what he deserved: a total maintenance nightmare that was now his and his alone to try to keep alive while also adding to it.

    I don't know what they were paying him, but I'm highly confident it wasn't enough for the pain he volunteered to put himself through.

    Cheers, oh brave but foolish Brian!! This drink's to you!

  • kAlvaro (unregistered)

    This snippet includes almost every single bullet point of the unspoken legacy PHP style guide:

    • Assume URL parameters can never be wrong or missing.
    • Sequentially numbered variables to keep all single use queries.
    • SQL injection.
    • Count returned rows.
    • Queries in loops.

    And, no proof but no doubt, it was developed with error reporting disabled.

    The only thing it lacks is that isn't using the old mysql extension, but I bet that's because it was removed from the language many years ago and the hosting provider upgraded PHP.

  • (nodebb)

    Okay, it might seem shocking with some here but there is actually a legit reason why you want to do a by-record update over a batch update: Optimistic concurrency.

    Now obviously that is not the case here (because there's no concurrency token check in that SQL), but when you data is volatile, you always should check if your expected values are still around. Sure, you can early lock all affected records in a transaction, but that is poison in high-concurrency systems, so in this case selecting a bunch of records and then updating them record by record while checking for concurrency mismatches (by checking the amount of records affected by the UPDATE) makes actually sense.

    Obviously if you don't use optimistic concurrency or operate in a non-concurrent system state, it doesn't matter and you can happily do those batch UPDATEs. But keep in mind that when you use eager locking, the comfort always comes with a high cost that scales extremely badly.

  • Mike Sandbox (unregistered) in reply to WTFGuy

    That comment hit me... 20 years ago I was an industrial automation engineer with aspirations of moving to pure development and had a very similar opportunity pop up. The new application was not as much of a pile of excrement as this one, but we had no one who both knew how to code and also had the time to adopt another responsibility. We were dependent on the supplier, whose job it was to travel all over the world installing the system at other clients' locations, often out of email range for weeks at a time. It happened to be written in a language I had some familiarity with.

    Jumped on that opportunity (some would say grenade, but potato/po-tah-to) with both feet. After a few months of me doing the work, they ended up creating a position to own that system and had me write the job requirements for said position... suffice to say I got that job.

  • (nodebb) in reply to MaxiTB

    Are you seriously expecting that someone with the sort of epically bad attitude that the "code guru" had would actually take all that into account?

  • Naomi (unregistered) in reply to Steve_The_Cynic

    They explicitly said they weren't ("Now obviously that is not the case here").

  • Argle (unregistered)

    I love a happy ending.

  • John Smith (unregistered)

    You mean there's a boss somewhere that actually listened to his employee? Most of my bosses would have poopooed that SQL injection demo as unbelievable because no one would actually do that. I do work for state government though, and our politically appointed bosses aren't known for their intelligence.

  • (nodebb)

    Remy's rewrite isn't correct. It's updating all the rows that have the same idfiche as the given event ID. A more correct rewrite (assuming the DB is MySQL) would be

    UPDATE fiche AS f1
JOIN fiche AS f2 ON f1.idfiche = f2.idfiche
SET f1.idevent = NULL
WHERE f2.idevent = ?

    Addendum 2024-02-29 12:27: (Unless idfiche is unique, then Remy is right.)

  • Klimax (unregistered)

    Isn't that codeguru from certain small city in East Bohemia? I think I had a run in with this codeguru in question. Aggressively idiotic aggressive amateur? Check. Atrocious SQL Injection? Check. Slow as hell? Check. Small wonder there weren't threats of lawsuit...

  • (nodebb)

    Remy is talking about the first line or first 7 characters. I was dead when I saw the language - probably indicated by the file extension.

  • (nodebb) in reply to Steve_The_Cynic

    Nah, of course not.

    I just wanted to point out that iterating on the client side over a database query itself is actually the best way to have concurrency safety with least performance impact by using an optimistic concurrency pattern. And the nice thing about it, you can use the same token to ensure actor to service concurrency validation (for example by using etags for web services), giving you an E2E concurrency check.

  • (nodebb)

    I think you mean "oh $n0000".

  • (nodebb)

    this whole application was running on a potato-powered server stuffed in the network closet

    Ah, so the European news agency was the Irish Times?

  • MaxiTB (unregistered) in reply to zomgwtf

    I am Austrian and I also worked at/with news providers; what I have seen, it could be coming from all of them. So don't be to quick to claim Irish ownership ;-)

  • Airdrik (unregistered)

    I'm definitely with Remy on the variable names. I'm impressed (as in: I'm impressed how much of your foot you managed to shove into your mouth back there) that at least they had the forethought to use 3 digits for the numeric bit; ya know, just in case you need to reuse that same prefix more than 99 times. Please tell me that they are only doing this just in case, and that there isn't actually a case where they did use more than (or really anywhere close to) 99 variables with the same prefix.

Leave a comment on “A Few Updates”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article