7
\$\begingroup\$

This is my first time messing with PHP and I tried to put together a simple address book that when the user fills out the form it would update a database table, then display the row data on the page after submission.

It works fine, but I just want to make sure I'm headed in the right direction.

index.php:

<?php

if ($_SERVER["REQUEST_METHOD"] == "POST") {

// connect to database
include("inc/dbconnect.php");

// assigns form data to table columns
$assign = "INSERT INTO contacts(firstName,lastName,email,phone,birthday) VALUES ('$_POST[firstName]','$_POST[lastName]','$_POST[email]','$_POST[phone]','$_POST[birthday]')";

// execute query
if (mysqli_query($database,$assign)) {
    header("Location:http://localhost/address-book/");
    exit;
} else {
    echo "Everything blew up!" . mysqli_error($database);
}

// closes database connection
mysqli_close($database);

}

?>

<!DOCTYPE html>
<html>
<head>
<title>Address Book</title>
</head>
<body>

<h1>Address Book</h1>

<table cellpadding="5" cellspacing="0">
    <thead>
        <tr>
            <th>Contact name</th>
            <th>Email address</th>
            <th>Phone</th>
            <th>Birth Date</th>
        <tr>
    </thead>
    <tbody>
        <?php include("inc/records.php"); ?>
    </tbody>
</table>


<h3>Add new contact</h3>

<form method="post" action="index.php">
    <input type="text" name="firstName" id="firstName" placeholder="First name">
    <input type="text" name="lastName" id="lastName" placeholder="Last Name"><br />
    <input type="text" name="email" id="email" placeholder="Email address">
    <input type="text" name="phone" id="phone" placeholder="Phone"><br />
    <input type="text" name="birthday" id="birthday" placeholder="Birthday">
    <input type="submit" value="Save record" placeholder="Save">
</form>


</body>
</html>

dbconnect.php:

// database connect
$database = mysqli_connect(
            "localhost",
            "user",
            "password",
            "database"
);

records.php:

// connects to database
include("dbconnect.php");

// targets the database table contacts
$records = mysqli_query($database,"SELECT * FROM contacts");

// pull row data from database
while($record = mysqli_fetch_array($records)) {
    echo "<tr>";
    echo "<td>" . $record['firstName'] . " " . $record['lastName'] . "</td>";
    echo "<td><a href='mailto:" . $record['email'] . "'>" . $record['email'] . "</a></td>";
    echo "<td>" . $record['phone'] . "</td>";
    echo "<td>" . $record['email'] . "</td>";
    echo "</tr>";
}

mysqli_close($database);
Improve this question
\$\endgroup\$
11
\$\begingroup\$

I think there are two major issues you should address right in the beginning when you start with PHP.

Separation of Layout and Logic

This means in general that you shouldn't mix HTML and PHP code. Later this will lead you to the MVC-Pattern.

index.php

<?php
// connect to database
include("inc/dbconnect.php");

if ($_SERVER["REQUEST_METHOD"] == "POST") {
    //...
}

$result = mysqli_query($database,"SELECT * FROM contacts");

$records=array();
while($row = mysqli_fetch_array($result)) {
    $record[]=$row;
}

mysqli_close($database);

include 'template.php';

template.php

<!DOCTYPE html>
<html>
...
    <tbody>
        <?php foreach ($records as $record):?>
        <tr>
             <td><?= $record['firstName']?> <?= $record['lastName']?></td>
             <td><a href="mailto:<?= $record['email']?>"><?= $record['email']?></a></td>
             <td><?= $record['phone']?></td>
             <td><?= $record['email']?></td>
        </tr>
        <?php endforeach;?>
    </tbody>
...
</html>

Prevent SQL Injection

You are passing user input directly to the query you send to the database. Wikipedia will elaborate some examples in detail and shows how queries can be manipulated.

The easies way for you to prevent this, is using PHP Data Objects and Prepared Statements. I think there is no need for going in detail here if you don't have a specific question, there are many good tutorials out there. The important part is, that the user is no longer able to change the query you send to the database.

Minor issues

  • If you only have one database connection PHP will use this connection automatically with the mysqli_ methods so you don't have to pass the $database around.
  • It is best practice to leave away the final '?>' in plain php files. This will prevent that you send any whitespace after ?> to the browser accidentally. (Important if you change the header later.)
  • echo "Everything blew up!" . mysqli_error($database); . May you should use die.
Improve this answer
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for reviewing everything! I'll definitely make these changes. I added a delete feature last night and plan on trying to add sorting, search and possibility an editing feature. \$\endgroup\$ – omyzach Jul 26 '13 at 15:10
2
\$\begingroup\$

Before I take a stab at giving some (hopefully) decent feedback here's something to think about..

if ($_SERVER["REQUEST_METHOD"] == "POST") {

The above I cannot fault but wouldn't the following be better?

 if ( isset($_POST['save_btn']) ){

I have this nagging need to improve clarity. And I get it, in this instance it's quite subjective but I prefer the latter since you are explicitly checking if the save button was clicked and it gives you a warm feeling of knowing where the POST request came from. If you're thinking 'I Disagree', I won't hold a grudge against you.

Vulnerability

Defending yourself

As a rule of thumb, never ever trust the data coming from the user. So what you're doing here is without exaggeration disastrous:

// assigns form data to table columns
$assign = "INSERT INTO contacts(firstName,lastName,email,phone,birthday) VALUES ('$_POST[firstName]','$_POST[lastName]','$_POST[email]','$_POST[phone]','$_POST[birthday]')";

This is better demonstrated with a simple example: 

//normal input
//POST value: dave
query = "SELECT username, password FROM users WHERE username='dave'";


//malicious input
//POST value: 'OR'1
query = "SELECT username, password FROM users WHERE username=''OR'1' ";

The nasty thing here is, 1 evaluates to true thus returning all usernames and passwords in the users table!

mysqli_real_escape_string() to the rescue

Despite being a mouthful to say, this function provides a safeguard by escaping injection attempts with MySQL-friendly '\' quote.

So pumping all your post data through this function provides a layer of security.

$username = mysqli_real_escape_string($_POST['username'];

Now hopefully that makes sense. Despite rhapsodising mysqli_real_escape_string() I would highly recommend (at some point) looking into using something a bit more sophisticated like PDO instead.

Alrighty, moving on..

Presentation vs. Logic

mnhg raises a great point about separation of presentation/layout from logic. Try to get your head around this concept as quickly as you can as it will not only improve the life of your code but it will save you from headaches like header already sent

The idea is quite simple once you catch on to it. Without re-inventing the wheel, taking mnhg's code as an example you'll noticed that the template.php file, also known as the 'view' is the presentation interface. It's only outputting data. Looping through data and having a conditional statement to check if the data exists is quite acceptable.

On the other hand index.php is known as the 'Controller'. An analogy would be like a flight control tower where all the 'thinking' or logic happens. And just like a control tower giving instructions to a flight, a Controller, in a similar fashion calls functions. So yes, think about the reusability of your code and function it up! (I emphasize the use of functions because you don't want to clutter your Controllers with redundant tasks when it can be extracted into a function and sometimes its just better represented via a function).

The above and more forms almost a de facto design pattern for web development called MVC

Improve this answer
\$\endgroup\$
1
  • \$\begingroup\$ Thanks. It was an oversight on my part. I did mean to use mysqli_real_escape_string(). Updated. \$\endgroup\$ – kaizenCoder Aug 7 '13 at 3:11

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.