My Blog, finally started it, would appreciate any opinions on my logic

[j.smith1981]

I am finally getting my blog started.

 

Actually having allot of fun with it to be honest, so far what I have been thinking about is actually working for a change lol.

 

This is more in some ways to show you what I can do actually aswell, all of the below code is obviously in procedural, but would you change anything at all?

 

I mean I know all of the below code is going to be a matter of preference, it works though so far, but I have been reading a few books and sort of memorised allot of it, all of the code below is my own perceptions of course, what do you think?

 

Here's my logic (I have excluded my 'inc.database.php' file, as that's got my mysql database login details), here it is anyways:

<?php
ini_set('display_errors', 1);

// include the database connection!
require_once 'inc.database.php';

function get_category($category_id) {
  $sql = 'SELECT category
          FROM blog_categories
	  WHERE category_id = '.$category_id;
  
  $result = mysql_query($sql);
  $row = mysql_fetch_assoc($result);
  extract($row);
  
  return $category;
}

// function get_responses($category_id, $post_id){
function get_responses($post_id){
  
  $post_id = mysql_real_escape_string($post_id);
  
  $sql = 'SELECT post_title, post_text, date
          FROM blog_posts
	  WHERE isreply = 1 AND parent = '.$post_id;
  
  $result = mysql_query($sql);
  return mysql_num_rows($result);
}

require_once 'header.html';

// if user has not selected a category in the URL then display the category links!
if(!isset($_GET['category'])) {
  printf("<p>Please select a blog category</p>\n");
  
  $sql = 'SELECT category_id, category
          FROM blog_categories
          WHERE avail = \'Y\'
	  ORDER BY category';
  
  $result = mysql_query($sql);
    
  // weird but if there's an error show the user firstly!
  if(!$result) {
    
printf("<p class=\"error\">Could not contact server, please try again later.</p>");
  
  } else {
  
  if(mysql_num_rows($result) > 0) {
    
    // else loop and output the category as links, so get can be retrieved the find all the posts:
    while($row = mysql_fetch_assoc($result)) {
      extract($row); // turns headings of column names into variables, so dont have to use $row['colname'] in database, like so:
      printf("<p><a href=\"index.php?category=%d\">%s</a><br /></p>\n", $category_id, $category);
    }
  
  } else {
    printf("<p>No categories available</p>");
  }
}
  
  } else {
  
    // trim the category, so someone isnt being stupid and tries to create like a DOS with empty spaces in the category URL
trim($_GET['category']); // removes any blank spaces

// if category ident is not empty:
if($_GET['category'] != ''){
  
  // make input safe:
  $category_id = mysql_real_escape_string($_GET['category']);
  
  // now need to select from which category it is!
  $sql = "SELECT post_id, post_title, post_text, date, username, category 
              FROM blog_posts
              LEFT JOIN blog_users ON blog_posts.user_id = blog_users.user_id
              INNER JOIN blog_categories ON blog_posts.category_id = blog_categories.category_id
              WHERE blog_posts.category_id = $category_id AND approved = 1 AND isreply != 1";
  
  $result = mysql_query($sql);
  
  printf("\n<h2>%s</h2>\n", get_category($category_id)); // calls a function to get the category name!
  
  if(mysql_num_rows($result) > 0) {
    // display the rows of posts:
    while($row = mysql_fetch_assoc($result))
    {
      extract($row);
      printf("<h3>%s</h3>\n %s<br />\n By %s  %s \n<br />", $post_title, $post_text, $username, $date);
	  
	  // get the no of replies and maybe even at some point show the oldest replies first:
	  // get_responses($category_id, $post_id);
	  printf("No of comments %d</p>",get_responses($post_id));
    }
  } else {
    printf("<p>No posts exist in this category</p>\n");
}

  } else {
  printf("<p class=\"error\">You did not select a blog category, please try again.</p>");
  }
}
  
require_once 'footer.html';
?>

 

Of course with the init_set('display_errors', 1) is set to true just for debugging though, when I am happy with it going live I will make this 0, so no errors appear to the user, but what do you think?

 

It is of course entirely reading from the database, no input on this page, as that will be on other scripts.

 

Thanks and I look forward to any replies,

Jez.

[Muddy_Funster]

Looks good: well commented, nice tabulation (no select * either  ).  There is, however, no admin alert attached to the error capture - you may want to look into that. Other than that, it may be slight overkill with the functions but that's nothing to worry about here.

 

When you get round to ordering posts by date, I would personaly do it in the SQL, rather than the PHP - as I would have used SQL COUNT(*) to get the post count aswell, but that's neither here nor there as it's a matter of personal prefference.

 

 

All in all

