Post script?

[doubledee]

Can I post my "Add a Comment" script here to get help improving it?

 

 

Debbie

[ecabrera]

Sure that's what this forum is use for

[doubledee]

Sure that's what this forum is use for

 

Okay, but I wasn't sure if I was allowed to post an *entire* script...

 

 

Debbie

 

[ecabrera]

you should post the part you need help improving

[doubledee]

you should post the part you need help improving

 

I want a critique of the entire script...

 

 

Debbie

 

[scootstah]

you should post the part you need help improving

 

I want a critique of the entire script...

 

 

Debbie

 

 

So post it.

[doubledee]

So post it.

 

Thought you'd never ask?! 

 

Here is a script that allows Users to "Add a Comment" to an article that they just read on my website.

 

The code is designed to split up the messaging from the initial form.  The form does, however, submit to itself, and then PHP does the form handling.

 

add_comment.php

<?php
// Initialize a session.
session_start();

// Access Constants.
require_once('config/config.inc.php');

// Initialize Variables.
$_SESSION['resultsCode'] = '';


// **************************
// Check for Primary Keys.	*
// **************************

// If either articleID or memberID do not exist for whatever reason, then we
// cannot insert a record, and therefore the form should never be displayed.
if (empty($_SESSION['articleID'])	|| empty($_SESSION['memberID'])){
	// Key(s) Missing.
	$_SESSION['resultsCode'] = '61';

	// Redirect to Processing Page.
	header("Location: " . WEB_ROOT . "process_comment.php");

	// End script.
	exit();
}


// ************************
// Check for Form Values.	*
// ************************

// If either pageTitle or memberFirstName do not exist for whatever reason,
// then either there were problems setting the $_SESSION, or the user may have
// arrived at this page directly which indicates a larger problem.  In either
// case, the form should not be displayed.
if (empty($_SESSION['pageTitle'])	|| empty($_SESSION['memberFirstName'])){
	// Form Values Missing.
	$_SESSION['resultsCode'] = '62';

	// Redirect to Processing Page.
	header("Location: " . WEB_ROOT . "process_comment.php");

	// End script.
	exit();
}else{
	// Set Local Variables.
	$pageTitle = $_SESSION['pageTitle'];
	$memberFirstName = $_SESSION['memberFirstName'];
}


// *************************************************************
// HANDLE FORM.																								 *
// *************************************************************
if ($_SERVER['REQUEST_METHOD']=='POST'){
	// Form was Submitted (Post).

	// Initialize Errors Array.
	$errors = array();

	// Trim all form data.
	$trimmed = array_map('trim', $_POST);


	// ************************
	// Validate Form Data.		*
	// ************************
	// Check Comment.
	if (empty($trimmed['body'])){
		$errors['body'] = 'Please enter a Comment.';
	}

	// **************************
	// Attempt to Add Comment.	*
	// **************************
	if (empty($errors)){
		// Form data clean.

		// Set Comment Variables.
		$articleID = $_SESSION['articleID'];
		$memberID = $_SESSION['memberID'];
		$body = $trimmed['body'];
		$status = 'Pending';


		// Connect to the database.
		require_once('private/mysqli_connect.php');

		// Build query.
		$q = "INSERT INTO comment(article_id, member_id, body, status, created_on)
						VALUES(?, ?, ?, ?, NOW())";

		// Prepare statement.
		$stmt = mysqli_prepare($dbc, $q);

		// Bind variable.
		mysqli_stmt_bind_param($stmt, 'iiss', $articleID, $memberID, $body, $status);

		// Execute query.
		mysqli_stmt_execute($stmt);

		// Verify Insert.
		if (mysqli_stmt_affected_rows($stmt)==1){
			// Insert Succeeded.
			$_SESSION['resultsCode'] = '75';
		}else{
			// Insert Failed.
			$_SESSION['resultsCode'] = '63';
		}// End of VERIFY INSERT.

		// Close prepared statement.
		mysqli_stmt_close($stmt);

		// Close the connection.
		mysqli_close($dbc);


		// Redirect to Display Outcome.
		header("Location: " . WEB_ROOT . "process_comment.php");

		// End script.
		exit();

	}else{
		// Form data has errors.			(** NEW **)
		// Drop-through to Form to display errors.

	}// End of CHECK FOR ERRORS.

}else{
	// Form was NOT Submitted (Get).
	// Drop-through to display Form.

}// End of HANDLE FORM.

