Related links for WordPress posts

Question

I have written the following class which is part of a pagination system in Wordpress. The class works as expected and also works as expected when used in the system.

Just as background, the class performs the following task

Takes an array of post ID's and creating links to the posts which ID's is supplied in the array.
Uses Wordpress functions get_permalink(), add_query_arg() and get_post_field() to retrieve the appropriate information to build the links

I need a review to asses the correctness of my code, basically how correct is my code, and also, how correct is my use of PHPDoc blocks and the information in these doc blocks.

Here is my class: (I have left out the interface)

<?php
namespace PG\Single\Post\Navigation;

/**
 * PostLinks class
 *
 * Creates clickable post links for post ID's given
 *
 * @param (array) $postIDs          Array of post IDs
 * @param (array) $extraQueryVars   Array of query variables to add to the URL's
 * @param (array) $args             Array of arguments. See below
 * - @param (bool)   previous          Whether or not to get adjacent post older or newer to current post Default true
 * - @param (bool)   boundary          Whether or not to get the boundary posts Default false
 * - @param (bool)   first             Whether or not to get the first or last post when the boundary parameter is set to true Default true
 * - @param (string) anchorText        Text to be used as an anchor text Default %anchor uses post title
 * - @param (string) postLinkText      Text to be used as link text Default %text uses post title
 * - @param (string) spanTextOldest    Text to be used as oldest post link Default Oldest post:
 * - @param (string) spanTextNewest    Text to be used as newest post link Default Newest post:
 * - @param (string) spanTextPrevious  Text to be used as older post link Default Older post:
 * - @param (string) spanTextNext      Text to be used as newer post link Default Newer post:
 *
 * @since 1.0.0
 */
class PostLinks implements PostLinksInterface
{
    /**
     * @since 1.0.0
     * @access protected
     * @var (array) $postIDs
    */
    protected $postIDs;

    /**
     * @since 1.0.0
     * @access protected
     * @var (array) $extraQueryVars
    */
    protected $extraQueryVars;

    /**
     * @since 1.0.0
     * @access protected
     * @var (array) $args
    */
    protected $args;

    /**
     * Sets the default arguments.
     *
     * @since 1.0.0
     * @access protected
     * @var (array) $defaults
    */
    protected $defaults = [
            'previous'           => true,
            'boundary'           => false,
            'first'              => true,
            'anchorText'         => '%anchor',
            'postLinkText'       => '%text',
            'spanTextOldest'     => 'Oldest post: ',
            'spanTextNewest'     => 'Newest post: ',
            'spanTextPrevious'   => 'Older post: ',
            'spanTextNext'       => 'Newer post: ',
        ];

    /**
     * Public constructor method
     *
     * @param (array) $postIDs          Array of post IDs
     * @param (array) $extraQueryVars   Array of query variables to add to the URL's
     * @param (array) $args             Array of arguments. See below
     * - @param (bool)   previous          Whether or not to get adjacent post older or newer to current post Default true
     * - @param (bool)   boundary          Whether or not to get the boundary posts Default false
     * - @param (bool)   first             Whether or not to get the first or last post when the boundary parameter is set to true Default true
     * - @param (string) anchorText        Text to be used as an anchor text Default %anchor
     * - @param (string) postLinkText      Text to be used as link text Default %text
     * - @param (string) spanTextOldest    Text to be used as oldest post link Default Oldest post:
     * - @param (string) spanTextNewest    Text to be used as newest post link Default Newest post:
     * - @param (string) spanTextPrevious  Text to be used as older post link Default Older post:
     * - @param (string) spanTextNext      Text to be used as newer post link Default Newer post:
     *
     * @since 1.0.0
     */
    public function __construct($postIDs = null, $extraQueryVars = null, $args = [])
    {
        $this->setPostIDs($postIDs);
        $this->setExtraQueryVars($extraQueryVars);
        $this->setArgs($args);
    }

    /**
     * Setter setPostLinks()
     *
     * Sets an array of posts IDs
     *
     * @since 1.0.0
     * @param $postIDs
     * @return $this
     */
    public function setPostIDs($postIDs)
    {
        $this->postIDs = filter_var($postIDs, FILTER_VALIDATE_INT, ['flags'  => FILTER_FORCE_ARRAY]);
        return $this;
    }

