[RESOLVED] Blog display script

[Pragz]

So here is the article processing script I wrote for a blog I am making. It calls the article ID from what is entered in the URL, makes sure it's an actual ID that corresponds to a written article, then goes on to display the info.

The includes files don't have any PHP in them aside from a thing I added to see how long it takes the pages to load.

So yeah, anything you would do differently? I want this to be as efficient as possible.

<?php


##############################
##	Connection stuff
##############################
$conn = mysql_connect("localhost", "", ""); mysql_select_db('circe_data', $conn);


##############################
##	Checks for the article ID
##############################
if (isset($_GET['article']) && ctype_digit($_GET['article']))
	{
		$article_call = $_GET['article'];
	}
else
	{
		$article_call = -1;
	}


#####
# Check if the article id entered has a corresponding article
#####
$article_check = "SELECT id
				  FROM blog_articles
				  WHERE id = $article_call";

$article_check_result = mysql_query($article_check);

if (!$article_check_result || mysql_num_rows($article_check_result) == 0) {
	$article_call = -1;
	}


#####
# And now we move on
#####
$sql = "SELECT *
		FROM	blog_articles
		WHERE	id = $article_call
		OR		article_title = $article_call";

$result = mysql_query($sql);

if (!$result) {
	echo "Could not successfully run query ($sql) from DB: " . mysql_error();
	exit;
}

if (mysql_num_rows($result) == 0) {
	$article_call = -1;
}

while ($row = mysql_fetch_assoc($result)) {
$article_id = $row["article_id"];
$article_category = $row["article_category"];
$article_title = $row["article_title"];
$article_author = $row["article_author"];
$article_posted = $row["article_posted"];
$article_body = $row["article_body"];
}

mysql_free_result($result);		

#
#	Start of page code
#
include ('includes/page_header.php');

echo "
	<div id=\"outer\">
		<div id=\"inner\">
			<div id=\"cwrap\">
				<div id=\"mcol\">
					<div class=\"content\">

					<div id=\"trail\">
						Current Article: 
						<ol>
							<li>&raquo; <a title=\"Go to the home page.\" href=\"http://www.pantheon.org/\">Home</a></li>
							<li>&raquo; <a title=\"Go to the category index.\" href=\"/areas/\">";echo $article_category; echo "</a></li>
							<li>&raquo; <a title=\"Go to the article index.\" href=\"/areas/mythology/\">";echo $article_title; echo "</a></li>
						</ol>
					</div>
			<h1>";

			echo $article_title;
			echo "

			</h1>
				<div id=\"author\">by $article_author</div>

					<div id=\"article\">";

						echo $article_body;
						echo "

					</div>
				<hr class=\"hide\" />
				</div>
			</div>
			<div class=\"clear\"></div>
		</div>
		<div id=\"rcol\">
			<div class=\"content\">
				<h4>Primary:</h4>";

				include ('includes/side_bar.php');
				echo "

				<hr class=\"hide\" />
			</div>
		</div>
		<div class=\"clear\"></div>
	</div>
</div>
<div id=\"footer\">";

	include ('includes/page_footer.php');
	echo "

</div>

</div>

</body>
</html>";


?>

[Weedpacket]

Well; you check to see if there is an article with the appropriate id, but then you fetch the article you consider the possibility that the article's title might be an ID number and you look there, too.

Also, the whole job could be done in one step. Consider this wee story.

If I want a sandwich I go through the following process: I get up from my desk, walk to the other end of the house, open the fridge, search for and find if I have bread, walk back to my desk, and sit down. Then, if there was bread in the fridge, I get up, walk to the other end of the house, search for and find the bread, get the bread out, sudo make myself a sandwich, close the fridge, and take the sandwich back to my desk.

If you can see the biggest drawback to that procedure then I'll point out that your code has the same drawback.

[bradgrafelman]

Weedpacket wrote:

If you can see the biggest drawback to that procedure

Is it that you didn't buy a mini-fridge that can sit within reach of your desk? 😃

[jkurrle]

I thought it was because he wasn't making enough trips to meet his "calorie burned" quota... =)

[Pragz]

facepalm I see what you mean. I don't know why I used two SQLs for that. >.<

Better?

<?php


##############################
##	Connection stuff
##############################
$conn = mysql_connect("localhost", "", ""); mysql_select_db('circe_data', $conn);


##############################
##	Checks for the article ID
##############################
if (isset($_GET['article']) && ctype_digit($_GET['article']))
	{
		$article_call = $_GET['article'];
	}
else
	{
		$article_call = -1;
	}


##############################
##	SQL, establishes article id, finds if article exists,
##	sets to error article if does not
##############################
$sql = "SELECT *
		FROM	blog_articles
		WHERE	id = $article_call
		OR		article_title = $article_call";

$result = mysql_query($sql);

if (!$result) {
	echo "Could not successfully run query ($sql) from DB: " . mysql_error();
	exit;
}

if (mysql_num_rows($result) == 0) {
	$article_call = -1;
}

while ($row = mysql_fetch_assoc($result)) {
$article_id = $row["article_id"];
$article_category = $row["article_category"];
$article_title = $row["article_title"];
$article_author = $row["article_author"];
$article_posted = $row["article_posted"];
$article_body = $row["article_body"];
}

