3
\$\begingroup\$

I'm really new to PHP, and a little less so in WordPress. I've created a working function that spits out some static HTML into a page template in WordPress. I'd like to know if there is a best practice for this, perhaps someone can help.

function my_html_function() {
    echo '<h4>Title</h4>
        <ul class="ref-list">
            <li id="one">A list item</li>
            <li id="two">A list item</li>
            <li id="three">A list item</li>
            <!-- I have 30 items here... -->
            <li id="thirty">A list item</li>
        </ul>';
    }

And within my WP page template I use:

<?php echo my_html_function(); ?>

This HTML is static and won't be changed.

\$\endgroup\$
  • \$\begingroup\$ You have to come up with a bit more code before asking whether it complies with 'best practice'. \$\endgroup\$ – KIKO Software Mar 5 '15 at 7:28
  • \$\begingroup\$ @KIKOSoftware I didn't realize there was a minimum requirement - perhaps "correct", "good", or "bad" would be better way to describe? I mean, the code works and all, I'm just asking (because I'm new to PHP) if this how some of the more experienced folks would do this. \$\endgroup\$ – alexwc_ Mar 5 '15 at 7:34
  • 1
    \$\begingroup\$ Save your static html into file and just use <?php include 'filename.html' ?> \$\endgroup\$ – shudder Mar 6 '15 at 11:47
3
\$\begingroup\$

There are several issues I have with your function. Some are about your function breaking the basics about what a function is/should do, others are perhaps a tad more personal (as in: a matter of personal preference).

Functions do stuff, without it impacting the rest of the code

It seems almost silly to explicitly state this, but let me explain what I mean. A function is a unit of code, that performs a single task, that might be required to be performed several times. Functions should not have "side-effects". That's a fancy way of saying that calling a function shouldn't disrupt the rest of the code (with the exception of a function throwing an Exception, of course).

Your function, however, generates output without the caller having any means of controlling what, how and to where that output is being sent. The function assumes the caller knows that it'll send output, but that's a rather dangerous assumption to make. A simple example: cookies can't be set after any output has been sent already. Therefore:

my_html_function();
set_cookie();//<-- FAILS!

The function contains an echo, and so output will be sent once a call is made. It might seem like an obvious thing to fix, but in larger projects, things like this can really trip you up, and you'll find yourself wasting hours trying to work out why the cookie isn't being set properly.

Bottom line: Functions return, they don't echo, so change the echo in your function with return, and echo in the template.

PHP stands for PHP: Hypertext Preprocessor

The important bit in this case is Preprocessor. You can use PHP to return/echo strings of markup, but why would you do that? You can embed PHP in HTML just as easily, so why not use that? If needs must, you can always use PHP to include bits and pieces of markup.

Markup isn't static

Sure, the markup your function echoes/returns is static in your case, but markup languages are, well, languages. They're not always static. It's not a big stretch to imagine a situation where you want your function to churn out a block of markup where the title value is different, or the number of list items is variable.
In those cases, you'll want to pass arguments to your function, and you'll find yourself generating markup dynamically. It's not a hard thing to do, and it's done all the time.

To this end, you have a number of options available to you. The easiest (and in this case reasonable) approach would be to string together the markup:

/**
 * ADD THIS, DOCUMENT YOUR CODE:
 * Pass the title string and an associative array of list items
 * @param string $title
 * @param array $listItems
 * @return string
 */
function getTitleList($title, array $listItems)
{
    $title = sprintf('<h4>%s</h4>', htmlentities($title));//<-- sanitize $title, it shouldn't contain markup
    $liNodes = [];//array of li nodes
    //again, we need to sanitize all this
    foreach ($listItems as $id => $value) {
        $liNodes[] = sprintf(
            '<li id="%s">%s</li>',
            str_replace('"', '\'', $id),//we're using "as id delimiter, so id can't use them
            htmlentities($value)//html entities must be removed
        );
    }
    $markup = $title . '<ul class="ref-list">' . implode('', $liNodes) . '</ul>';
    return $markup;
}

The alternative here would be to use an actual DOM parser/builder, like the DOMDocument class, although in this case, that would be overkill IMO. As @KIKOSoftware said: sprintf is marginally slower than regular concatenation, and it could be said that it's a bit harder to use at first. If you want, you can use regular concatenation just as easily:

$title = '<h4>' . htmlentities($title) . '</h4>';//same as sprintf above

Another thing that was pointed out to me in the comments was that, if the contents of any of your tags contains markup, then obviously htmlentities is not what you want to use. If you require your function to be flexible (and allow the insertion of nested tags), you should really look into using an actual DOMDocument-like tool. Something that enables you to build the DOM as you go (creating new nodes, setting attributes and inserting them throughout the actual DOM tree).

\$\endgroup\$
  • \$\begingroup\$ Indeed, double quotes should be filtered out of the $id. I don't agree with translating everything to html entities though. What, for instance, if a link <a href=... is part of your list item? Or a piece of bold text? I think it's too restrictive. Also sprintf() tends to be a tat on the slow side: micro-optimization.com/… \$\endgroup\$ – KIKO Software Mar 6 '15 at 11:35
  • \$\begingroup\$ @KIKOSoftware: sprintf is indeed a tad slow, but I like its readability (though that's subjective). I'll add concatenation as an alternative. If markup is part of the data, but isn't always required, you're better off using DOMDocument and actually build the markup with a tool designed to do just that IMHO \$\endgroup\$ – Elias Van Ootegem Mar 6 '15 at 11:54
  • \$\begingroup\$ This is a very comprehensive answer! Perhaps my question isn't clear, or I'm missing something in your answer because I know so little: The HTML isn't from a user input, it's just an HTML list that won't be changed, but has to be included on a number of specific pages - I could cut and paste this onto each page, but future pages need to have this. \$\endgroup\$ – alexwc_ Mar 6 '15 at 15:10
1
\$\begingroup\$

Instead of just echoing a chunck of HTML you could use a bit more of the power of PHP. Your function is very specialized, good for only one thing. Try to generalize more. For instance:

function markupList($title,$items,$class = 'list') 
{
    // this will be the output
    $html = '';
    // show title, if it exists
    if (trim($title) != '') $html .= "<h4>$title</h4>".PHP_EOL;
    // make list of items, if there are any
    if (is_array($items) && (count($items) > 0))
    {
        // start with the list wrapper
        $html .= '<ul class="'.$class.'">'.PHP_EOL.
        // then do all the list items
        foreach ($items as $id => $item) 
        {
            $html .= '  <li id="list_'.$id.'">'.$item.'</li>'.PHP_EOL;
        }
        // and end again with the wrapper
        $html .= '</ul>'.PHP_EOL;
    }
    return $html;
 }

 // define a list, not using any HTML
 $myTitle = 'My first very own title!';
 $myItems = array('one'   => 'A list item',
                  'two'   => 'A list item',
                  'three' => 'A list item');

 // turn my list into HTML and echo it
 echo markupList($myTitle,$myItems,'ref-list'); 

As you can see, that looks quite different from what you had. You now have a more general function, it can turn any list into HTML for you.

Of course this does look a bit overdone for what it does, but hey, that's what you gave me to play with. There is much more 'good practice' in this:

  • Note the naming convention.
  • Always comment everyting you do, so in years time it's easier to understand.
  • Try to abstract and generalize.
  • Always check for problems, like no title or items.
  • Note the { and }, there two conventions, the one you used, I choose the other.
  • By returning the html from the function, instead of echoing it inside the function, I can work with the html after it has been generated.
  • I seperated content from form. I can define a list without thinking about html.

But I agree, this does look nothing like what you wrote. At bit more code, but also more to talk about.

\$\endgroup\$
  • 1
    \$\begingroup\$ What if $title is: GOTCHA<script>window.location='http://spyware-central.com'</script>? There's no sanitation anywhere, that's a big omission \$\endgroup\$ – Elias Van Ootegem Mar 6 '15 at 11:09
  • \$\begingroup\$ @Elias Van Ootegem: I don't think this is a sensible remark. If, and I stress IF, the $title were to come from user input, then it should have been filtered at the source, not at this late stage. \$\endgroup\$ – KIKO Software Mar 6 '15 at 11:12
  • \$\begingroup\$ OP is new to PHP, and is using WP, failing to sanitize data is a common mistake we've all made when we were new to programming. Regardless of where the values should be sanitized (I agree, that's best done ASAP), it's worth mentioning the need for actual sanitation, and what the risk are \$\endgroup\$ – Elias Van Ootegem Mar 6 '15 at 11:52
  • \$\begingroup\$ @KIKOSoftware I would disagree with that. You should prevent XSS as late as possible, ie when displaying data. You can have early filtering in addition (I would actually prefer something like mod_security for this instead of adding code), but only filtering when getting the input is a bad idea (you don't know where it will be put, when echoing you don't know where it came from, etc). And in wp only admin and editor have the right to publish unfiltered HTML, and I wouldn't just assume that eg authors don't have access to this function. \$\endgroup\$ – tim Mar 6 '15 at 12:18
  • \$\begingroup\$ Yeah, WP isn't my thing. I'm sure you're both right that some filtering is needed there. Anyway, the object of the example wasn't to provide the best WP output function possible, but to illustrate some points I was trying to make. \$\endgroup\$ – KIKO Software Mar 6 '15 at 12:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.