    /**
     * Returns the posts IDs.
     *
     * @since 1.0.0
     * @return (array) $this->postIDs
    */ 
    public function getPostIDs()
    {
        return $this->postIDs;
    }

    /**
     * Setter setExtraQueryVars()
     *
     * Sets an array of additional query variables to add to the URL's
     *
     * @since 1.0.0
     * @param $extraQueryVars
     * @return $this
     */
    public function setExtraQueryVars($extraQueryVars)
    {
        $this->extraQueryVars = $extraQueryVars;
        return $this;
    }

    /**
     * Returns the array of query variables.
     *
     * @since 1.0.0
     * @return (array) $this->extraQueryVars
    */ 
    public function getExtraQueryVars()
    {
        return $this->extraQueryVars;
    }

    /**
     * Setter setArgs
     *
     * Sets the arguments and merges them with the defaults and also cast array to an object.
     *
     * @since 1.0.0
     * @param $args
     * @return $this
     */
    public function setArgs($args)
    {
        $this->args = (object) array_merge($this->defaults, $args);
        return $this;
    }   

    /**
     * Returns an object of arguments.
     *
     * @since 1.0.0
     * @return (object) $this->args
    */ 
    public function getArgs()
    {
        return $this->args;
    }

    /**
     * Conditional tag to check if the boundaryPosts parameter is set to true. Any other value returns false.
     *
     * @access private
     * @since 1.0.0
     * @return (bool) true on success false on failure
    */ 
    private function isBoundary()
    {
        return $this->args->boundary === true ? true : false; 
    }

    /**
     * Conditional tag to check if the previous parameter is set to true. Any other value returns false.
     *
     * @access private
     * @since 1.0.0
     * @return (bool) true on success false on failure
    */ 
    private function isPrevious()
    {
        return $this->args->previous === true ? true : false; 
    }

    /**
     * Conditional tag to check if the first parameter is set to true. Any other value returns false.
     *
     * @access private
     * @since 1.0.0
     * @return (bool) true on success false on failure
    */ 
    private function isFirst()
    {
        return $this->args->first === true ? true : false; 
    }

    /**
     * Text to be used as post link pre-text according to parameter values set
     * by previous, boundary and first
     *
     * @access private
     * @since 1.0.0
     * @return (string) $text
     */
    private function spanTextText()
    {
        $text = null;

        if ($this->isBoundary() !== true) {

            if ($this->isPrevious()) {

                $text = filter_var($this->args->spanTextPrevious, FILTER_SANITIZE_STRING);

            } else {

                $text = filter_var($this->args->spanTextNext, FILTER_SANITIZE_STRING);

            }

        } else {

            if ($this->isFirst()) {

                $text = filter_var($this->args->spanTextOldest, FILTER_SANITIZE_STRING);

            } else {

                $text = filter_var($this->args->spanTextNewest, FILTER_SANITIZE_STRING);

            }

        }
        return $text;
    }

    /**
     * CSS classes to be used for post links according to parameter values set
     * by previous, boundary and first. 
     *
     * @access private
     * @since 1.0.0
     * @return (string) $classes
     */
    private function linkclasses()
    {
        $classes = null;

        if ($this->isBoundary() !== true) {

            if ($this->isPrevious()) {

                $classes = 'previous';

            } else {

                $classes = 'next';

            }

        } else {

            if ($this->isFirst()) {

                $classes = 'oldest';

            } else {

                $classes = 'newest';

            }

        }
        return $classes;
    }