mysql_free_result($result);	

if (!$article_id || mysql_num_rows($article_id) == 0) {
	$article_call = -1;
	}

#
#	Start of page code
#
include ('includes/page_header.php');

echo "
	<div id=\"outer\">
		<div id=\"inner\">
			<div id=\"cwrap\">
				<div id=\"mcol\">
					<div class=\"content\">

					<div id=\"trail\">
						Current Article:
						<ol>
							<li>&raquo; <a title=\"Go to the home page.\" href=\"http://www.pantheon.org/\">Home</a></li>
							<li>&raquo; <a title=\"Go to the category index.\" href=\"/areas/\">";echo $article_category; echo "</a></li>
							<li>&raquo; <a title=\"Go to the article index.\" href=\"/areas/mythology/\">";echo $article_title; echo "</a></li>
						</ol>
					</div>
			<h1>";

			echo $article_title;
			echo "

			</h1>
				<div id=\"author\">by $article_author</div>

					<div id=\"article\">";

						echo $article_body;
						echo "

					</div>
				<hr class=\"hide\" />
				</div>
			</div>
			<div class=\"clear\"></div>
		</div>
		<div id=\"rcol\">
			<div class=\"content\">
				<h4>Primary:</h4>";

				include ('includes/side_bar.php');
				echo "

				<hr class=\"hide\" />
			</div>
		</div>
		<div class=\"clear\"></div>
	</div>
</div>
<div id=\"footer\">";

	include ('includes/page_footer.php');
	echo "

</div>

</div>

</body>
</html>";


?> 

[bradgrafelman]

Few comments...

1. If $article_call is supposed to refer to the article id, why are you also including the article_title field in the query? If article_data is a string-type column, comparing it to an integer will not only generate a warning but also end up with some odd results depending upon the string data and the integer.

2. You shouldn't use "SELECT " queries unless you absolutely mean to select the entire table, such as when doing an "INSERT INTO .. SELECT" query. Instead, you should always list the columns from which you actually need data.
   

3. Why do you use a while() loop to retrieve the results and yet only keep the last row in the result set? If you're only expecting one row, then there's no need for a loop.
   

4. Why do you pass $article_id to [man]mysql_num_rows/man, a function that expects a MySQL resource?
   

5. Note that this:
   

   $article_id = $row["article_id"]; 
   $article_category = $row["article_category"]; 
   $article_title = $row["article_title"]; 
   $article_author = $row["article_author"]; 
   $article_posted = $row["article_posted"]; 
   $article_body = $row["article_body"]; 

   could be reduced down to this:

   extract($row);

   provided that you addressed comment #2 above.

[Pragz]

1. Ack, that's a bit of code I forgot to remove. I originally had it to where if the article= at the end of the URL had a number or the title name it would pull the article. I liked this way better.

2. I am selecting the entire table and using all of the fields in it.

3. I don't understand what you mean here. ):

4. Wow; I can't believe I wrote that. Thanks for pointing that out.

5. So then how would I write that variables for that in the rest of the page? Just as $row["field_name"]?

   I don't think I'll change that, though. Just for personal preference of variables.

[bradgrafelman]

Pragz wrote:

I don't understand what you mean here. ):

If you are expecting more than one row to be returned, then the while() loop is going to loop through all of the rows and overwrite values as it goes along. The code after the loop will then only see the last values written, a.k.a. the last row in the result set.

If you're only expecting 1 row to be returned (which I suspect is the case, assuming this "id" field is something like an AUTO_INCREMENT column), then it's just silly to use a loop that you know is only going to run once. The point of using a loop is to repeat a section of code a number of times.

Pragz wrote:

So then how would I write that variables for that in the rest of the page? Just as $row["field_name"]?

No - as $field_name, just as you're doing now. Read the manual entry for [man]extract/man.

[Pragz]

Ah, I see what you mean. So then is this better?

##############################
##	SQL, establishes article id, finds if article exists,
##	sets to error article if does not
##############################
$sql = "SELECT *
		FROM	blog_articles
		WHERE	article_id = $article_call";

$result = mysql_query($sql);

if (!$result) {
	echo "Could not successfully run query ($sql) from DB: " . mysql_error();
	exit;
}

if (mysql_num_rows($result) == 0) {
	$article_call = -1;
}

$row = mysql_fetch_assoc($result);
extract($row);

mysql_free_result($result);	

if (!$article_id) {
	$article_call = -1;
	}

[bradgrafelman]

Looks like it, yes, as long as you remove the MySQL error information before using this script on a production site.

[Pragz]

Yeah; I'm going to set it to show an error article when I open it.

Thanks so much for the help!

[Weedpacket]

bradgrafelman wrote:

Is it that you didn't buy a mini-fridge that can sit within reach of your desk?

That's just a symptom of the problem, which is that the mains wiring in this room isn't rated for the load required to run everything and a mini-fridge.

[Pragz]

Ack, not resolved! ):