[PFMaBiSmAd]

mysql_real_escape_string() only protects against sql injection when used on string data. Most/all of your queries are expecting numerical data and mysql_real_escape_string won't stop sql injection in those cases. You must either validate your numerical data as being a only a number or cast it as a number to prevent sql injection.

 

You only have error checking logic on one of your mysql_query() results. You need to have error checking, error reporting/logging, and error recovery logic on every query.

 

You are also not always checking if a query returned any row(s) before attempting to fetch and use data that might not exist.

 

You should use double-quotes to start and end your $sql = "..."; statements. This will allow you to put php variables directly into the statements and you won't need to escape single-quotes that you are putting inside the statements.

 

Your get_responses() function is selecting three columns of data and all the matching rows, only to throw that result set away, because all you are doing in that function is to get a count of the number of matching rows. You should use count() in your query so that you only return the count and not all the data itself.

 

You are outputting the category name in your links and other text 'content' on the page. You should use htmlentities() with the second parameter set to ENT_QUOTES on ALL text that you output (anything that is not a HTML tag) so that any html special characters in it won't break the html on your web page. Doing this will also prevent any XSS (cross site scripting) that someone includes in the external data values they send to your script.

[j.smith1981]

Oh no i forgot about that, is_numeric() would be a good way of doing that wouldnt it?

 

Right going to go through that in a sec about error checking and stuff, just wanted to get around the rest of it but I will go back in just a sec to sort that out, thanks for the reminder 

 

Of course counting the no of rows as much as possible before anything returned, again forgot that for a minute then, again thank you for the reminder.

 

Will encase all my sql query strings into double quotes, just sort of rushing really lol, shouldn't ever rush.

 

Of course with the function that gets the no of responses to a post, I'd need to do that count and then return, ahhh I suppose when you think about it critically, thats a case of where you would use that as opposed to using mysql_num_rows() function NICE ONE!!

 

I will run through a few examples of your last point PFMaBiSmAd, but very much appreciated!

 

Also Muddy_Funster thank you ever so much for your help too, yea it's all a matter of personal preference really.

 

Going to keep cracking on with this, I am really quite chuffed to be honest the lack of errors I have made so far, that's really not bad, though there are better ways of doing things (always is).

 

But thank you for both of your replies, again much appreciated,

Jez!

[j.smith1981]

Due to another post I have made, I decided to go from the ground up again on my blog.

 

This time putting in type casting into the categories part like so:

 

<?php
require_once 'header.html';

require_once 'inc.database.php'; // include database connection details
  
  if(!isset($_GET['category'])) { // if user has not selected a category
    
printf("<h3>Please select a blog category below</h3>");

$sql = "SELECT category_id, category\n";
$sql .= "FROM blog_categories\n";
    $sql .= "WHERE avail != 'N'";

$result = mysql_query($sql);
if(!$result) {
  // if a system error with categories, notify users:
  printf("<p>Something went wrong when retrieving categories, please try again later</p>");
  
} else {
  
  // if rows of categories is greater than 0, then display them, if not do message to say there's not any available:
  if(mysql_num_rows($result) > 0) {
    // now display categories
    while($row = mysql_fetch_assoc($result))
    {
	  extract($row);
      printf("<p><a href=\"index.php?category=%d\">%s</a></p>", $category_id, htmlentities($category));
    }
  
  } else { // if there arent any rows returned, show so:
        printf("<p>No categories available, please try again later</p>");
      }	  
}

  } else if(is_numeric($_GET['category'])) { // only checks if the value is numeric so could be 2.22
    $category = (int) $_GET['category'];

// error checking, to cover all bases:
if($category != $_GET['category']) {
  printf("<p>You entered an invalid category id, please go back <a href=\"index.php\">here</a></p>");
}
  }

require_once 'footer.html';
?>

[j.smith1981]

Ok I have been fiddling around with the issues in user authentication.

 

This is ideally for my blog, just completely understand how a timeout function would work, if I wanted to implement that, sort of building up a set of how to use functions in PHP really.

 

With that being said, if I wanted to allow users to login and post comments, if I was to get them to login and check against a session in the MySQL database, which is ideally where I want to store them, I know there's a thing called custom session handlers in PHP but I feel thats a bit over the top of my head at this very moment in time, want to do it the hard way, learn from it and then on a later date incorporate this into my authentication logic.

 

Having said all that above, about users logging in and storing their sessions in a database, would be be a good idea just to set a cookie (as opposed to $_SESSION) set of variables sorry (excuse my explanation of course) and get those variables checked against the session time out say I have set in the database.

 

Would this be a good way to go about this idea/concept?

 

Just for thoughts really,

Jeremy.