    /**
     * Create the post links according to input values of the class
     *
     * @since 1.0.0
     * @return (string) $link
     */
    public function links()
    {
        $link  = '';

        if ($this->postIDs !== null) {

            $link .= '<div class"paginate-nav-links ' . $this->linkclasses() . '">';

                foreach ($this->postIDs as $key=>$postID) {

                    /*
                     * Get post post_title according to ID.
                     *
                     * @uses get_post_field()
                     * @see http://codex.wordpress.org/Function_Reference/get_post_field
                     */
                    $postTitle = get_post_field('post_title', $postID);

                    /*
                     * Test to see if WP_Error is not triggered. If so, continue 
                     */
                    if (is_wp_error($postTitle))
                        continue;

                    /*
                     * Made it to here, build the post links.
                     */
                    if ($this->extraQueryVars === null) {   

                        /**
                         * Get the post permalink.
                         *
                         * @uses get_permalink()
                         * @see https://codex.wordpress.org/Function_Reference/get_permalink
                         */
                        $url = get_permalink($postID);

                    } else {

                        /*
                         * Test if $this->extraQueryVars is a valid array. Throw exception on error
                         */
                        if (!is_array($this->extraQueryVars)) {

                            throw new \InvalidArgumentException(
                                sprintf(
                                    __('%s: The value of $extraQueryVars should be an array. Please recheck the the $extraQueryVars input'),
                                    __METHOD__ 
                                )
                            );

                        }

                        /*
                         * If an array of query vars is set. sanitize the array and add to the URL
                         */
                        foreach ($this->extraQueryVars as $k=>$v)
                            $vars[filter_var($k, FILTER_SANITIZE_STRING)] = filter_var($v, FILTER_SANITIZE_STRING);

                        /**
                         * Add the custom query variables to the post URL
                         *
                         * @uses add_query_arg()
                         * @see https://codex.wordpress.org/Function_Reference/add_query_arg
                         */
                        $url = add_query_arg($vars, get_permalink($postID));

                    }

                    /*
                     * If defaults are used, $anchor and $linkText will default to post titles
                     */
                    $anchor   = $this->args->anchorText == '%anchor' ? $postTitle :  $this->args->anchorText;
                    $linkText = $this->args->postLinkText == '%text' ? $postTitle :  $this->args->postLinkText;

                    if ($key === 0) {
                        /*
                         * Don't print any mark-up if $this->spanTextText() is empty or null
                         */
                        if ($this->spanTextText()) {
                            $link .= '<div class"paginate-nav-links ' . $this->linkclasses() . '-text">'; 
                            $link .= filter_var($this->spanTextText(), FILTER_SANITIZE_STRING);
                            $link .= '</div>';
                        }
                            $link .= '<div class"paginate-nav-links links">';
                    }
                            $link .= '<div class"paginate-nav-links link-' . ($key + 1) . '">';
                            $link .= '<a href="' . filter_var($url, FILTER_SANITIZE_URL) . '" title="' . filter_var($anchor, FILTER_SANITIZE_STRING) . '">';
                            $link .= filter_var($linkText, FILTER_SANITIZE_STRING);
                            $link .= '</a>';
                            $link .= '</div>';

                    if (!array_key_exists(($key + 1), $this->postIDs)) {
                            $link .= '</div>';
                    }
                }

            $link .= '</div>';
        }

        /*
         * return null if no post links exists
         */ 
        if (!$link)
            $link = null;

        /*
         * return a string holding the post links
         */ 
        return $link;
    }
}

Community · Accepted Answer · 2020-06-10 13:24:26Z

I haven't really looked into your code that much, but the first thing I noticed is that you have a number of methods (like the constructor) that you expect to be an array.

That's fine, sometimes you want an array to be passed. But if you really need an argument to be an array, enforce it by using a type hint:

public function thisMethodRequiresAnArray(array $argument)
{
    return is_array($argument);//will ALWAYS be true
}

Whenever this method is called, and the provided argument is not an array, an error will be raised.
However, in case of your constructor, it's clear that the array you expect the user to pass needs to be a specific format. You require certain keys to be present, and these keys will have an impact on the way your class behaves.

I'd strongly suggest you write a PostLinksConfig class, which defines all of the properties your class requires, and initializes them to a default value (the default array you declare in your class can serve as a template here). That way, you can type-hint for that config class:

public function __construct(array $postIds = null, array $extraQueryVars = null, PostLinksConfig $args = null)
{
}

That way, you're certain that $args->getPrevious(); will always return a value (either the default, or a value the user set on the PostLinksConfig instance).
What's more, if you write the config class with protected/private properties, and use setters, you can validate, format and normalize the parameters the user provides you with, and throw exceptions if a really outlandish value is being set. For example: you expect previous to be a boolean, so in the config class, your setter could look like this:

public function setPrevious($prev)
{
    //either check type:
    if (!is_bool($prev)) {
        throw new InvalidArgumentException('Previous MUST be a bool');
    }
    //or cast the value to a boolean
    $this->previous = (bool) $prev;
    return $this;
}

In addition to giving you more control over the format of the arguments the user passes to your methods, this will cut down on the rather verbose doc-blocks you have now. Your constructor can be documented like so:

