0

I'm writing a database driven admin tool (really, its basically just a GUI to manage the db). I have quite a bit of PHP, most of it mixed in with pure HTML, in use and have started breaking everything up into much smaller pieces to increase readability. However, while this works, I have no idea if this is a good idea, either based on convention or performance.

Here's an example. This code is repeated a bunch for each form field:

 <?php
        print '<div class="form-group">';
        print '<label>Host Description</label>';
        print '<textarea name="Description" class="form-control" rows="3">'.$_GET["Description"].'</textarea>';
        print '</div>';
    ...code is repeated for other fields...
    ?>

I have started changing that to this:

<div class="form-group">
    <label>Host Description</label>
    <?php print '<textarea name="Description" class="form-control" rows="3">'.$_GET["Description"].'</textarea>';?>
</div>
...code is repeated for other fields...

When I say the code is repeated, I mean that different form fields and form field types are displayed one after the other.

Does this make sense? It makes the code much more readable with only the lines that need to be PHP part of the script. I have no idea what kind of conventions I'm breaking or performance hits I'm taking though. Is this a good idea? Bad idea?

  • Just choose the thing that is most readable. – PeeHaa Jun 09 '14 at 18:55
  • Second one is better/more readable but you may also check [MVC](http://www.sitepoint.com/the-mvc-pattern-and-php-1/) which is used to separate code/layers. – The Alpha Jun 09 '14 at 18:59
  • That's wide open to XSS attacks. You should use `htmlentities()` prior to displaying info gotten from the user. – Matthew Johnson Jun 09 '14 at 19:17
  • The site is only available on an internal network (behind an annoyingly high amount of security), but I'll include the htmlentities call since it seems to be the norm. – OfficerHalf Jun 09 '14 at 19:24
  • @MatthewJohnson s/`htmlentities`/`htmlspecialchars` – PeeHaa Jun 09 '14 at 19:57
  • I checked, and you're right, [OWASP PHP Cheatsheet](https://www.owasp.org/index.php/PHP_Security_Cheat_Sheet#XSS_Cheat_Sheet) uses `htmlspecialchars()`. Sad that the first result on Google from SO states `htmlentities`. – Matthew Johnson Jun 09 '14 at 22:02
  • @PeeHaa I think Laravel uses the Blade Template engine, which uses `htmlentities`. Can either be used if encoding is specified? – Matthew Johnson Jun 10 '14 at 13:28
  • @MatthewJohnson `htmlentities` converts *any* character for which there is an entity. In almost all cases this really is not needed. – PeeHaa Jun 10 '14 at 13:34

2 Answers2

-1

Just make a function that print it? Like

function PrintHostDesc()
{
print '<div class="form-group">';
        print '<label>Host Description</label>';
        print '<textarea name="Description" class="form-control" rows="3">'.$_GET["Description"].'</textarea>';
        print '</div>';
}

That might make the code a bit more readable and shorter in your editor

robinp7720
  • 443
  • 4
  • 12
  • I've done this for the larger tasks, like gathering a bunch of info from the database, but really there is only one statement of php here ($_GET["Description"]). I clarified what I meant by 'code is repeated', as well. I'd end up with 12 different functions to print a single line. I guess I could have it print different things based on an argument, but then the function loses its readability. – OfficerHalf Jun 09 '14 at 19:06
-1

It's commonly done in this way, I would recommend it:

<div class="form-group">
    <label>Host Description</label>
    <textarea name="Description" class="form-control" rows="3"><?php echo htmlentities($_GET["Description"]) ?></textarea>
</div>
groovenectar
  • 2,828
  • 3
  • 22
  • 26