-1

I wanted to create a login screen that a user can choose which database to save to depending on their profession via radio buttons. I've got most errors ironed out, but these ones:

Notice: Undefined variable: sqlinsert in C:\xampp\htdocs\project\save.php on line 34

Warning: mysqli_query(): Empty query in C:\xampp\htdocs\project\save.php on line 34

have refused to go away. Here's my Html:

<form method="post" action="http://localhost/project/save.php">
    <legend align="left">Login</legend>
    <input type="text" placeholder="Username" name="uname"><br>
    <input type="password" placeholder="Password" name="password"><br>
    <p>
        <label>
        <input type="radio" name="dbchoice" value="admin" id="RadioGroup1_0">
        Admin</label>
        <br>
        <label>
        <input type="radio" name="dbchoice" value="expert" id="RadioGroup1_1">Mental health experts</label>
        <br>
    </p>
    <input type="submit" value="Save" name="save">
    <input type="reset" value="Cancel">
</form>

And here's my PHP:

<?php

$hostname = "localhost";
$username = "root";
$password = "";
$db = "project";

//connect to the database using procedural method

$dbconnect = mysqli_connect($hostname,$username,$password,$db);

if(!$dbconnect){
    echo "an error occured while connecting to the database";
}

if(isset($_POST['save'])){
    //get values from the form
    $username = $_POST['uname'];
    $password = $_POST['password'];


    //write the SQL STATEMENT FOR SAVING - INSERT
    if (isset($dbchoice) && $dbchoice==$_POST["admin"]) {
        $sqlinsert = "INSERT INTO administrator 
                                    (username, password) 
                            VALUES('$username','$password')";
    }elseif (isset($dbchoice) && $dbchoice==$_POST["expert"]){
        $sqlinsert = "INSERT INTO experts
                                    (username, password) 
                            VALUES('$username','$password')";   
    }else{
        echo "An error occured while writing to database.";
    }
    if(mysqli_query($dbconnect,$sqlinsert)){
        echo "<script type='text/javascript'> alert('Data saved successfully');</script>";
        header('location: index.php');
    }else{
        echo "An error occured while saving";
    }

    //close the connection
    mysqli_close($dbconnect);

}else{
    header('location: index.php');
}
?>
Dharman
  • 30,962
  • 25
  • 85
  • 135
Biscuit
  • 21
  • 3
  • I'd suggest adding a default `selected` attribute to one or other radio button to ensure it is always present in the POST array... or a hidden field with a default value – Professor Abronsius Jun 20 '20 at 11:12
  • You have a variable `$dbchoice` that is not defined in the code above before it is called – Professor Abronsius Jun 20 '20 at 11:13
  • Your sql is vulnerable to SQL Injection – Professor Abronsius Jun 20 '20 at 11:15
  • Good code indentation would help us read the code and more importantly it will help **you debug your code** [Take a quick look at a coding standard](https://www.php-fig.org/psr/psr-12/) for your own benefit. You may be asked to amend this code in a few weeks/months and you will thank me in the end. – RiggsFolly Jun 20 '20 at 12:36
  • 1
    **Never store passwords in clear text or using MD5/SHA1!** Only store password hashes created using PHP's [`password_hash()`](https://php.net/manual/en/function.password-hash.php), which you can then verify using [`password_verify()`](https://php.net/manual/en/function.password-verify.php). Take a look at this post: [How to use password_hash](https://stackoverflow.com/q/30279321/1839439) and learn more about [bcrypt & password hashing in PHP](https://stackoverflow.com/a/6337021/1839439) – Dharman Jun 20 '20 at 12:38
  • **WARNING**: Writing an access control layer is not easy and there are many opportunities to get it severely wrong. Any modern [development framework](https://www.cloudways.com/blog/best-php-frameworks/) like [Laravel](http://laravel.com/) comes with an [authentication system](https://laravel.com/docs/master/authentication) built-in, and there are [authentication libraries](http://phprbac.net/) you can use. At the absolute least follow [recommended security best practices](http://www.phptherightway.com/#security) and **never store passwords as plain-text** or a weak hash like **SHA1 or MD5**. – tadman Jun 20 '20 at 23:25
  • 1
    The real question here is why you have two different tables rather than one table with a `role` type column. – tadman Jun 20 '20 at 23:26
  • Note: The [object-oriented interface to `mysqli`](https://www.php.net/manual/en/mysqli.quickstart.connections.php) is significantly less verbose, making code easier to read and audit, and is not easily confused with the obsolete `mysql_query` interface where missing a single `i` can cause trouble. Example: `$db = new mysqli(…)` and `$db->prepare("…")` The procedural interface is an artifact from the PHP 4 era and should not be used in new code. – tadman Jun 20 '20 at 23:26
  • 1
    @tadman by the way, there is another reason why procedural interface is inferior. when procedural prepare is called, [it doesn't produce an error](https://stackoverflow.com/q/57982685/285587), even if exceptions are enabled. not to mention that procedural calls are more permissive, just mumbling warnings but chugging on, instead of dying early as it would be with object syntax. – Your Common Sense Jun 21 '20 at 06:57
  • @YourCommonSense Interesting to note there. I'll try and amend that remark. I'm just persistently disappointed that tutorials keep using these outdated methods. Why can't they just use PDO, which doesn't lock people into MySQL, and move on? Every other language has something like JDBC or DBO or something generic to invest your time learning in. – tadman Jun 21 '20 at 07:03

1 Answers1

3

You are incorrectly trying to compare a variable $dbchoice with a POST field that does not exist $dbchoice==$_POST["admin"] - the field is dbchoice but the value is admin

You need to test that these fields are present in the POST array and assign as variables if and only if they are- perhaps this might help. Also - if there is a default table that should be used then set that radio button as selected maybe?!

Never take unsanitised user input as safe - do not use it in your SQL statements as you did here, always use a prepared statement with correctly bound parameters.

<?php

error_reporting(E_ALL);

if (isset($_POST['save'], $_POST['uname'], $_POST['password'], $_POST['dbchoice'])) {
    mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);

    $hostname = "localhost";
    $username = "root";
    $password = "";
    $db = "project";
    $dbconnect = mysqli_connect($hostname, $username, $password, $db);

    $username = $_POST['uname'];
    $password = $_POST['password'];
    $dbchoice = $_POST['dbchoice'];

    switch ($dbchoice) {
        case 'admin':
            $sql = 'insert into `administrator` (`username`, `password`) values (?,?)';
        break;
        case 'expert':
            $sql = 'insert into `experts` (`username`, `password`) values( ?,? )';
        break;
        default:
            $sql = false;
        break;
    }
    
    if ($sql) {
        $stmt = $dbconnect->prepare($sql);
        $stmt->bind_param('ss', $username, $password);
        $stmt->execute();
        
        header('Location: index.php');
        exit;
    }
}
Dharman
  • 30,962
  • 25
  • 85
  • 135
Professor Abronsius
  • 33,063
  • 5
  • 32
  • 46