/**
 * Constructor - Some description here...
 *
 * @param array $postIds = null <-- add the default value
 * @param array $extraQueryVars = null
 * @param PostLinksConfig $args = null
 */
public function __construct()
{}

there's just no need to document what $args should/can contain, as this is already documented by the type-hint: the class dictates the format, and if its methods are documented, then the @param annotation should suffice.

I've also noticed you have some @return annotations that look like this:

/**
 * @return (array) $this->postIDs
 */

I'd simply write @return array. The @return annotation is mostly there for the benefit of the user. If I'm using any half-decent IDE, it'll use the doc-blocks for auto completion, and it'll let me know what the return value of any given method is. To be able to so, the key parts are @return, to signal what information the annotation will provide and array (or any other type), to let me know what is being returned.
I couldn't give a damn if the return value is coming from property X or Y of that class, it could even be a hard-coded array for all I care. Classes are about abstracting the nitty-gritty away from the user. I don't need to know how a class does what it does, I just need to know how to use it, and what to expect from it.

If nothing else, by leaving the property names out of the annotations, you save yourself the bother of having to change your annotations whenever you decide to rename a property. I know, it's not a great argument, but it could be something you might want to consider. Not only in this case, but when writing code in general. Be cleverly lazy: always ensure you're not doing work that has been done before, and write code in such a way that maintaining, debugging and refactoring it will cost you as little effort as humanly possible.

A couple of little nit-picks:

I've also spotted code like this:

return $this->args->first === true ? true : false;

Which could be written a lot shorter:

return $this->args->first;

And assuming you're going to refactor your code to use a config object:

return $this->args->isFirst();//or getFirst(), whichever works best for you

There's a lot of validation (in the form of filter_var calls) going on, which is good, but again: using a config class, with validating setters would make most (if not all) of this validation redundant in the class you've posted here. Another reason why adding a secondary class would be well worth considering.

Last but not least, I'll just leave you with the less-than-helpful remark that the links method should be reworked, and possibly be separated out into another object. Generating markup by stringing it together is a dangerous game to play. It's error prone, and a nightmare to maintain. It might be best to write a utility class that builds around one of PHP's own extensions that handle markup a lot better (DOMDocument or SimpleXMLElement to name a couple). These classes provide you with a (relatively) clean API, and a lot more capabilities than what you're doing now. Adding nodes to the DOM at any given point becomes quite easy if you take the time to get acquainted with their respective API's. As ever, the DOM api's are a bit verbose, and sometimes cumbersome, but in the long run, I promise you it's well worth learning how to use them.

I might come back to this answer, and look at your code a bit more attentive, because right now, it really is time for me to go to bed.

Have fun, hope this helps you along a bit

Update

First off, I'm sorry to have kept you waiting, but I'm rather busy lately, so I couldn't get back to you sooner.

About that links method, though I've not been able to go through it all in great detail, I've noticed a couple of things:

