The little PHP script that couldn't



  • I was recently tasked with cleaning up a small PHP-powered web site that was running slowly. It's a simple site, designed for tracking shipments. Each shipment contains fields for who to ship to, where it's being shipped from, etc. Here's a screenshot:

     

     

    The boss tells me a bit more about the problem - within the local network it's quite fast, but once he gets home it can take several minutes to download the full list (about 100 shipments). My first thought was that there was a giant bitmap lurking around somewhere, but no such luck. It wasn't until I started reading through the code (divided into ~60 PHP files, one per function, named things like shipper_save.php and shipper_save_new.php) that I found the culprit.

    Each button was its own form.

     

    Literally, every button on that page was in a separate form. Well, technically none of them were actually in a form, but it was close enough for Internet Explorer. The end result was that every row was about 5k worth of markup. The solution, obviously, was to enable mod_deflate on the server. Here's a sample row, for your pleasure (lovely formatting from the original):

    <tr>
          <tr>
                <td class="item">
                26         </td>
                <td class="item" width="10">
                Fred         </td>
                <td class="item">
                Alice         </td>
                <td class="item">
                Fri Jun 08         </td>
                <td class="item">
                Jul 18         </td>
                <td class="item">
                     Jun 11         </td>
                <td class="item">
                NO         </td>



    <form name="pickMeUp" method="post" action=""></form>
                    <td class="item" align="center" valign="middle">
                      <input value="26" name="id" type="hidden">
                      <input value="Fred" name="recipient" type="hidden">
                      <input value="Alice" name="sender" type="hidden">
                      <input value="Fri Jun 08" name="due_date" type="hidden">
                      <input value="Jul 18" name="pickup" type="hidden">
                      <input value="Jun 11" name="deliver" type="hidden">
                      <input value="NO" name="finish" type="hidden">
                      <input value="Pick Up" class="pickup" onclick="this.form.submit()" type="button">
             </td>


    <form name="deliverMe" method="post" action=""></form>
                    <td class="item" align="center" valign="middle">
                      <input value="26" name="id" type="hidden">
                      <input value="Fred" name="recipient" type="hidden">
                      <input value="Alice" name="sender" type="hidden">
                      <input value="Fri Jun 08" name="due_date" type="hidden">
                      <input value="Jul 18" name="pickup" type="hidden">
                      <input value="Jun 11" name="deliver" type="hidden">
                      <input value="NO" name="finish" type="hidden">
                   <input value="Deliver" class="deliver" onclick="this.form.submit()" type="button">
             </td>


    <form name="finishMe" method="post" action=""></form>
                    <td class="item" align="center" valign="middle">
                      <input value="26" name="id" type="hidden">
                      <input value="Fred" name="recipient" type="hidden">
                      <input value="Alice" name="sender" type="hidden">
                      <input value="Fri Jun 08" name="due_date" type="hidden">
                      <input value="Jul 18" name="pickup" type="hidden">
                      <input value="Jun 11" name="deliver" type="hidden">
                      <input value="NO" name="finish" type="hidden">
                   <input value="Finish" class="finish" onclick="this.form.submit()" type="button">
             </td>   
           

    <form name="viewForm" method="post" action="case_log_view"></form>
                    <td class="item" align="center" valign="middle">
                      <input value="26" name="id" type="hidden">
                      <input value="Fred" name="recipient" type="hidden">
                      <input value="Alice" name="sender" type="hidden">
                      <input value="Fri Jun 08" name="due_date" type="hidden">
                      <input value="Jul 18" name="pickup" type="hidden">
                      <input value="Jun 11" name="deliver" type="hidden">
                      <input value="NO" name="finish" type="hidden">
                   <input value="View" class="view" onclick="doView(this, 26)" type="button">
             </td>           





    <form name="editForm" method="post" action="case_log_edit"></form>
                    <td class="item" align="center" valign="middle">
                      <input value="26" name="id" type="hidden">
                      <input value="Fred" name="recipient" type="hidden">
                      <input value="Alice" name="sender" type="hidden">
                      <input value="Fri Jun 08" name="due_date" type="hidden">
                      <input value="Jul 18" name="pickup" type="hidden">
                      <input value="Jun 11" name="deliver" type="hidden">
                      <input value="NO" name="finish" type="hidden">
                      <input value="Edit" class="edit" onclick="doEdit(this, 26)" type="button">
             </td>




    <form name="uploadForm" method="post" action="case_log_upload"></form>
                    <td class="item" align="center" valign="middle">
                      <input value="26" name="id" type="hidden">
                      <input value="Fred" name="recipient" type="hidden">
                      <input value="Alice" name="sender" type="hidden">
                      <input value="Fri Jun 08" name="due_date" type="hidden">
                      <input value="Jul 18" name="pickup" type="hidden">
                      <input value="Jun 11" name="deliver" type="hidden">
                      <input value="NO" name="finish" type="hidden">
                   <input value="Upload" class="upload" onclick="doUpload(this, 26)" type="button">
             </td>





    <form name="deleteMe" method="post" action=""></form>
                    <td class="item" align="center" valign="middle">
                      <input value="26" name="id" type="hidden">
                      <input value="Fred" name="recipient" type="hidden">
                      <input value="Alice" name="sender" type="hidden">
                      <input value="Fri Jun 08" name="due_date" type="hidden">
                      <input value="Jul 18" name="pickup" type="hidden">
                      <input value="Jun 11" name="deliver" type="hidden">
                      <input value="NO" name="finish" type="hidden">
                   <input value="Delete" class="delete" onclick="doDelete(this, 26)" type="button">
             </td>






          </tr>


  • Considered Harmful

    Each button depends on JavaScript anyway, so why not just call a function with the ID for the parameter and lose the needless forms?  Also, the only hidden field that should be necessary is the ID.  I'd hate to see the database code on the other side.



  • To be fair, the HTTP spec states that you should always use a POST if you are performing an action that changes data.  This is to stop aggressive forward-looking caches from deleting things (there was a good dailywtf a year or so ago about just this issue).

    However, things like "View" should be a normal href link.

    It doesn't excuse not using an id for the value however, so it's still a WTF

     



  • I think despite all the laughter and bashings they (often correctly) get around here, this would be a justified place for an AJAX application. First of all, you don't have the usual problem of a bazillion incompatible interfaces here, because the application is limited to a relatively small set of computers with predictable setup. You don't need to worry about search engine accessibility either. Finally, there is a lot of stuff that could be optimized: Granted, what that guy did was just crazy but if you think about it, there is no really efficient way to transmit the data using pure HTML: You always have to send the table data at least twice: One time for the visible table cells and another time for the hidden inputs.

    I think the best solution here would really be, store all of the data in a Java Script structure at the beginning, do as much as you can on the client side (like sorting) and send the necessary data back using XMLHTTP or a hidden form and HTTP No Content.



  • I probably would have exploited the Javascript expando properties on those HTML tags...

    <form name="pickMeUp" method="post" action=""></form>
                    <td class="item" align="center" valign="middle">
                      <input value="26" name="id" type="hidden">
                      <input value="Fred" name="recipient" type="hidden">
                      <input value="Alice" name="sender" type="hidden">
                      <input value="Fri Jun 08" name="due_date" type="hidden">
                      <input value="Jul 18" name="pickup" type="hidden">
                      <input value="Jun 11" name="deliver" type="hidden">
                      <input value="NO" name="finish" type="hidden">
                      <input value="Pick Up" class="pickup" onclick="this.form.submit()" type="button">

    would have been more like this:

    // -- somewhere at the top --//

    <script>
      function ParseAndSubmit(ob) {
        return DoSubmit(ob.id, ob.recipient, ob.sender, ob.duedate, ob.pickup, ob.deliver, ob.finish);
      }
      function DoSubmit(id, recipient, sender, duedate, pickup, deliver, finish) {
        theForm.id.value = id;
        theForm.recipient.value = recipient;
        theForm.sender.value = sender;
        theForm.due_date.value = duedate;
        theForm.pickup.value = pickup;
        theForm.deliver.value = deliver;
        theForm.finish.value = finish;
        theForm.submit();

        return;
      }
    </script>

    // -- snip -- //

    <td class="item" align="center" valign="middle" id="26" recipient="Fred" sender="Alice" duedate="Fri Jun 08" pickup="Jul 18" deliver="Jun 11" finish="NO" onclick="return ParseAndSubmit(this);">



  • Why not just use one form per row of data and in the form include all of the buttons with a value attribute.  Then on the server just check to see what button was pressed.  It'll come up in the post data.  Also the type attribute on the button input tag should be changed to submit.  Its a wtf on its own to have a regular button call javascript to submit a form.  I noticed that some of the forms go to different pages.  If you don't want to take the time to make some kind of dispatching page you could use javascript to change the action on the form to the proper action according to what button was pressed.  Something like onclick="this.form.action='case_log_upload'; this.form.submit();".  Keep the buttons as button types instead of submits then or you'll get double postings.

     



  • While AJAX would do the trick here nicely, this can be solved with straight HTML (and a good amount of thought in the processing page. It could even be done with a single form.

     
    To go with the one form per row concept, you start a new form on every row, with the action being the same for all (or maybe you could pass the row ID as a URL parameter) then you have 7 submit buttons. Each submit button has the same name (perhaps 'action') and the various values as needed ('Pick Up', 'Deliver', etc)

    When the user clicks on a button, the form is submitted, the processing page gets the row ID (from either the URL or the hidden form element) and then checks to see what it should be doing by looking at the value of 'action'. Then processes accordingly.

     
    Straight-forward HTML with no Javascript and a minimum of markup. As for the one form per page idea, you would just have to append the row ID to either the name of the submit button, preferably in the form of 'action[$rowID]' so this way it shows up as an array. Then on the processing page, we can parse that out of the submitted data.

    
    foreach ($_POST['action'] as $id=>$action)
    {
       // Do whatever you want in here.
       // This will only really go through these lines once
    }
    So it's possible to do this with straight XHTML and some good PHP. 


  • @xiian said:

    While AJAX would do the trick here nicely, this can be solved with straight HTML (and a good amount of thought in the processing page. It could even be done with a single form.

     
    To go with the one form per row concept, you start a new form on every row, with the action being the same for all (or maybe you could pass the row ID as a URL parameter) then you have 7 submit buttons. Each submit button has the same name (perhaps 'action') and the various values as needed ('Pick Up', 'Deliver', etc)

    When the user clicks on a button, the form is submitted, the processing page gets the row ID (from either the URL or the hidden form element) and then checks to see what it should be doing by looking at the value of 'action'. Then processes accordingly.

     
    Straight-forward HTML with no Javascript and a minimum of markup. As for the one form per page idea, you would just have to append the row ID to either the name of the submit button, preferably in the form of 'action[$rowID]' so this way it shows up as an array. Then on the processing page, we can parse that out of the submitted data.

    
    foreach ($_POST['action'] as $id=>$action)
    {
       // Do whatever you want in here.
       // This will only really go through these lines once
    }
    So it's possible to do this with straight XHTML and some good PHP. 

    It may be easily possible on the server side but not on the client side. The problem is that due to HTML's usual awesome non-logic, you can't make a form per row. If you wanted to, you would need to put the <form> element between the <table> and <tr> or between the <tr> and <td> tags:

    (Method 1) 

    <table>
    <form action="handle.php?row=1">
    <tr>
    <td>
    Cell 1,1
    </td>
    <td>
    Cell 1,2
    </td>
    </tr>
    </form>
    <form action="handle.php?row=1">
    <tr>
    <td>
    Cell 2,1
    </td>
    <td>
    Cell 2,2
    </td>
    </tr>
    </form>
    </table>
    (Method 2) 
    <table>
    <tr>
    <form action="handle.php?row=1">
    <td>
    Cell 1,1
    </td>
    <td>
    Cell 1,2
    </td>
    </form>
    </tr>

    <tr>
    <form action="handle.php?row=2">
    <td>
    Cell 2,1
    </td>
    <td>
    Cell 2,2
    </td>
    </form>
    </tr>
    </table>

    Unfortunately both methods are invalid HTML (don't even start with XHTML), because only the special table handling tags may be between a <table> and a <td>.
    Basically, if you want your HTML still to validate, you have two choices: Wrap the form around the whole table or put one into each cell like our guy did. That's it.

    Of course validity isn't everything. It's just that you get into the realm of obscure browser bugs and undefined behavior that way...



  • There's always a way.  If you are really concerned about validation a nested table containing the form and the button in the last cell of the data?  Setting padding and margin styles should make it virtually unnoticeable.



  • Or, since the data in the table isn't editable anyways, you could just throw all hidden inputs out of the window, replace them with a single row identifier and stop sending the server information that he already knows.

    <edit>

    which joe.edwards posted already ages ago. I feel stupid now. 



  • Was I the only one to pick up on this... Check out where the form tag starts and ends...

        <form name="pickMeUp" method="post" action=""></form>
            <input value="26" name="id" type="hidden">
            <input value="Fred" name="recipient" type="hidden">
            <input value="Alice" name="sender" type="hidden">
            <input value="Fri Jun 08" name="due_date" type="hidden">
            <input value="Jul 18" name="pickup" type="hidden">
            <input value="Jun 11" name="deliver" type="hidden">
            <input value="NO" name="finish" type="hidden">
            <input value="Pick Up" class="pickup" onclick="this.form.submit()" type="button">
    

    So you have a form with no fields defined inside it, and a whole bunch of fields (which are repeated) which aren't associated with any forms.

    I have no idea how this thing works, but it certainly shouldn't do!!! 



  • @Samus said:

    Why not just use one form per row of data and in the form include all of the buttons with a value attribute.  Then on the server just check to see what button was pressed.  It'll come up in the post data.  Also the type attribute on the button input tag should be changed to submit.  Its a wtf on its own to have a regular button call javascript to submit a form.  I noticed that some of the forms go to different pages.  If you don't want to take the time to make some kind of dispatching page you could use javascript to change the action on the form to the proper action according to what button was pressed.  Something like onclick="this.form.action='case_log_upload'; this.form.submit();".  Keep the buttons as button types instead of submits then or you'll get double postings.

     

    You could take this idea a step further so that instead of having one form per row you simply have one form and the values of the submit buttons are created from a function name and a row id. If the server is receiving value strings of "edit132" or "dele456" then it should be easy to split out into function and row id and process as appropriate. Possibly not the best of solutions admittedly but is an improvement over hundreds of forms and circumvents the form/row problem already mentioned.



  • I for one will just sit back and appreciate the simple beauty of the OP's solution: mod_deflate.


Log in to reply