I was doing a little fiddling around and checked and now one of the functions has stopped working. If they enter an article ID that doesn't exist into the URL, it still acts as if it does and just displays no info. It used to not do that! ):

I don't know what I did to mess that up, and all my attempts at readding that just do nothing.

Here's where my code stands now:

##############################
##    Checks for the article ID
##############################
if (isset($_GET['article']) && ctype_digit($_GET['article']))
    {
        $article_call = $_GET['article'];
    }
else
    {
        $article_call = -1;
    }


##############################
##    SQL, establishes article id, finds if article exists,
##    sets to error article if does not
##############################
$sql = "SELECT *
        FROM    blog_articles
        WHERE    article_id = $article_call";

$result = mysql_query($sql);

if (!$result) {
    echo "Could not successfully run query ($sql) from DB: " . mysql_error();
    exit;
}

if (mysql_num_rows($result) == 0) {
    $article_call = -1;
}

$row = mysql_fetch_assoc($result);
if (!$row) {
    $article_call = -1;
}
else {
    extract($row);
}

mysql_free_result($result);    

if (!$article_id) {
    $article_call = -1;
    }

Any ideas?

[bradgrafelman]

Not sure why you keep using that -1 and all of that... why not just wrap the SQL-related stuff into an if/else block? e.g.

if(mysql_num_rows($result) == 0) {
    // oops, not found.
} else {
    // found - process row here
}

[Pragz]

I already have that exact code in there. ):

[bradgrafelman]

No, you don't. You use that function to see if no rows are found, but instead of handling that as a separate case, you simply set a value to -1 and continue on as if there actually were rows found.

[Pragz]

I've been running in circles with this script... I realized that after my hours of work, it's the exact same script with heavier use of the tab key...

##############################
##	Checks for the article ID
##############################
if (isset($_GET['article']) && ctype_digit($_GET['article'])) {
		$article_call = $_GET['article'];
	}
else {
		$article_call = -1;
	}

$sql = "SELECT *
		FROM	blog_articles
		WHERE	article_id = $article_call";

$result = mysql_query($sql);

if (!$result) {
	echo "Could not successfully run query ($sql) from DB: " . mysql_error();
	}

if (mysql_num_rows($result) == 0) {
	$article_call = -1;
	}
else {

$row = mysql_fetch_assoc($result);

if (!$row) {
		$article_call = -1;
	}
else {
	extract($row);
	}

mysql_free_result($result);	

if (!$article_id) {
	$article_call = -1;
	}
}

I don't know what I'm doing wrong nor how to fix it. ):

[Weedpacket]

It's the $article_call business that's messing you up; you're using it to mean multiple things (it's an article id, then it indicates a query failure, then it indicates no match for the given article id (which has now been lost because it was being stored in $article_call).

I wrote this just now; it's probably a bit broken because I did it while waiting for a bunch of archives to unpack, and waiting for the cat on my lap to get comfortable.

$article_id = 0;
if(!empty($_GET['article']))
{
	$article_id = (int)$_GET['article'];
}

// Select the fields you need, not just *. More efficient that way,
// and you have an explicit list of the values you end up with.
// The "Limit" clause shouldn't be necessary in my opinion: a UNIQUE
// index (automatic in the case of primary keys) on the column would
// be enough to tell the dbms that there can be no more than one match
$sql = "Select * From blog_articles Where article_id=$article_id Limit 1";
$result = mysql_query($sql);
if($result)
{
	$database_error = false;
	if(mysql_num_rows($result))
	{
		$row = mysql_fetch_assoc($result);
	}
	else
	{
		$row = null;
	}
	mysql_free_result($result);
}
else
{
	$database_error = mysql_error();
}

Later, what gets displayed depends on what happened above...

if($database_error)
{
	// The query borked. $database_error contains the error message returned by MySQL.
	// Deal with this.
}
elseif(!$row)
{
	// No article with the given ID was found.
}
else
{
	// I prefer to work out of the array, myself, but that's a matter of taste.
	extract($row);
	// Display the article
}

[Pragz]

That doesn't do the trick, and it has also gotten rid of my sanitation of alphabetic characters. ):

Thanks for the help, though! I may just need to use that second query as before to do this. ):

Edit: Setting the article call to -1 is supposed to render an article, just as if one was found. I manually entered an error article under the ID of -1 and want it to display when anything that isn't found comes up.

I don't know if that helps you understand my problem.

[Pragz]

Okay, I got it! Probably not the wisest way to do this, but it works without adding an extra query.

##############################
##
##	Checks for the article ID
##
##############################
if (isset($_GET['article']) && ctype_digit($_GET['article'])) {
		$article_call = $_GET['article'];
	}
else {
		$article_call = -1;
	}

$sql = "SELECT	*
		FROM	blog_articles
		WHERE	article_id = $article_call";

$result = mysql_query($sql);

if (!$result) {
	echo "Could not successfully run query ($sql) from DB: " . mysql_error();
	}

if (mysql_num_rows($result)) {
	$row = mysql_fetch_assoc($result);

if (!$row) {
	include ('language/error_values.php');
	}
else {
	extract($row);
	}
}
else {
	include ('language/error_values.php');
}

mysql_free_result($result);