Problem
Is this the way PHP developers write PHP or is echoing the html the preferred method?
<html>
<head>
</head>
<body>
<?php
require_once 'db_connect.php';
session_start();
$result = mysql_query("SELECT * FROM products");
?>
<table border=1>
<th>
Product ID
</th>
<th>
Name
</th>
<th>
Description
</th>
<th>
Price
</th>
<th>
Image
</th>
<th colspan=2>
Action
</th>
<?php
while($row = mysql_fetch_array($result))
{
?>
<tr>
<td>
<?php
echo $row['p_id'];
?>
</td>
<td>
<?php
echo $row['p_name'];
?>
</td>
<td>
<?php
echo $row['p_description'];
?>
</td>
<td>
<?php
echo $row['p_price'];
?>
</td>
<td>
<?php
echo $row['p_image_path'];
?>
</td>
<td>
edit
</td>
<td>
delete
</td>
</tr>
<?php
}
?>
</table>
</body>
</html>
Solution
A few points:
Put the session_start() and as much of processing as possible on top of the file, before any output. This will be useful for emitting http-headers.
When using a database: don’t forget to handle exceptions.
When using a database: don’t forget to handle charsets.
When selecting from the database: always limit your results. Add pagination of some kind.
Do not use mysql_*, use a db library that offers prepared statements and is as db-independent as possible. I would recommend PDO. I would also recommend getting the data from the db as objects. If necessary you can define a class to hold some methods you need for these objects.
I cuncur with Cygal: using php as the templating language is quite ok. See WordPress for an example where this is used extensively in themes.
Try to write your HTML in a compact, readable way. No matter what you use for templating: HTML is an important part of your code, and should be just as readable as any other code you write.
Output valid HTML. Don’t forget question marks around attribute values, don’t forget -Tags.
<?php
session_start();
include "config.php";
if( ! $DB_NAME ) die('please create config.php, define $DB_NAME, $DB_USER, $DB_PASS there');
try {
$dbh = new PDO("mysql:dbname=$DB_NAME", $DB_USER, $DB_PASS);
$dbh->exec('SET CHARACTER SET utf8') ;
$result = $dbh->query("SELECT * FROM products LIMIT 0,20");
$products = $result->fetchAll(PDO::FETCH_OBJ);
} catch( Exception $e ) {
error_log( $e->getMessage() . " ". $e->getTraceAsString() );
include("500-failwhale.php");
exit;
}
?>
<!DOCTYPE html>
<html>
<head>
<title>Products</title>
<meta charset="utf-8">
</head>
<body>
<table class="with_border">
<tr>
<th>Product ID </th>
<th>Name </th>
<th>Description </th>
<th>Price </th>
<th>Image </th>
<th colspan="2"> Action </th>
</tr>
<?php foreach($products as $p) : ?>
<tr>
<td><?= $p->p_id ?></td>
<td><?= $p->p_name ?></td>
<td><?= $p->p_description ?></td>
<td><?= $p->p_price ?></td>
<td><?php if ( $p->p_image_path ) : ?>
<img src="<?= $p->p_image_path ?>" alt="<?= $p->p_name ?>">
<?php endif; ?></td>
<td>edit</td>
<td>delete</td>
</tr>
<?php endforeach; ?>
</table>
</body>
</html>
In small scripts, it’s OK. PHP has features to make this easier:
<?= $var ?>is equivalent to<?php echo $var ?>(such short tags should not be used at the beginning of the document).if (condition):andendif;is equivalent toif (condition) {and}. (This alternative syntax allows you to mark clearly the end of ifs and loops)
PHP is at its core a templating language, which is why it works so well, and even though you could implement something else, it would provide no benefit. For larger programs, you would however separate concerns and use a framework encouraging you to have a view whose only role is to display HTML (CodeIgniter is a good example).
Finally, don’t use mysql_*, it’s not secure! Consider using prepared statement in mysqli or PDO.
Is this the way PHP developers write PHP or is echoing the html the preferred method?
In my opinion, neither are.
The first and most important thing to learn about writing good PHP code is that you must separate the code that gathers data from the code that outputs it.
In your sample code this means that you don’t call your data extracting function and display what you get, but you stuff everything in an array and later display its contents. This is really important: even if you write everything in one script, the two pieces of your code should be completely separated so that it could be possible to split them in two files with close to no changes.
This means that you can change how you display your data without touching how you get them, or you can change the source of your data and keep the output exactly as it is (so long as you follow some conventions regarding variable names and data formats).
As already mentioned, you can’t call session_start() after output has already being sent (your output being the first lines of HTML) as starting a session implies setting headers and you can’t do that once they have already been sent.
Also I would advice against the use of short open tags (<?= $var ?>) @Cygal suggests, as they are not recommended and by default disabled on many hosting services.
Regarding the interface to the database, while in simple scripts it can be okay, I think it’s useful to skip mysql_* functions in favor of the PDO interface which gives you a lot of advantages (most notably prepared statements) at the little cost of learning its syntax.
I would recommend to change the while loop, so it will handle any rows:
while($row = mysql_fetch_array($result))
{
echo '<tr>'
foreach ($row as $key => $value) {
echo '<td>' . $value . '</td>'
}
echo '<td> edit </td> <td> delete </td> </tr>'
}
and if you do not want to output all values from the row, but just a subset, or if you want particular order in output it is better to modify your SQL query as appropriate.