Related links for WordPress posts

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)

```
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 D

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

StackExchange Code Review Q#84733, answer score: 3