We are all used to WordPress magically deciding which post is the current one, and we have all written functions like follows. But I will show you why it might be a bad idea.
/**
* Does something related to a post.
*
* @param int|WP_Post|null $post_id A WordPress post ID or WP_Post Object, defaults to the current post.
* @return string
*/
function my_get_something_post_related( ?int|WP_Post $post_id ): string {
if ( $post_id instance_of WP_Post ) {
$post_id = $post_id->ID;
} elseif ( $post_id === null ) {
$post_id = get_the_ID();
}
// some code
return $some_string;
}WordPress is full of functions like it. While it is convenient and super easy to use it’s still bad. Just a few years ago I was all onboard the “yeah, WordPress does it, so will I” train.
Why is it bad?
Well, there are two main reasons why it’s a bad idea. If you’re just in the WordPress ecosystem this is common practice and you will thing it’s good practice, but my former colleague Joshua showed me an outside perspective to it.
Testability / Code complexity
The complexity to test code like this increases, you need to mock get_the_ID() and always keep this in mind. But you might throw in “Hey Christoph, I don’t write unit tests”, and that’s OK, I don’t write unit tests for every line of code either, but I try to write new code with a mindset that allows to add unit tests without too much trouble. That mindset will reduce complexity in functions which, will make errors easier to catch, even without Unit Tests.
In the example, you should consider to cover at least the following cases in a Unit test
nullpassed withget_the_ID()returning a post_idnullpassed withget_the_ID()returning no post_idWP_Postpassed with the desired post type (maybe a post, but no attachment)WP_Postpassed with a wrong post typeintpassed as valid post_idintpassed as some negative integer or zerointpassed as any positive integer, but there is no valid post for that ID.
If I change the code to this, it does get better.
/**
* Does something related to a post.
*
* @param int|WP_Post $post_id A WordPress post ID or WP_Post Object.
* @return string
*/
function my_get_something_post_related( int|WP_Post $post_id ): string {
if ( $post_id instance_of WP_Post ) {
$post_id = $post_id->ID;
}
// some code
return $some_string;
}WP_Postpassed with the desired post type (maybe a post, but no attachment)WP_Postpassed with a wrong post typeintpassed as valid post_idintpassed as some negative integer or zerointpassed as any positive integer that is not a post_id
Or we change it to this?
/**
* Does something related to a post.
*
* @param int $post_id A WordPress post ID.
* @return string
*/
function my_get_something_post_related( int $post_id ): string {
// some code
return $some_string;
}intpassed as valid post_idintpassed as some negative integer or zerointpassed as any positive integer that is not a post_id
And the best approach would be this.
/**
* Does something related to a post.
*
* @param WP_Post $post A WordPress Post Object.
* @return string
*/
function my_get_something_post_related( WP_Post $post ): string {
// some code
return $some_string;
}WP_Postpassed with the desired post type (maybe a post, but no attachment)WP_Postpassed with a wrong post type
As you see, the more precise your parameter becomes, the easier your code becomes and the more robust it will become.
Oh, and did I mention, since you will likely have 5, 10 or more functions, you reduce the complexity for all of them that.
Robustness / Predictability
If the unit tests did not yet convince you, I have more. Lets create an example, I admit this is constructed and bad code, but it will prove the point.
while ( have_posts() ) {
the_post();
my_magic_function_1(); // This will work as intended
// We want to query some related posts here.
$args = []; // Some args for the sub query.
$sub_query = new WP_Query( $args );
while ( $sub_query->have_posts() ) {
$sub_query->the_post();
// do something with this
}
// At this point you need to call wp_reset_postdata(), otherwise the context will be wrong.
// This will reference the last post from the sub query, since I forgot to reset by design.
my_magic_function_2();
}I know that the code example is broken, but the point is to show the downside of the automatic approach. And if the example is less obvious, finding the bug and debugging this, is very cumbersome. I have been in this situation, and it took way too long to debug this.
By relying less on these magic/guessing functions, the code becomes more robust, even if you do an error.
while ( have_posts() ) {
the_post();
$current_post = get_post(); // the only place where we will use "magic"
my_magic_function_1( $current_post );
// We want to query some related posts here.
$args = []; // Some args for the sub query.
$sub_query = new WP_Query( $args );
while ( $sub_query->have_posts() ) {
$sub_query->the_post();
// do something with this
}
// At this point you need to call wp_reset_postdata(), otherwise the context will be wrong.
my_magic_function_2( $current_post );
}The code example is still broken, and should not pass through a code review, but at least you won’t have any side effects and your frontend will behave like it should.
You will still be able to use this the wrong way, but it’s way easier to catch errors, either since PHP will throw them or it’s easier to catch them in code reviews.
Conclusion
I agree that you still won’t write Unit Tests right away, but adopting this will improve the code quality and will make your code less cluttered. True, you’ll need to add a very little extra thought when using the frontend. But it’s worth it and as good practice you could start using WordPress core functions alike as well get_the_title( $current_post ) will improve the intention and will actually have a minor positive impact on performance.
What are your thought on this?