struggling with some php

Discussion in 'PHP' started by ianhaney, Nov 22, 2013.

  1. #1
    Hi

    I got two issues I think

    I got a edit.php page and it has a form that should update a mysql record but it is not doing so

    If I don't select any active or inactive radio button, I get the following error

    Notice: Undefined index: status in /home/sites/irhwebsites.com/public_html/sites/sgr/admin/edit.php on line 18

    If I do select a active or inactive radio button, the form just resets itself and does nothing

    I have pasted the coding from the edit.php into pastebin and included the link below

    http://pastebin.com/zwqpGMw9

    just can't work out where it is going wrong

    Thank you in advance

    Ian
     
    ianhaney, Nov 22, 2013 IP
  2. PoPSiCLe

    PoPSiCLe Illustrious Member

    Messages:
    4,623
    Likes Received:
    725
    Best Answers:
    152
    Trophy Points:
    470
    #2
    Seriously, you need to filter those $_POST-variables before putting them into a query, mate. NEVER EVER use raw $_POST-variables, it be in a site, in a query, nowhere, please.
    That being said, if you assign each $_POST-variable to a similarily named $variable, and run it through mysql_real_escape_string(), it will help a bit (you shouldn't be using mysql_ extension, it's deprecated and should be avoided like the plague - use mysqli_ or PDO instead).

    All that being said, your query have a lot of this:
    
    "UPDATE sgrjobs SET ref='".$_POST['ref']."',
    role='".$_POST['role']."',
    contract='".$_POST['contract']."',
    
    PHP:
    which should be this:
    
    "UPDATE sgrjobs SET ref='$ref',role='$role',contract='$contract',
    
    PHP:
    and of course you need to do something like this first:
    
    $ref = mysql_real_escape_string($_POST['ref']);
    $role = mysql_real_escape_string($_POST['role']);
    $contract = mysql_real_escape_string($_POST['contract']);
    
    PHP:
    But you NEED to fix that code. It's a veritable sinkhole of security issues.
     
    PoPSiCLe, Nov 22, 2013 IP
  3. deathshadow

    deathshadow Acclaimed Member

    Messages:
    9,732
    Likes Received:
    1,999
    Best Answers:
    253
    Trophy Points:
    515
    #3
    Or to drag it kicking and screaming into THIS century, STOP using mysql_ functions. We've been told for EIGHT YEARS to stop using those (which is why nobody has) to use mySQLi or PDO instead -- which would give you prepared queries and auto-sanitation removing that worry. They've even added giant red warning boxes about it, yet people still have their craniums superglued to the inside of 1997's rectum on things like this. Outdated PHP methodology with outdated markup practices -- hardly a shock most development teams end up a human centipede knockoff. "Feed her, FEED HER!"

    I'd look closer at this, but the late '90's markup and train wreck of outdated code means IMHO whatever it is you're trying to do here, it needs to be thrown out completely and started over with MODERN techniques. The use of mysql_ and tables for layout being the tip of the iceberg when there's incomplete/inaccessible forms, opening and closing of PHP for no fathomable reason, incomplete value checking, etc, etc...
     
    deathshadow, Nov 22, 2013 IP
    ryan_uk, NetStar and nico_swd like this.
  4. Darkhodge

    Darkhodge Well-Known Member

    Messages:
    2,111
    Likes Received:
    76
    Best Answers:
    1
    Trophy Points:
    185
    #4
    The guys above me definitely have a point, so you really should look into using Mysqli or PDO. Failing that, at least look into escaping your values before using them in your queries.

    Anyhow try out the code below. I haven't tested it as it's 3:23am and I'm uber tired but it'll give you something to work with. You'll need to set the values for the variables at the top and also change the action of the form to the current page if it's not "edit.php".

    In addition to the points the guys above me made, you should start indenting to make your code easier to read. Also don't use <br> for spacing. You can use CSS instead.

    
    <?php
    
    // Database details
    $dbHost = 'localhost';
    $dbName = 'db_name':
    $dbUser = 'mysql_user';
    $dbPass = 'mysql_password';
    
    // Set up connection
    $conn = mysql_connect($dbHost, $dbUser, $dbPass);
    if (!$conn)
        die('Could not connect: ' . mysql_error());
    
    // Select database
    $db = mysql_select_db($dbName, $conn);
    
    // If post exists
    if($_POST){
    
        // Check for new values
        if(isset($_POST['ref']) && !empty($_POST['ref']))
            $newValues[] = "`ref` = '".mysql_real_escape_string($_POST['ref'], $conn)."'";
        
        if(isset($_POST['role']) && !empty($_POST['role']))
            $newValues[] = "`role` = '".mysql_real_escape_string($_POST['role'], $conn)."'";
        
        if(isset($_POST['contract']) && !empty($_POST['contract']))
            $newValues[] = "`contract` = '".mysql_real_escape_string($_POST['contract'], $conn)."'";
        
        if(isset($_POST['location']) && !empty($_POST['location']))
            $newValues[] = "`location` = '".mysql_real_escape_string($_POST['location'], $conn)."'";
        
        if(isset($_POST['salary']) && !empty($_POST['salary']))
            $newValues[] = "`salary` = '".mysql_real_escape_string($_POST['salary'], $conn)."'";
        
        if(isset($_POST['description']) && !empty($_POST['description']))
            $newValues[] = "`description` = '".mysql_real_escape_string($_POST['description'], $conn)."'";
        
        if(isset($_POST['addedby']) && !empty($_POST['addedby']))
            $newValues[] = "`addedby` = '".mysql_real_escape_string($_POST['addedby'], $conn)."'";
        
        if(isset($_POST['status']) && !empty($_POST['status']))
            $newValues[] = "`status` = '".mysql_real_escape_string($_POST['status'], $conn)."'";
    
        // If new values found and id is set & an integer
        if(isset($newValues) && isset($_POST['id']) && is_int($_POST['id'])){
    
            // Set up query
            // Note - There's no need to escape id as we've checked it's an integer above
            $sql = "UPDATE `sgrjobs` SET ".implode(",", $newValues)."    WHERE    `id` = $id LIMIT 1";
        
            // Run query
            $result = mysql_query($sql, $conn);
        
            if(!$result)
                die("Issue updating record.");
        
        }
    }
    
    // Set title
    $title = "Admin Edit Page";
    
    // Include header
    include ( 'includes/header.php' );
    
    // Grab existing record
    $q= mysql_query("SELECT * FROM `sgrjobs` WHERE id='".mysql_real_escape_string($_REQUEST['id'], $conn)."'") or die(mysql_error());
    $r = mysql_fetch_array($q);
    ?>
    </head>
    <body>
    <div id="container">
        <a href="index.php">Back to Jobs</a>
        <form action="edit.php" method="post">
            <table>
                <tr>
                    <td><label for="ref">Job Ref:</label></td>
                    <td><input type="text" name="ref" id="ref" value="<?php echo $r['ref']; ?>" /></td>
                    <td></td>
                </tr>
                <tr>
                    <td><label for="role">Job Role:</label></td>
                    <td><input type="text" name="role" id="role" value="<?php echo $r['role']; ?>" /></td>
                    <td></td>
                </tr>
                <tr>
                    <td><label for="contract">Contract:</label></td>
                    <td>
                        <select name="contract" id="contract">
                            <option value="Temporary" <?php echo ($r['contract'] == 'Temporary' ? 'selected' : '') ?>>Temporary</option>
                            <option value="Permenant" <?php echo ($r['contract'] == 'Permenant' ? 'selected' : '') ?>>Permenant</option>
                        </select>
                    </td>
                    <td></td>
                </tr>
                <tr>
                    <td><label for="location">Location:</label></td>
                    <td><input type="text" name="location" id="location" value="<?php echo $r['location']; ?>" /></td>
                    <td></td>
                </tr>
                <tr>
                    <td><label for="salary">Salary:</label></td>
                    <td><input type="text" name="salary" id="salary" value="<?php echo $r['salary']; ?>" /></td>
                    <td></td>
                </tr>
                <tr>
                    <td><label for="description">Description:</label></td>
                    <td><textarea name="description" id="description" cols="30" rows="6" /><?php echo $r['description']; ?></textarea></td>
                    <td></td>
                </tr>
                <tr>
                    <td><label for="addedby">Added By:</label></td>
                    <td><input type="text" name="addedby" value="<?php echo $r['addedby']; ?>" /></td>
                    <td></td>
                </tr>
                <tr>
                    <td>Status:</td>
                    <td>
                        <label for="statusActive">Active</label> <input type="radio" name="status" id="statusActive" value="active" <?php echo ($r['status'] == 'active' ? 'checked' : '') ?> />
                        <label for="statusInactive">Inactive</label> <input type="radio" name="status" id="statusInactive" value="inactive" <?php echo ($r['status'] == 'inactive' ? 'checked' : '') ?> />
                    </td>
                    <td></td>
                </tr>
                <tr>
                    <td></td>
                    <td>
                        <input type="hidden" name="id" value="<?php echo $r['id'] ?>" />
                        <input type="submit" name='submit' value="Submit" />
                    </td>
                    <td></td>
                </tr>
            </table>
        </form>
    </div>
    </body>
    </html>
    
    PHP:
     
    Last edited: Nov 23, 2013
    Darkhodge, Nov 23, 2013 IP