?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
"http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">

<head>
<!-- HTML Metadata -->
<title>Comment on an Article</title>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<meta name="description"
			content="" />
<meta name="keywords"
			content="" />

<!-- Page Stylesheets -->
<link type="text/css" rel="stylesheet" href="css/main.css" />
<link type="text/css" rel="stylesheet" href="css/dropdown.css" />
<link type="text/css" rel="stylesheet" href="css/components.css" />
<link type="text/css" rel="stylesheet" href="css/add_comment.css" />
</head>

<body>
<div id="wrapper" class="clearfix">
	<div id="inner">
		<!-- Include BODY HEADER -->
		<?php require_once(ROOT . 'components/body_header.inc.php');	?>

		<!-- MIDDLE COLUMN -->
		<div id="middle_2col">
			<div class="box_Content">
				<div id="box_Comments">
					<h3 class="header2_h3">What Do You Think?</h3>

					<form id="addComment" action="" method="post">
						<!-- ADD A COMMENT FORM -->
						<fieldset>
							<ol>
								<!-- Set Form Token -->
<!--
								<li>
									<input type="hidden" name="form_token" value="<?php echo (isset($_SESSION['form_token']) ? $_SESSION['form_token'] : ''); ?>" />
								</li>
-->
								<!-- Article Title -->
								<li>
									<p class="fauxLabel">Article Title:</p>
									<p class="fauxInput"><?php echo '"' . htmlspecialchars($pageTitle, ENT_QUOTES) . '"'; ?></p>
								</li>

								<!-- First Name -->
								<li>
									<p class="fauxLabel">First Name:</p>
									<p class="fauxInput"><?php echo htmlspecialchars($memberFirstName, ENT_QUOTES); ?></p>
								</li>

								<!-- Comment -->
								<li>
									<label for="body">Comment:</label>
									<textarea id="body" name="body" cols="50" rows="15"><?php if (isset($body)){echo htmlspecialchars($body, ENT_QUOTES);} ?></textarea>
<!--
									<textarea id="body" name="body" cols="50" rows="15"><?php //echo $body; ?></textarea>
									<textarea id="body" name="body" cols="50" rows="15"><?php //echo htmlspecialchars($body, ENT_QUOTES); ?></textarea>
-->
									<?php
										if (!empty($errors['body'])){
											echo '<span class="error">' . $errors['body'] . '</span>';
										}
									?>
								</li>

								<!-- Submit Form -->
								<li>
									<input type="submit" name="addComment" class="button" value="Add a Comment"/>
								</li>
							</ol>
						</fieldset>
					</form>
				</div><!-- End of BOX COMMENTS -->
			</div><!-- End of #BOX_CONTENT -->
		</div><!-- End of #MIDDLE -->

		<!-- RIGHT COLUMN -->
		<div id="right">
			<!-- Insert STICKY -->
			<?php	require_once('components/content_sticky.inc.php');	?>

		</div><!-- End of #RIGHT -->

	</div><!-- End of #INNER -->
</div><!-- End of #WRAPPER -->

<!-- Include BODY FOOTER -->
<?php	require_once(ROOT . 'components/body_footer.inc.php');	?>

</body>
</html><? //Build Date: 2011-09-06 6:46pm ?>

 

 

Questions:

1.) How does my code look?

 

2.) What are your thoughts on how I assign an Error Code and then use my Session to pass that to the "Outcome Page"?

 

3.) Does my code look secure?

 

4.) How can I further improve things?

 

5.) How can I further separate the Logic from the Presentation?

 

(BTW, I don't know OOP or MVC, so I'd say hold off on advice recommending those right now.)

 

Thanks,

 

 

Debbie

 

[scootstah]

1. It's pretty well commented.

 

2. I do not like this at all. The reason is because the "error codes" seem arbitrary and are meaningless. Without searching around, nobody knows what the error "62" means. Instead, you should use meaningful error codes. So instead of "62", "form_values_missing". You could then further improve this by not serving a static message, but by loading a language file where "form_values_missing" is given a value - this makes your application extensible by allowing different languages, and also organizes all of your text in one convenient location.

 

3. You have a form_token field but don't do anything with it, which means you are open to CSRF attacks. Also, unless you are sanitizing data on output, you are open to XSS as well.

 

 

