patternphpModerate

Simple comment system

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

commentsystemsimple

Problem

This is a comment system that I wrote, is it secure?


*Please enter a Guest Name*';  
    } 

    if (isset($_POST['submit_comment']) && empty($comment)) {
        echo '*Please enter a Comment*'; 
    } 

//List comments
    function list_comments () {
        $query = "SELECT * FROM `user_comments` ORDER BY `ID` DESC";
        $query_run = mysql_query($query);
        while ($result = mysql_fetch_array($query_run)) {
            echo '' . $result['Guest Name'] . ' - ' . $result['Date'] . ' - ' . $result['Time'] . '' . $result['Comment'] . '';  
        }
    };

?>
Main_Page

    
    
    

Comments

Solution

<?PHP

Normally it's all lower case, just saying.

//Turn off error reporting (Not Necessary)
error_reporting(E_ALL ^ E_NOTICE);

This is one prime example of confusing and not helpful commenting. If this call is not necessary, why is it there at all?

if (!@mysql_connect('localhost', 'user', '') or !@mysql_select_db('comments')) {

Stop using the mysql_* functions, seriously.

$guest_name = htmlentities(str_replace(' ', '',$_POST['guest_name']));

Why are you replacing entities here? You should not replace stuff when saving it into your database. Ideally the data in your database would be as unbiased as possible. Only replace entities if you need to, f.e. you display it on a webpage.

$time = date('g:i A', time());
$date = date('n/j/Y');
$query = "INSERT INTO `user_comments` VALUES(
    '".mysql_real_escape_string('')."',
    '".mysql_real_escape_string($guest_name)."',
    '".mysql_real_escape_string($comment)."',
    '".mysql_real_escape_string($date)."',
    '".mysql_real_escape_string($time)."'
    )";

From this query I can tell you a few things:

Your database layout is sub-optimal, it uses VARCHAR fields if it should use a DATETIME field.

You like to copy and paste code, that's bad.

You don't do things explicitly, consider adding a fields list to your queries.

First, you should be using a DATETIME type if you're gonna save date and time. MySQL supports also a 'pure' DATE and TIME format, there's absolutely no reason and no excuse to use use a VARCHAR field for that.

And then there's your missing fields list, I made a habit out of adding a fields list to my queries for two reasons:

It let's the reader know exactly what is getting set to where.

It makes your code more robust, the query will not fail if you add a field. Removing a field is also easier, as a simple grep for the field will yield all queries that use it.

So, your query should look like this:

$query = "
    INSERT INTO 
        user_comments
    SET
        name = :name,
        comment = :comment;
";

It's easy to read and you know exactly what ends where. Additionally the date/time stuff is here completely missing, because I changed the database layout:

CREATE TABLE user_comments
    name VARCHAR,
    comment VARCHAR,
    timestamp TIMESTAMP DEFAULT CURRENT_TIMESTAMP;

The TIMESTAMP type is optimal for this kind of operation, as it is able to save automatically the time when the row is created.

$array_query = mysql_query("SELECT * FROM `comments` ORDER BY `ID` DESC");

Same here, your usage of the * makes it easily broken by adding new fields. Additionally you don't know what fields are fetched until you look at the table declaration.

header('Location: index.php');

I'm 99% sure that this should not work. Headers can not be changed after data was send, you set the header in the middle of the file, but your php script should be moved to the top of the file. Additionally if you want to redirect people, it would be good to die() afterwards to make sure that your script stops executing at this point:

");
    }
?>

    
    ...

This will also allow everyone who is not automatically redirected to see where to go to now.

echo '*Please enter a Guest Name*';

You're outputting stuff while inside the `, that's bad practice, content should go into the

.



What is this?!

And just to make sure:

Stop using the

mysql_*` functions and start using prepared queries!

Code Snippets

//Turn off error reporting (Not Necessary)
error_reporting(E_ALL ^ E_NOTICE);

if (!@mysql_connect('localhost', 'user', '') or !@mysql_select_db('comments')) {

$guest_name = htmlentities(str_replace(' ', '',$_POST['guest_name']));

$time = date('g:i A', time());
$date = date('n/j/Y');
$query = "INSERT INTO `user_comments` VALUES(
    '".mysql_real_escape_string('')."',
    '".mysql_real_escape_string($guest_name)."',
    '".mysql_real_escape_string($comment)."',
    '".mysql_real_escape_string($date)."',
    '".mysql_real_escape_string($time)."'
    )";

$query = "
    INSERT INTO 
        user_comments
    SET
        name = :name,
        comment = :comment;
";

Context

StackExchange Code Review Q#41209, answer score: 13

Revisions (0)

No revisions yet.