5
\$\begingroup\$

I'm new to PHP and web development in general. Is this a good coding style for me as a beginner? I don't care about password hashing. I use MD5 which is, as far as I know, not a good encryption method, and I'm trying to create just the login for now.

connection.php:

<?php   

try {
    $pdo = new PDO("mysql:host=localhost;dbname=akar","akar","raparen");
} catch (PDOException $e) {
    echo $e->getMessage();
}
?>

index.php:

<?php 
session_start();
include "connection.php";

if(isset($_SESSION['logged_in']) == true){
    //Redirect the user to members
    header("Location: member.php");
    exit();

} else {
    ?>

    <?php 
}
    if(isset($_POST['username']) && isset($_POST['password'])) {
        $username = $_POST['username'];
        $password = md5($_POST['password']);

        if(empty($username) or empty($password)) {
            //Display Errors
            $error = "All fields are required!";

        } else {
            //Verify the user
            $sql = "SELECT * FROM `users` WHERE user_name = ? AND user_password = ?";
            $query = $pdo->prepare($sql);
            $query->bindValue(1,$username);
            $query->bindValue(2,$password);
            $query->execute();

            if ($query->rowCount() > 0) {
                //User details right
                $_SESSION['logged_in'] = true;
                header("Location: member.php");
            } else {
                //User details wrong
                $error = "Wrong username or password";
            }
        }
    }

?>

<?php if (isset($error))  { ?>
 <small style="color:#aa0000; font-weight:bold;">
    <?php echo $error; ?>
 </small>
<?php } ?>

member.php:

<?php 
session_start();

if($_SESSION['logged_in']) {
    echo "You're logged in";
} else {
    header("Location: index.php");
}

?>

<a href="logout.php">Logout</a>

logout.php

<?php   
    session_start();

    session_destroy();
    header("Location: index.php");
?>

Here is my form:

 <form action="index.php" method="POST" autocomplete="off">
    <input type="text" name="username" placeholder="Username" />
    <input type="password" name="password" placeholder="Password" />
    <input type="submit" name="submit" value="Login" />
 </form>
\$\endgroup\$
5
\$\begingroup\$

Here are few things I observed:

  1. You repeated session_start() twice in your member.php - you can only do that once in order to start the session

  2. You are not sanitizing your input for bad values - its best practice to do so to avoid SQL injection - however as a beginner its a good idea that you started with PDO and not legacy mysql_query

  3. After a redirect you should exit(); because your script will continue to process until end of the file. Why let a script continue when you're done.

  4. I know you are trying to section your code but if PHP code continues don't close it un-necessarily. (referring to index.php and to the if error part).

  5. You should use require or require_once. include causes an error if include is invoked (by mistake) more than one time.

  6. Last observation would be in your member.php it should be a blacklist if and not if/else. You should basically tell the user that you shouldn't be here if you are not logged in..if not just proceed to the rest of the script (ask question if you didn't understand this part)

\$\endgroup\$
  • \$\begingroup\$ Oh, Sorry. for #1 the repeated session_start was for login.php. edited the post. \$\endgroup\$ – Akar Mar 19 '14 at 16:25
  • \$\begingroup\$ @AkarMuhamad I will edit my post with one more notice. \$\endgroup\$ – azngunit81 Mar 19 '14 at 16:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.