4. You can improve things by being a little more DRY (Don't Repeat Yourself). For example, instead of

if ... blah
     redirect

if ... blah
     redirect

 

And so on, you could do something like:

$response = '';

if ... blah
     $response = 'some response';

if ... blah
     $response = 'some other response';

...

if (!empty($response))
     redirect

 

5. You can do this by not having a huge wall of HTML at the bottom of your script. This script is business logic, presentation should be in another place. Also, having the entire block of HTML like that is not a good idea. You should only have the HTML that is relevant to that page, and the rest in header/footer files (or a layout file). This is again going on the DRY principle. What if you ever wanted to change a style sheet? You'd have to change it in every single script that displays something.

 

[doubledee]

2. I do not like this at all. The reason is because the "error codes" seem arbitrary and are meaningless. Without searching around, nobody knows what the error "62" means.

 

Who?  The Developer?

 

 

Instead, you should use meaningful error codes. So instead of "62", "form_values_missing".

 

But isn't that harder to maintain and match up against?

 

I display a meaningful message to the user, so why make the actual unique identifier for the message wordy?

 

Also, what happens if I put this into a database?

 

Much easier to have the primary key be "62" than "form_values_missing"...

 

 

3. You have a form_token field but don't do anything with it, which means you are open to CSRF attacks. Also, unless you are sanitizing data on output, you are open to XSS as well.

 

I don't remember what the token was for?!  Can you help me figure this out?

 

Also, I thought this code did santize things before they were output...

	<!-- Article Title -->
	<li>
		<p class="fauxLabel">Article Title:</p>
		<p class="fauxInput"><?php echo '"' . htmlspecialchars($pageTitle, ENT_QUOTES) . '"'; ?></p>
	</li>

 

Thanks,

 

 

Debbie

 

[kicken]

But isn't that harder to maintain and match up against?

 

I display a meaningful message to the user, so why make the actual unique identifier for the message wordy?

 

Also, what happens if I put this into a database?

 

Much easier to have the primary key be "62" than "form_values_missing"...

 

Because it makes it easier for you, or someone in the future who is maintaining your code, to know what the error is without having to track down what '62' means.  It is much more obvious when you use a short phrase such as 'form_values_missing'.

 

Using a short phrase does not necessarily prevent use from using a numbered system as well.  You could have a file called say errorcodes.inc.php that you include in every script and the only thing in it is several defines:

 

<?php

define('ERR_ID_MISSING', 61);
define('ERR_FORM_VALUES_MISSING', 62);
define('ERR_QUERY_FAILED', 75);
...

 

 

[doubledee]

But isn't that harder to maintain and match up against?

 

I display a meaningful message to the user, so why make the actual unique identifier for the message wordy?

 

Also, what happens if I put this into a database?

 

Much easier to have the primary key be "62" than "form_values_missing"...

 

Because it makes it easier for you, or someone in the future who is maintaining your code, to know what the error is without having to track down what '62' means.  It is much more obvious when you use a short phrase such as 'form_values_missing'.

 

So, what happens if I need to change the short phrases down the road?

 

Any suggestions for how to approach/tackle coming up with an error-code scheme for my entire application?  (It seems like now would be a good time to devise such a thing?!)

 

I am thinking maybe I could put all of my error-codes into a MySQL table? 

 

 

Using a short phrase does not necessarily prevent use from using a numbered system as well.  You could have a file called say errorcodes.inc.php that you include in every script and the only thing in it is several defines:

 

<?php

define('ERR_ID_MISSING', 61);
define('ERR_FORM_VALUES_MISSING', 62);
define('ERR_QUERY_FAILED', 75);
...

 

You lost me there.

 

So if I used the DEFINES like you have above...

 

1.) How would I use that in my code?

 

2.) If I needed to look up the code - presumably numeric - to display an Error Message from my database in another script, how would these constants work?

 

Thanks,

 

 

Debbie

 

P.S.  What do you think about how my script above is laid out?

 

 

[kicken]

So, what happens if I need to change the short phrases down the road?

The same thing that would happen if you decided to change the number, you would have to update all the references to it.  Generally, once you create it, you don't change it.  You can always change the actual error message text without changing the error number/phrase.

 

 

So if I used the DEFINES like you have above...

 

1.) How would I use that in my code?

 

2.) If I needed to look up the code - presumably numeric - to display an Error Message from my database in another script, how would these constants work?

 

Defined constants are used like variables, but without the preceeding $.  Just type the name of the constant where you want to use it.

