I'm having trouble with a Form...

[Stalingrad]

Hey guys. I have created a forum for my website from scratch. Everything is working great except the part where you create the actual TOPIC. (When you start a new topic). The problem is, I have a select box saying the name like, ie.. "General Chat" and "Help Chat" as two of the options in the dropdown. The topics have a TOPIC ID that also needs to be filled in. How do I go about filling in the topic ID? I don't want to have to go through having to code if statement for every board topic... I have like 8 topics... I know there must be a way around this. I will post the code that has the part where you create the topic.

 

if(isset($newtopic)) {
$name = $_POST['tname'];
$tbody = $_POST['tbody'];
$tcat = $_POST['tcat'];
$nownow = $_POST['submit'];
if(!isset($nownow)) {
?>
<form action="<?php echo "$PHP_SELF"; ?>" method="POST">
Topic Title: <input type="text" name="tname"><br>
<select name="topic"><option value="1">General Chat</option>
<option value="2">Help Chat</option>
</select><br><br>Body:<br><textarea name="tbody" cols="38" rows="8"></textarea><br><br><input type="submit" name="submit" value="Create Topic"></form>
<?php
}
if(isset($nownow)) {
mysql_query("INSERT INTO topics (name, username, body, countit, boardin, created) VALUES ('$name', '$showusername', '$tbody', '0', '$tcat', 'sitetime')");
echo "<font color=green>Success! Your Topic has been created.</font>";
}
}
}
?>

I know there are a lot of curly brackets, but those are also for the other parts of my code... I didn't pos the whole thing. ANy help is greatly appreciated, thank you! =]

[coded4u]

your ending the <?php tags before closing the if statement. Strange way of returning html to screen but if you're doing it that way then try...

I'd also just put the name of the page in the form action="example.php"

19054_.php

[Jessica]

Just select them from your database and loop through...

[Christian F.]

C4U: There's nothing odd about closing the PHP tags like that. Granted it's not a recommended way of doing it, but it's a very common way (amongst newbies). On the other hand, your code is just plain wrong. Not only did you fail to remove the opening PHP tags in the form's action attribute, but you've invalidated the use of variables with the single quotes. Your modifications didn't do anything to help, but simply added more problems to the code.

It is commendable that you want to help people, but please do make sure that the stuff you recommend is actually helping and not just compounding on the problem. That includes using proper indentation, commenting and other good coding practises when posting code. Otherwise you're not helping, rather the contrary.

 

Stalingrad: As Jesi stated you should be storing those category IDs (and names) in a table in the database. Simply retrieve them from the database, loop through them, and build the dropdown automatically. Also, remember to verify that the ID sent by the user is a valid category ID, and that he actually has the right to create a new thread in said category.

 

You also need to validate all of the other input you're getting, so that you can ensure that the user posted valid data. That means investigating the input, to make sure it matches a pattern of expected and legit input data. Such as IDs only containing numbers, names not containing any special characters that shouldn't be found in names ($, ; and so forth), to name a few examples.

Next you also need to escape output, to ensure that your SQL and HTML code doesn't break. Either by accident, or someone attacking your site. This is done with mysqli_real_escape_string () or Prepared Statements for MySQL, and with htmlspecialchars () for HTML output. There are plenty of guides available for more information on this, so I recommend a search and read through a few of them. (Lots of varying quality, I'm afraid.)

 

Now, in closing I'd like to correct any damage that C4U might have caused by his changes. Thus, this is the recommended way of writing the code you posted above:

