Is this an acceptable way of writing PHP code(in scriptlets)

Question

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>

If this was JSP I'd say absolutely not. You'd use the Java Standard Tag Library. I'm not sure if there is an equivalent for PHP. — Michael K
– Michael K, Commented Mar 14, 2012 at 18:16
I would suggest a framework or if it's just a simple site use a template library to separate as much as possible the logic from the view. — AntonioCS
– AntonioCS, Commented Mar 14, 2012 at 18:56

bjelli · Accepted Answer · 2012-03-16 06:27:51Z

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>

Quentin Pradet · Accepted Answer · 2012-03-16 08:56:39Z

1

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): and endif; is equivalent to if (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.

edited Mar 16, 2012 at 8:56

answered Mar 14, 2012 at 18:26

Quentin Pradet

7,0641 gold badge25 silver badges44 bronze badges

\$\begingroup\$ PHP in the beginning was a templating language. NOW it's nothing of the sort. Please stop saying stuff like this. \$\endgroup\$

AntonioCS
– AntonioCS

2012-03-14 18:54:07 +00:00
Commented Mar 14, 2012 at 18:54
1

\$\begingroup\$ It's not a templating language, but a lot of features help doing that. Templating code in PHP only serve to limit the power of templates, and frameworks such as CodeIgniter don't try to provide a templating language: you're encouraged to simply use PHP. So yes, at its core, it's a templating language. \$\endgroup\$

Quentin Pradet
– Quentin Pradet

2012-03-14 18:57:07 +00:00
Commented Mar 14, 2012 at 18:57
\$\begingroup\$ Another way to see it is to say that the subset of PHP I showed in my answer is a useful templating language, and no templating tool would make it better (except at limiting feature abuse). \$\endgroup\$

Quentin Pradet
– Quentin Pradet

2012-03-14 18:59:24 +00:00
Commented Mar 14, 2012 at 18:59
1

\$\begingroup\$ According to PHP manual the mysql_* functions are not deprecated. Though there exists the improved mysqli, I'd say it's actually a worse API than the older one. \$\endgroup\$

Rene Saarsoo
– Rene Saarsoo

2012-03-14 22:22:51 +00:00
Commented Mar 14, 2012 at 22:22
1

\$\begingroup\$ I think this advice is mostly about prepared statements, which allow you to write queries that are more secure. \$\endgroup\$

Quentin Pradet
– Quentin Pradet

2012-03-15 08:16:32 +00:00
Commented Mar 15, 2012 at 8:16

| Show 1 more comment

Matteo Riva · Accepted Answer · 2012-03-16 13:56:13Z

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.

Michael · Accepted Answer · 2012-03-16 14:26:23Z

1

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.

answered Mar 16, 2012 at 14:26

Michael

5892 silver badges7 bronze badges

Add a comment |

Stack Exchange Network

Is this an acceptable way of writing PHP code(in scriptlets)

4 Answers 4

You must log in to answer this question.

Hot Network Questions

Is this an acceptable way of writing PHP code(in scriptlets)

4 Answers 4

You must log in to answer this question.

Related

Hot Network Questions