You're only creating div and a elements
All of the divs share the paginate-nav-links class (which might be redundant, if you rewrite part of the CSS, but that's not up for review here).
The a tags are only assigned an href and title attribute, so that logic could be separated out into a non-public method (protected or private depending on your needs)
The entire markup string you're constructing is contained within a single div element

So where to start, I'd suggest using the DOMDocument API, and in this case the DOMElement class in particular. I'd start by initializing $links to null, then simply start using the DOM api:

if ($this->postIDs !== null) {
    $links = new DOMElement('div');
    //either add class attribute like so:
    $links->setAttribute('class', $classString);
    //or opt for a more "scolarly" approach:
    $classAttr = new DOMAttr('class', $classString);
    $links->setAttributeNode($classAttr);
}

The benefit of using the attribute node is that you can add classes later on, depending on what is required, you can also clone it at a certain point, and use that clone to build on for other DOM elements.
The main reason for my brining this up is because I've noticed a lot of class values being the same. At the very least, I'd consider using a sprintf here and there to avoid my having to duplicate string constants all over the place, replacing:

'<div class"paginate-nav-links ' . $this->linkclasses() .'">';

With:

//outside of any loop
$classFormat = 'paginate-nav-links %s';
$classNames = sprintf(
    $classFormat,
    $this->linkclasses()
);

At least, you'll save yourself the trouble of having to refactor the entire method whenever the CSS classes change.

Next, building the rest of the DOM is quite easy. Depending on where you want to add a given child to the DOM (the links, for example, are children of a DIV, that is in turn the child of a div (created when $key === 0 and closed after processing the links, which is again a child of the outer div we've created in the snippet above.

Instead of constructing the div containing the "spanTextText()" value, and all of the links inside of a loop, I'd construct it outside of it. Simply because the if ensures us that there will be at least one link. Same goes for that spanTextText business, so:

if ($this->spanTextText()) {
    $span = new DOMElement('div', $this->spanTextText());//set its inner value
    $span->setAttribute(
        'class',
        sprintf(
            $classFormat,
            $this->linkclasses().'-text'
        );
    $link->appendChild($span);//add to the main $link element
}
$container = new DOMElement('div');
$container->setAttribute('class', sprintf($classFormat, 'links'));
$link->appendChild($container);

Inside the loop, you'll simply be appending child elements to the $container object, and each of these elements can contain children of their own (which they will: the actual links).

Now say that, for some reason, the $container object was already added to the dom, but to get to the spanTextText value, you'd actually have to perform the loop, in that case, just keep the code above (creating and adding $container to $link), and simply add the child where you want it (before $container in this case):

foreach ($this->postIDs as $key=>$postID) {
    //do stuff
    if ($key === 0 && $this->spanTextText()) {
        //create $span like before, then:
        $link->insertBefore($span, $container);//inserts $span before $container
    }
    //adding a link to the container
    $inner = new DOMElement('div');
    $inner->setAttribute('class', sprintf($classFormat, 'link-'.($key+1)));
    $a = new DOMElement('a', $linkText);
    $a->setAttribute('href', $url);
    //add link to inner div
    $inner->appendChild($a);
    //add inner div to container (which is already added to $link)
    $container->appendChild($inner);
    //you can still update the nodes you've added:
    $a->setAttribute('title', $anchor);
}

So as you can see, the DOM API is pretty flexible, and more than capable of constructing the markup you need. However, to get to the actual markup string, you'll need to use the DOMDocument::saveHTML method, which isn't that hard either. However, at this point I'm not sure which approach will work best/as expected, so I'll list all of the possible approaches, based on the docs

//after loop:
$dom = new DOMDocument();
//you may or may not need this bit:
$dom->appendChild($link);//add to own DOM
$htmlString = $dom->saveHTML($link);//should yield markup for $link
//alternatively, one that I know for sure that works
$body = $dom->createElement('body');
$dom->appendChild($body);
$body->appendNode($link);
//or, one that I know for sure will work:
$htmlString = substr(
    $dom->saveXML($body),
     6, -7//6 and -7 => strlen('<body>'), -1*strlen('</body>')
);

The saveXML usage is documented here

Anyway, that's how I'd generate markup: by using a DOM API instead of having to rely on error-prone, hard to maintain code that strings together markup.

Thanks for your input. Appreciated. It seems that my main issue here is the links() method as it uses Wordpress functions. To be honest, I really don't know how to make that method work otherwise. I would appreciate it if you can revisit this some other time and maybe put your thoughts into a small code example of how the links() method can be refactored. I'm not in a hurry, this is a project I'm working on on the sideline for myself for now to improve my coding skills. In the mean time I can work on your other suggestions which looks very promising. Again, thanks for your suggestions. — Pieter Goosen
– Pieter Goosen, Commented Mar 25, 2015 at 4:26
@PieterGoosen: Managed to find some time, updated my answer, explaining how I'd set about generating valid markup — Elias Van Ootegem
– Elias Van Ootegem, Commented Apr 1, 2015 at 10:39
Great, you havegiven me something that I have never thought about, this isth problem with being stuck to one platform :-). Thanks for taking time and replying back. Your idea of creating a separate arguments class worked perfectly taking my complete project in consideration. I have the complete project coded in spaghetti, rewritting it to OOP is a new and fun way to learn the basics. I'm happy to award you the bounty as you have really helped me a lot, not just in this question, but it others. I really appreciate it. Thanks again — Pieter Goosen
– Pieter Goosen, Commented Apr 1, 2015 at 10:57

Stack Exchange Network

Related links for WordPress posts

1 Answer 1

Update

You must log in to answer this question.

Hot Network Questions