1

I have a table called user_bio. I have manually entered one row for username conor:

id: 1
age: 30
studying: Business
language: English
relationship_status: Single
username: conor
about_me: This is conor's bio.

A bio is unique to a user, and obviously, a user cannot manually set their bio from inserting it in the database. Consider the following scenario's:

  • Logged in as Conor. Since Conor already has a row in the database, I simply want to run an UPDATE query to update the field where username is equal to conor.
  • Logged in as Alice. Since Alice has no row in the database corresponding to her username. Then I want to run an INSERT query. For all users, all users will have to have their details inputted, and then updated correspondingly.

At the moment, I am struggling with inserting data in the database when no rows exist in the database.

Here is my current approach:

$about_me = htmlentities(trim(strip_tags(@$_POST['biotextarea'])));
$new_studying = htmlentities(trim(strip_tags(@$_POST['studying'])));
$new_lang = htmlentities(trim(strip_tags(@$_POST['lang'])));
$new_rel = htmlentities(strip_tags(@$_POST['rel']));

if(isset($_POST['update_data'])){
    // need to check if the username has data already in the db, if so, then we update the data, otherwise we insert data.
        $get_bio = mysqli_query($connect, "SELECT * FROM user_bio WHERE username ='$username'");
        $row_returned = mysqli_num_rows($get_bio);
            $get_row = mysqli_fetch_assoc ($get_bio);
                $u_name = $get_row['username'];

        if ($u_name == $username){
            $update_details_query = mysqli_query ($connect, "UPDATE user_bio SET studying ='$new_studying', language ='$new_lang', 
                                                        relationship_status = '$new_rel', about_me = '$about_me' WHERE username ='$username'");
                echo "  <div class='details_updated'>
                            <p> Details updated successfully! </p>
                        </div>";
        } else {    
            $insert_query = mysqli_query ($connect, "INSERT INTO user_bio 
                                                    VALUES ('', '$age','$new_studying','$new_lang','$new_rel', '$username', '$about_me'");                                                          
                mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
                echo "  <div class='details_updated'>
                            <p> Details added successfully! $row_returned </p>
                        </div>";
        }

}

The UPDATE query works fine, when logged in as Conor. But again, INSERT does not work when logged in as Alice.

Freddy
  • 683
  • 4
  • 35
  • 114
  • Why do you use mysql_ functions? They are deprecated for a long time now.... Also you should not inject user-provided values in your SQL strings. Replacing with HTML entities is no protection against SQL injection. – trincot Apr 08 '16 at 22:55
  • @trincot - Sorry, where have I used mysql functions? I can't seem to spot it, maybe im just blind aha, but I am using mysqli. And thanks for the advice! I'v in fact spent the last few hours amending my code to ensure it's more secure :) – Freddy Apr 08 '16 at 23:09
  • Sorry, I thought I saw a mysql_ in your code, but I must be mistaken. I retract that comment ;-) – trincot Apr 08 '16 at 23:27
  • Note that @s_mart answer is the way to go. Did you try it? It will cut your PHP code to 25% of what it is now. – trincot Apr 08 '16 at 23:28
  • @trincot - haha it's ok, we all make mistakes :) And truthfully, no. I have tried to use `ON DUPLICATE KEY ` in the past and it caused me a whole bundle of problems. I don't quite understand it (yes, even with all the documentation available). As of this prior experience, I tend to stay away from it. – Freddy Apr 08 '16 at 23:41
  • 1
    Ok, I still think you should revisit `ON DUPLICATE KEY`, but I have provided an answer transforming your code into an SQL-injection-safe version that also deals with the UPDATE/INSERT switch with one less query. – trincot Apr 09 '16 at 00:42

3 Answers3

4

MySQL support INSERT ... ON DUPLICATE KEY UPDATE type of queries. So you do not need to make few queries to check existance of row in your php code, just add corrct indexes and let your DB take care about this.

You can read about such type of queries here

s_mart
  • 735
  • 7
  • 21
2

Here are a few things you could do to make it work:

Prevent SQL injection

As this is an important issue, and the suggested corrections provided below depend on this point, I mention it as the first issue to fix:

You should use prepared statements instead of injecting user-provided data directly in SQL, as this makes your application vulnerable for SQL injection. Any dynamic arguments can be passed to the SQL engine aside from the SQL string, so that there is no injection happening.

Reduce the number of queries

You do not need to first query whether the user has already a bio record. You can perform the update immediately and then count the records that have been updated. If none, you can then issue the insert statement.

With the INSERT ... ON DUPLICATE KEY UPDATE Syntax, you could further reduce the remaining two queries to one. It would look like this (prepared):

INSERT INTO user_bio(age, studying, language,
                     relationship_status, username, about_me) 
         VALUES (?, ?, ?, ?, ?, ?)
  ON DUPLICATE KEY 
  UPDATE studying = VALUES(studying), 
         language = VALUES(language), 
         relationship_status = VALUES(relationship_status), 
         about_me = VALUES(about_me);

This works only if you have a unique key constraint on username (which you should have).

With this statement you'll benefit from having the data modification executed in one transaction.

Also take note of some considerations listed in the above mentioned documentation.

NB: As in comments you indicated that you prefer not to go with the ON DUPLICATE KEY UPDATE syntax, I will not use it in the suggested code below, but use the 2-query option. Still, I would suggest you give the ON DUPLICATE KEY UPDATE construct a go. The benefits are non-negligible.

Specify the columns you insert

Your INSERT statement might have failed because of:

  • the (empty) string value you provided for what might be an AUTO_INCREMENT key, in which case you get an error like:

    Incorrect integer value: '' for column 'id'

  • a missing column value, i.e. when there are more columns in the table than that you provided values for.

It is anyway better to specify explicitly the list of columns in an INSERT statement, and to not include the auto incremented column, like this:

INSERT INTO user_bio(age, studying, language,
                     relationship_status, username, about_me) 
VALUES (?, ?, ?, ?, ?, ?)

Make sure you get notified about errors

You might also have missed the above (or other) error, as you set your error reporting options only after having executed your queries. So execute that line before doing any query:

mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);

And also add there:

error_reporting(E_ALL);
ini_set('display_errors', 1);

In a production environment you should probably pay some more attention to solid error reporting, as you don't want to reveal technical information in the client in such an environment. But during development you should make sure that (unexpected) errors do not go unnoticed.

Do not store HTML entities in the database

It would be better not to store HTML entities in your database. They are specific to HTML, which your database should be independent of.

Instead, insert these entities (if needed) upon retrieval of the data. In the below code, I removed the calls to htmlentities, but you should then add them in code where you SELECT and display these values.

Separate view from model

This is a topic on its own, but you should avoid echo statements that are inter-weaved with your database access code. Putting status in variables instead of displaying them on the spot might be a first step in the right direction.

Suggested code

Here is some (untested) code which implements most of the above mentioned issues.

// Calls to htmlentities removed:
$about_me = trim(strip_tags(@$_POST['biotextarea']));
$new_studying = trim(strip_tags(@$_POST['studying']));
$new_lang = trim(strip_tags(@$_POST['lang']));
$new_rel = trim(strip_tags(@$_POST['rel']));
// Set the error reporting options at the start
mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
if (isset($_POST['update_data'])) {
    // Do not query for existence, but make the update immediately 
    $update_stmt = mysqli_prepare ($connect,
            "UPDATE user_bio 
             SET    studying = ?, 
                    language = ?, 
                    relationship_status = ?, 
                    about_me = ? 
             WHERE  username = ?");
    mysqli_stmt_bind_param($update_stmt, "sssss", 
            $new_studying, $new_lang, $new_rel, $about_me, $username);
    mysqli_stmt_execute($update_stmt);
    $num_updated_rows = mysqli_stmt_affected_rows($update_stmt);
    mysqli_stmt_close($update_stmt);
    if ($num_updated_rows === 0) {
        $insert_stmt = mysqli_prepare ($connect,
                "INSERT INTO user_bio(age, studying, language,
                             relationship_status, username, about_me) 
                 VALUES (?, ?, ?, ?, ?, ?)");
        mysqli_stmt_bind_param($insert_stmt, "isssss", 
                $age, $new_studying, $new_lang, $new_rel, $username, $about_me);
        mysqli_stmt_execute($insert_stmt);
        mysqli_stmt_close($insert_stmt);
    }
    // Separate section for output
    $result = $num_updated_rows ? "Details updated successfully!"
                                : "Details added successfully!";
    echo "  <div class='details_updated'><p>$result</p></div>";
}
trincot
  • 317,000
  • 35
  • 244
  • 286
  • Wow, a very informative and educating answer! I have tested your code above, again the `UPDATE` works perfectly, still no new row being generated if no row exists in the database for the `$username` trying to add their bio (INSERT not working). I have tried to implement the `ON DUPLICATE KEY` approach, but neither the UPDATE nor INSERT works with that approach. Also yes, I do have a UNIQUE key constraint on `username`. – Freddy Apr 09 '16 at 14:15
  • When you say that it doesn't work, what happens? Errors? Does the `echo` part display the "Details added successfully"? Could you add `error_reporting(E_ALL);` and `ini_set('display_errors', 1);` at the top to make sure all errors are reported? – trincot Apr 09 '16 at 14:20
  • So, I currently, I am logged in as Alice, who has no row in the `user_bio` table. When I click submit on the form where Alice is typing in her details, no echo is presented to me. No new row is generated in the database either. It echo's `Details updated successfully` for when UPDATING. – Freddy Apr 09 '16 at 14:27
  • Ok, so I have added the error_reporting. I now get the following errors: 1. `mysqli_stmt_bind_param(): Couldn't fetch mysqli_stmt` 2. ` Uncaught exception 'mysqli_sql_exception' with message 'No data supplied for parameters in prepared statement'` 3. `mysqli_sql_exception: No data supplied for parameters in prepared statement`. – Freddy Apr 09 '16 at 14:30
  • There was an error in my code with the **second** `mysqli_stmt_bind_param` call: it used the wrong statement variable. It must pass as first argument *$insert_stmt* (not *$update_stmt*). Please try again with the corrected version. The second error was a consequence of the first. – trincot Apr 09 '16 at 14:36
  • Still the same, UPDATE works, but INSERT does not. I now get three new errors: 1. `mysqli_stmt_bind_param(): Number of elements in type definition string doesn't match number of bind variables`. 2. ` Uncaught exception 'mysqli_sql_exception' with message 'No data supplied for parameters in prepared statement'` .3. ` mysqli_sql_exception: No data supplied for parameters in prepared statement `. Can I ask, what is the purpose of this data provided `issss` and `sssss` provided in the `mysqli_stmt_bind_param`? – Freddy Apr 09 '16 at 14:43
  • Again a mistake of mine: there should be one more *s* in *"isssss"*. I corrected my answer. There must be one character in that string per *?* in the SQL. You can read about this method in the [docs](http://php.net/manual/en/mysqli-stmt.bind-param.php) – trincot Apr 09 '16 at 14:55
  • Ahh, we finally got there! So I have tested it now, it now INSERT's and UPDATE's user bio's accordingly! Many thanks not just for the solution, but also for the educating answer :) – Freddy Apr 09 '16 at 14:59
  • Will award bounty once it let's me :) – Freddy Apr 09 '16 at 14:59
0

Aside from security issues and bad coding practices, here are a couple things you can do.

  1. You don't need to compare the name to check if the bio already exists. You can just count the number of rows returned. If it is more than zero, then the user bio already exists.

  2. When comparing strings, === is preferred over ==. You can read about it and find out why but here is an example (2nd answer)

  3. You should really look into either REPLACE INTO or ON DUPLICATE KEY UPDATE. Just using either of there 2, depending on your use case pick one, you can pretty much eliminate more than half of your currently displayed code. Basically, both will insert and if the record already exists, they updates. Thus, you wouldn't even need to check if the record already exists.

Community
  • 1
  • 1
RisingSun
  • 1,693
  • 27
  • 45