Feedback on my code

[bigggnick]

Hey all,

 

I was looking for some feedback on my code. It doesn't seem to be working, maybe you all can find the problem. I also wanted to know what you may have done differently, any suggestions, things to make it more efficient, constructive criticism, etc.  This is my first "big" php project, so don't expect much

 

Basically what it is is an article system where users submit an article. An admin logs in, sees if the article is worth posting, approves it, and can be viewed. The admin section will be protected with .htaccess. Here are the files:

 

Heres the main article page (articles.php)

<?php
include (styles.css);
include (config.php);
  $show = mysql_query("SELECT article_name, snippit, auth_name FROM articles WHERE catagory='$cat'  AND approved='yes'");

while($display=mysql_fetch_array($show)){
echo 'a href="show_article.php?article_name=' . $display[article_name] . '">' . $display[article_name] . '</a>';
echo 'By:' . $display[auth_name];
echo $display[snippit];

?>

 

 

Heres the show_article page

<?php
include (config.php);
include (styles.css);


$article_name = $_GET['article_name'];
$snippit = mysql_query("SELECT snippit FROM articles WHERE article_name='$article_name''");
$content = mysql_query("SELECT article_content FROM articles WHERE article_name='$article_name''");
$author = mysql_query("SELECT auth_name FROM articles WHERE article_name='$article_name''");

echo "<h1>" . $article_name . "</h1>";
echo "By: " . $author . "<br />";
echo $content;

?>

 

 

Heres the code for article_process.php, where the submit form goes to:

<?php
include "config.php";
$article_name = mysql_real_escape_string(htmlentities($_POST['article_name']));
$auth_name = mysql_real_escape_string(htmlentities($_POST['auth_name']));
$snippit = mysql_real_escape_string(htmlentities($_POST['snippit']));
$article = mysql_real_escape_string(htmlentities($_POST['article']));

if (isset($article_name) && isset($auth_name) && isset($snippit) && isset($article))
mysql_query("INSERT INTO articles(auth_name, article_name, snippit, article) VALUES('$article_name', '$auth_name', '$snippit', '$article')");
echo "Article sent.";
else
echo "You must fill the form out!";


?>

 

 

Heres where the admin approves or denys the article:

<?php

include "../config.php";

$article_name = $_REQUEST['article_name'];
//include (styles.css);


  $result = mysql_query("SELECT article_content, auth_name FROM articles WHERE article_name='$article_name' ORDER BY article_name");
  while($row = mysql_fetch_array($result, MYSQL_BOTH)){
echo '<h1>' . $row[article_name] . '</h1>';
echo 'By:' . $row[auth_name];
echo $row[article_content];
echo 'Would you like to approve this article?';
echo '<a href="approve_deny.php?article_name=$article_name&approved=yes">YES</a><a href="approve_deny.php?article_name=$article_name&approved=no">NO</a>';
}


?>

 

 

Heres approve_deny.php:

<?php

include "config.php";

$article_name = htmlentities($_REQUEST['article_name']);
$approval = htmlentities($_REQUEST['approved']);
//include (styles.css);
  
  if ($approval=="yes")
  {
  mysql_query("UPDATE articles SET approval = 'yes' WHERE article_name = '$article_name'");
}
elseif ($approval=="no")
{
mysql_query("DELETE FROM articles WHERE article_name='$article_name'");

}
else
{
echo "Either approve it or dont.";
}
?>

 

 

Ya, I know. It needs a lot of work. A lot.

 

Thanks,

 

Nick

[btherl]

htmlentities() is for escaping text for display in an HTML document.  For processiong input variables, take a look at the example at the top here.

 

After you've removed html escaping on your input, the next step is to make the string safe with mysql_real_escape_string().  Then your string is safe for use in mysql.  Note that it still MUST be enclosed in quotes inside the mysql statement, or else it's not safe.

 

The only other thing I notice right away is your inconsistent syntax for include().  It's also better to use include_once() or require_once(), as that will save you trouble with recursive includes.

[bigggnick]

So what you're saying is I should do something like:

$article_name = urldecode($_REQUEST['article_name']);

 

Is that right?

 

[btherl]

Yes, I've used that with success.  I'm not sure if the html_entity_decode() in the example I linked to is actually necessary.  I've found urldecode() alone to work for me.

 

After that, you should use mysql_real_escape_string() if that data is going into a mysql query.  But if it's going to be displayed in an html page, you should use htmlentities().

 

$article_name = urldecode($_REQUEST['article_name']);
$article_name_for_db = mysql_real_escape_string($article_name);
$query = "SELECT * FROM table WHERE article_name = '$article_name_for_db'";
$article_name_for_html = htmlentities($article_name);
print "<h1>$article_name_for_html</h1>";

[bigggnick]

K, that makes sense