if (empty($_SESSION['articleID'])	|| empty($_SESSION['memberID'])){
	// Key(s) Missing.
	$_SESSION['resultsCode'] = ERR_ID_MISSING;

	// Redirect to Processing Page.
	header("Location: " . WEB_ROOT . "process_comment.php");

	// End script.
	exit();
}

 

As for looking up the error code, that depends on how you store the messages.  If you store them in a db, you just do a db query like when you lookup anything else.  You could have a function, say getErrorMessage($code) that gets the message of the given code via whatever means are necessary for how you store them.

 

ex:

function getErrorMessage($code){
    switch ($code){
        case ERR_ID_MISSING:
             return 'A necessary ID value was not passed';
        case ERR_FORM_VALUES_MISSING:
             return 'Some form fields were left blank';
        case ERR_QUERY_FAILED:
             return 'A database error occured';
    }
}

echo getErrorMessage(ERR_ID_MISSING);

[doubledee]

So, what happens if I need to change the short phrases down the road?

The same thing that would happen if you decided to change the number, you would have to update all the references to it.  Generally, once you create it, you don't change it.  You can always change the actual error message text without changing the error number/phrase.

 

Except this is like the debate of why you shouldn't use natural keys as a primary key in a database...  If you use "Sarah Smith" as the primary key in a Users table, and Sarah marries "Walter Wilson" then you have "Sarah Smith" as a PK for "Sarah Wilson"...

 

 

 

So if I used the DEFINES like you have above...

 

1.) How would I use that in my code?

 

2.) If I needed to look up the code - presumably numeric - to display an Error Message from my database in another script, how would these constants work?

 

Defined constants are used like variables, but without the preceeding $.  Just type the name of the constant where you want to use it.

if (empty($_SESSION['articleID'])	|| empty($_SESSION['memberID'])){
	// Key(s) Missing.
	$_SESSION['resultsCode'] = ERR_ID_MISSING;

	// Redirect to Processing Page.
	header("Location: " . WEB_ROOT . "process_comment.php");

	// End script.
	exit();
}

 

As for looking up the error code, that depends on how you store the messages.  If you store them in a db, you just do a db query like when you lookup anything else.  You could have a function, say getErrorMessage($code) that gets the message of the given code via whatever means are necessary for how you store them.

 

ex:

function getErrorMessage($code){
    switch ($code){
        case ERR_ID_MISSING:
             return 'A necessary ID value was not passed';
        case ERR_FORM_VALUES_MISSING:
             return 'Some form fields were left blank';
        case ERR_QUERY_FAILED:
             return 'A database error occured';
    }
}

echo getErrorMessage(ERR_ID_MISSING);

 

Okay, but I thought the idea of defining CONSTANTS was that 'ERR_ID_MISSING' = 61 ??

 

Do constants have a "global" scope?

 

If I was in "create_account.php" and I have...

		$_SESSION['resultsCode'] = ERR_ID_MISSING;

 

...and then I redirect the user to "handle_outcome.php", could I query my database like this...

SELECT error_message
FROM error
WHERE error_id = 61;

 

And if I can do that, isn't it overkill to have an "ErrorID", "ErrorCode", and "Error Message"?

 

 

 

Debbie

 

[kicken]

Except this is like the debate of why you shouldn't use natural keys as a primary key in a database...  If you use "Sarah Smith" as the primary key in a Users table, and Sarah marries "Walter Wilson" then you have "Sarah Smith" as a PK for "Sarah Wilson"...

 

Hence the don't change it part of my statement.  Generally if your going to use codes at all, your codes will either be

1) Very generalized so they apply to several situations -or-

2) Quite specific so they only apply to one particular case.

 

If you have a new error condition, then you create a new code.  If your updating code that uses a code already, decide whether the new requirements still fit with that code. If they do, keep using it, un-change.  If they don't, create a new code.

 

Changes of the error message text associated with that code generally would be limited to re-phrasing or adding/removing additional details.  Not changing the actual meaning of the code.

 

Okay, but I thought the idea of defining CONSTANTS was that 'ERR_ID_MISSING' = 61 ??

 

Yes, that is the idea

 

 

Do constants have a "global" scope?

 

They have global scope within the script they are defined in.  You can use them in any context.  If you define them in every script (via an included file for instance) then they have "global" scope throughout your application.

 