<?php
function write_form ($title = '', $message = '', $categoryID = 0, $error = array ()) {
// Escape output, to prevent XSS attacks.
$title = htmlspecialchars ($title);
$message = htmlspecialchars ($message);

// Add the selected attribute to the correct option value.
// Using arrays for ease of use and readability, but this
// should be a part of the automated code.
$cat = array ();
$cat[intval ($categoryID)] = ' selected="selected"';

// If an error message has been sent, format it properly.
if (!empty ($error)) {
 $error = '<p class="error">'.implode ("<br>\n", $error)."</p>\n";
}

// Create the form using the HereDOC syntax, and return it to the calling function.
return <<<FormContent
$error
<form action="" method="post">
<fieldset>
 <legend for="inp_title">Topic Title:</label>
 <input id="title" type="text" name="tname" value="{$title}">

 <select name="topic">
  <option value="0">Select a category</option>
  <option value="1"{$Cat[1]}>General Chat</option>
  <option value="2"{$Cat[2]}>Help Chat</option>
 </select>

 <label for="inp_message">Body:</label>
 <textarea name="tbody" cols="38" rows="8">{$message}</textarea>
</fieldset>

<fieldset class="buttons">
 <input type="submit" name="submit" value="Create Topic"></form>';
</fieldset>
</form>
FormContent;
}
function process_form ($newtopic) {
// If not $newtopic is set (for whatever reason), exit early.
if (!isset ($newtopic)) {
 return;
}

// If form hasn't been submitted, create an empty form.
if (!isset ($_POST['submit'])) {
 return write_form ();
}

// Create values to hold error message and for validation confirmation purposes.
$check = true;
$error = array ();
// Make sure the topic name only contains legal characters.
if (!$name = validate_name ($_POST['tname'])) {
 // It didn't, so mark validation failed and add to error message for the user.
 $error[] = 'Invalid name.';
 $check = false;
}

// Validate the message itself.
// TODO: You'll want to this be more premissive, though do keep in mind that it increases the security risk.
if (!$tbody = $_POST['tbody']) {
 // Validation failed, so mark as such and add error message.
 // TODO: You'll want to add a better error message here.
 $error[] = 'Topic body not valid.';
 $check = false;
}

// Create an array of the legit topic IDs, and validate the posted ID.
// TODO: You'll want to create this array from the database.
$categories = array (1 => true, 2 => true);
$catID = intval ($_POST['topic']);
if (!isset ($categories[$catID])) {
 // Failed validation, mark as such and add to error message.
 $error[] = "No valid category selected.";
 $check = false;
}

// If validation fails, show the form with the user's data prefilled.
if (!$check) {
 return write_form ($_POST['title'], $_POST['tbody'], $_POST['tcat'], $error);
}

// Create the query template, and add the values after escaping them.
$query = "INSERT INTO topics (name, username, body, countit, boardin, created) VALUES ('%s', '%s', '%s', %d, %d, %s)";
$query = sprintf ($query, $db->real_escape_string ($name), $db->real_escape_string ($showusername),
  $db->real_escape_string ($tbody), 0, $catID, 'NOW()');

// Execute the query, and show error message if it didn't succeed.
if (!$res = mysql_query ($query)) {
 $error[] = 'Could not save data in database, please try again or contact an admin if the error persists.';
 return write_form ($name, $message, $catID, $error);
}

// Retrieve the topic's ID, and redirect the user to prevent refresh-resend problems.
header ("Location: read.php?id=".$res->insert_id);
die();
}
// Process the form, and any potential posted data.
$form = process_form ($newtopic);
?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<title>Example</title>
</head>
<body>
<?php echo $form; ?>
</body>
</html>

 

Read the comments, and the code line by line. Play around with it, and check what every line does. Keep doing so until you understand it all, and can see what it does and (most importantly) why it does it.

[Jessica]

I wouldn't use HEREDOC, for an entire form? I'd use a template. But I do love me some Smarty.

[Christian F.]

Yeah, I'd normally use a template myself, but I figured it'd be a bit overkill to introduce too many things to a beginner.

Though, I prefer my own template engine, thank you very much.

[phpfreak]

Yeah, there's nothing wrong with breaking in / out of PHP and popping in HTMl like that. Pretty common! Not everyone needs an uber php template engine to write a single PHP page that outputs HTML.