Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 0 down vote favorite

So, I have coded a PHP script that will make it able for the users to edit their Post, or comments on a site that runs SociaEngine 4.

So, Is there anything that could get inproved/fixed on this?

<?php
    session_start();
    $Mysql_Hostname = "192.168.1.110";
    $Mysql_Username = "*user";
    $Mysql_Password = "************";
    $Mysql_Database = "Socialengine";
    $Sql = new mysqli($Mysql_Hostname, $Mysql_Username, $Mysql_Password, $Mysql_Database);
    if ($Sql->connect_error){die($Sql->connect_error);}

    $ID = session_id(); // Get session cookie data / id
    $GetSession = $Sql->query("SELECT * FROM  `engine4_core_session` WHERE id =  '".$ID."'"); // Get the session data for the user.
    $Session = $GetSession->fetch_array(); // blah blah...
        if($Session['user_id'] == NULL) { die("You need to be logged inn to use this"); } // Check if the user is logged inn

    $GetUser = $Sql->query("SELECT * FROM  `engine4_users` WHERE `user_id` = '".$Session['user_id']."'"); // Get user data
    $User = $GetUser->fetch_array(); // blah...

    echo "<p>You are currenty logged inn as: ".$User['displayname']."</p>"; // Give a message that they are logged in

    if($_GET['mode'] == "done") { // If they have edited, Then close the window
        echo '<script type="text/javascript"> window.close(); </script>';
    }

    if($_GET['mode'] == "edit") { // If they want to edit a comment...
        $CID = $_GET['id']; // Get comment id
        $GetComment = $Sql->query("SELECT * FROM  `engine4_activity_comments` WHERE `poster_id` = '".$Session['user_id']."' AND `comment_id` = '".$Sql->real_escape_string($CID)."'"); // Get the comment from Mysql with User restriction...
        $Comment = $GetComment->fetch_array(); // blah
        if($GetComment->num_rows == 0) { die("Not authorized to edit"); } // If the comment wasn't theirs, Then die...
    echo <<<HTML
    <form action="Session.php?mode=done&type=comment" method="post">
    <input type="hidden" name="ID" value="{$Comment['comment_id']}">
    <textarea rows="3" cols="45"  name="Comment">{$Comment['body']}</textarea>
    <p><input type="submit" value="Edit comment"></p>
    </form>

HTML;
// Render the edit form

    exit; // Stop processing
    }

    if($_GET['mode'] == "post") { // If they want to edit a post...
        $CID = $_GET['id']; // Get post id
        $GetPost = $Sql->query("SELECT * FROM  `engine4_activity_actions` WHERE `subject_id` = '".$Session['user_id']."' AND `action_id` = '".$Sql->real_escape_string($CID)."'"); // Same as comments, Get with post id and user id
        $Post = $GetPost->fetch_array(); // blah
            if($GetPost->num_rows == 0) { die("Not authorized to edit"); } // No! Don't try to edit others post's...
    echo <<<HTML
    <form action="Session.php?mode=done&type=post" method="post">
    <input type="hidden" name="ID" value="{$Post['action_id']}">
    <textarea rows="3" cols="45" name="Comment">{$Post['body']}</textarea>
    <p><input type="submit" value="Edit post"></p>
    </form>

HTML;
// Render the post edit form
    exit; // and stop
    }

    if(isset($_POST['Comment'])) { // If the form has been submitted.
        if($_GET['type'] == "comment") { // Check if it's a comment or a post
            $Verify = $Sql->query("SELECT * FROM  `engine4_activity_comments` WHERE `poster_id` = '".$Session['user_id']."' AND `comment_id` = '".$Sql->real_escape_string($_POST['ID'])."'"); // Verify if it's theirs...
            if($Verify->num_rows == 1) { // If it's theirs, Contiune
                $Update = $Sql->query("UPDATE  `Manehatten`.`engine4_activity_comments` SET  `body` =  '".$Sql->real_escape_string($_POST['Comment'])."' 
                WHERE `poster_id` = '".$Session['user_id']."' AND `comment_id` = '".$Sql->real_escape_string($_POST['ID'])."'"); // Update the data in mysql
            }
        }
        elseif($_GET['type'] == "post") { // Check if it's a comment or a post
            $Verify = $Sql->query("SELECT * FROM  `engine4_activity_actions` WHERE `subject_id` = '".$Session['user_id']."' AND `action_id` = '".$Sql->real_escape_string($_POST['ID'])."'"); // Verify if it's theirs...
            if($Verify->num_rows == 1) { // If it's theirs, Contiune
                $Update = $Sql->query("UPDATE  `Manehatten`.`engine4_activity_actions` SET  `body` =  '".$Sql->real_escape_string($_POST['Comment'])."' 
                WHERE `subject_id` = '".$Session['user_id']."' AND `action_id` = '".$Sql->real_escape_string($_POST['ID'])."'"); // Update the data..
            }
        }
    }
    // This script is only for test, or comment id getting /IGNORE THIS/
    echo "<p>Displaying Comments with Limit of 30 </p>";
    echo "<hr>";
    $GetComments = $Sql->query("SELECT * FROM  `engine4_activity_comments` WHERE `poster_id` = '".$Session['user_id']."' ORDER BY  `engine4_activity_comments`.`creation_date` DESC LIMIT 0, 30");
    while($Data = $GetComments->fetch_assoc()){
        echo "<p>".$Data['body'].' | ID: <a href="Session.php?mode=edit&id='.$Data['comment_id'].'">'.$Data['comment_id']."</a> <- Click to edit</p>";
    }

I have added comments for what lines do what.

I am just a bit unsure if they can use this and alter their session cookie to gather a valid user cookie.

The script works fine, Just that if there was something that could be improved.

share|improve this question

1 Answer

up vote 2 down vote accepted

You might consider incorporating a config file for your SQL connection. This will allow you to reuse it should that become necessary and update it more easily. Additionally, the $Mysql_* namespace seems unnecessary.

Typically variables are not capitalized. This may just be a style choice, but it looks odd and may cause issues should anyone decide to try and integrate your script into their own.

die() is a very inelegant way of terminating your script. You should throw an error or render an error page, with the former eventually accomplishing the latter anyways. You especially shouldn't just dump the error onto the page for any malicious user to see. Save it to a log. The same goes for exit, except you should return early instead. Returning early is really only possible with functions, but exiting the script completely stops everything. That means that if this script were included in another and that other script still needed to do something it couldn't, making your script very hard to incorporate.

Comments explaining your code should be unnecessary. If your code is self-documenting then it should be obvious. You should especially avoid comments that don't add anything to your code, such as "blah...", its just clutter and makes your code less legible.

You are using MySQLi, which is a prepared SQL language, but you aren't using prepared statements. I'm not much of a SQL guru here, but I thought that was the whole reason for the switch from MySQL to MySQLi/PDO.

You might also consider adding whitespace to your SQL statements and long PHP statements. Neither PHP nor SQL minds extra whitespace, so making your code more legible is always a plus. The following might be a little excessive, I rewrote it using my style, but it just demonstrates how little whitespace makes a difference in operation, and how much more it adds to legibility.

$GetSession = $Sql->query( "
    SELECT *
    FROM  `engine4_core_session`
    WHERE id = '" . $ID . "'
" );

//and

if( $Session[ 'user_id' ] == NULL ) {
    die( "You need to be logged inn to use this" );
}

Also, there is no need to escape the string sequence to incorporate the PHP variable. That's the whole reason for using double quotes. PHP automatically escapes any entities or variables it finds in double quoted strings.

"WHERE id = '$ID'"

There is no need to explicitly check for a NULL value when doing a loose == comparison. Unless you are specifically looking for NULL, in which case you should be using an absolute === comparison.

if( ! $Session[ 'user_id' ] ) {

Instead of echoing HTML, you should consider escaping from PHP. This will allow your IDE to do proper tag matching/highlighting, and will make your code a little easier to read.

?>

<p>You are currently logged in as: <?php echo $User[ 'displayname' ]; ?></p>

<?php

When outputting a lot of HTML, you can consider creating an include. This cleans up some of the clutter from your PHP and separates the display from the logic.

//include page
<form action="Session.php?mode=done&type=comment" method="post">
<input type="hidden" name="ID" value="<?php echo $Comment[ 'comment_id' ]; ?>">
<textarea rows="3" cols="45"  name="Comment">
    <?php echo $Comment[ 'body' ]; ?>
</textarea>
<p><input type="submit" value="Edit comment"></p>
</form>

When you find yourself comparing the same variable multiple times against different values, you might consider using a switch. Switches are slightly faster than a normal if statement and are a little cleaner.

switch( $_GET[ 'mode' ] ) {
    case 'done' :
        //etc...
    break;

    case 'edit' :
        //etc...
    break;

    //etc...
}

The rest appears to be more of the same. The biggest improvement I can suggest would be to incorporate the use of functions to make this more legible and reusable. Also you appear to be violating the "Don't Repeat Yourself" (DRY) Principle. As the name implies, your code should not repeat itself. Some of those latter SQL statements appeared very similar, meaning you could probably create a template string and modify it as necessary. Hope this helps.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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