And if I can do that, isn't it overkill to have an "ErrorID", "ErrorCode", and "Error Message"?

 

You wouldn't need an 'ErrorCode' column in your DB if that is what your saying.  Only the ErrorID (61) and ErrorMessage ('A necessary ID value was not passed').

 

The codes are only for making an easier to remember reference to the ErrorID that you can use in your scripts.  You could, if you desired, store the code in the DB so you can easily see by looking at the DB what code name to use in your PHP file.  If you did, you could then also have your codes automatically defined so you wouldn't have to update both the DB and the codes include when adding a code.

 

Sometimes you have to do a little extra work or "waste" a little extra space to ensure something is easy to read, understand, and maintain.

 

Take for example this bit from the mysql source.  Which is easier to understand, regard what error occured:

Without their defined names:

int cli_stmt_execute(MYSQL_STMT *stmt)
{
    //...

    if (!stmt->bind_param_done)
    {
      set_stmt_error(stmt, 2031, unknown_sqlstate, NULL);
      DBUG_RETURN(1);
    }
    if (mysql->status != MYSQL_STATUS_READY || mysql->server_status & SERVER_MORE_RESULTS_EXISTS)
    {
      set_stmt_error(stmt, 2014, unknown_sqlstate, NULL);
      DBUG_RETURN(1);
    }

    //...
    if (!(param_data= my_memdup(net->buff, length, MYF(0))))
    {
      set_stmt_error(stmt, 2008, unknown_sqlstate, NULL);
      DBUG_RETURN(1);
    }
    //...
}

 

With their defined names:

int cli_stmt_execute(MYSQL_STMT *stmt)
{
    //...

    if (!stmt->bind_param_done)
    {
      set_stmt_error(stmt, CR_PARAMS_NOT_BOUND, unknown_sqlstate, NULL);
      DBUG_RETURN(1);
    }
    if (mysql->status != MYSQL_STATUS_READY || mysql->server_status & SERVER_MORE_RESULTS_EXISTS)
    {
      set_stmt_error(stmt, CR_COMMANDS_OUT_OF_SYNC, unknown_sqlstate, NULL);
      DBUG_RETURN(1);
    }

    //...
    if (!(param_data= my_memdup(net->buff, length, MYF(0))))
    {
      set_stmt_error(stmt, CR_OUT_OF_MEMORY, unknown_sqlstate, NULL);
      DBUG_RETURN(1);
    }
    //...
}

 

The second makes it clear at a glance what exactly the problem is.

[doubledee]

Kicken,

 

So do you have suggestions on coming up with a scheme for my entire site?

 

Obviously it is hard to say what my website will evolve in to, but still is there some generic approach that will keep things organized and avoid "insert_failed_001", "insert_failed_002", "insert_failed_003" because I didn't plan things out properly?

 

 

Also, what about putting an "Error Id" in the "Error Code" so you get the best of both worlds?

 

Instead of 'ERR_ID_MISSING' have 'ERR_10037_ID_MISSING' ??

 

That way every "Error Code" is guaranteed to be unique.

 

 

Debbie

 

 

[kicken]

Kicken,

 

So do you have suggestions on coming up with a scheme for my entire site?

 

I personally keep my pages self-contained so I have little need for passing errors from one page to the other. In instances where I do, I just create some codes for that specific instance that only those two pages know about.  Ex, from one page where a user selects a course to work on:

 

On all the work pages (via common include)

	if ($Course && $Session){
		if (!ValidateCourseDeveloper($Course, $Session)){
			$this->Redirect('course_development.php?err=permission-denied');
		}

		if (CourseIsLocked($Course, $Session)){
			$this->Redirect('course_development.php?err=course-locked');
		}
	}
	else {
			$this->Redirect('course_development.php?err=no-course');
	}

 

Then on the selection page:

if (isset($_GET['err'])){
switch ($_GET['err']){
	case 'permission-denied':
		$smarty->assign("msg", "You do not have access to develop the selected course.");
		break;
	case 'no-course':
		$smarty->assign('msg', 'You must select a session and course to develop.');
		break;
	case 'course-locked':
		$smarty->assign('msg', 'This course has been locked and changes can no longer be made.');
		break;
}
}

 

 

For all the other self-contained pages I usually just have a $Errors array which I put my errors in, then display on the template.  Non validation type errors (eg, sql query failures) are handled by throwing an exception which gets caught at the top-level and then displays an error page to the user.