Type declarations should not be used on filter callbacks

[joshlevinson]

By nature, arguments passed around in filters are unpredictable. While core may indicate that a developer can expect their callback to be passed an argument of a given type, any other implementor of that filter could have changed the value being passed around.

I suggest that it's best to not rely on PHP's type declaration feature in the definition of a filter/action callback, and to:

• Remove the type declaration from the callback's arguments and check for the expected type within the callback body

or–if it's undesirable to change the declaration of the callback–

• Define an intermediary function that checks the type before calling the callback, i.e.

<?php
add_filter( 'hook', function( $thing ) {
  if( is_object( $thing ) && $a instanceof \Thing ) {
    callback( $thing );
  }
}, 10, 1 );

function callback( \Thing $thing ) {}

I'm happy to open up a PR for this; I presume Design Patterns might make a good home?

[nicoladj77]

I think that this depends a lot on what you are developing. I agree with you that Type Hinting should not be used when you are developing a plugin which will be Open sourced and will be used by everyone. In this case Type Hinting is bad, because it would bring down a site if another plugin breaks the filter contract ( with contract I mean the signature, the type of the parameters passed to the filter ) . Usually it's better to just go along, I've been there and done it, it's not good. I think that the situation is different if you are developing a Theme for a single customer. In that case Type Hinting might be used as an early failure system, to detect early if a third party plugin which breaks the filter contract which might otherwise pass unnoticed. Usually we control the whole environment and such a thing would be caught early on in the development cycle.

[joshlevinson]

@nicoladj77 Thanks for your feedback! I definitely see where you're coming from. The specific case where I encountered this is where Yoast SEO was calling get_permalink over a stdClass object that was built to model a WP_Post (the object was the result of a direct $wpdb query). The callback in our theme strictly declared that the $post argument should be a WP_Post (as core indicates it would be). Since it wasn't, things broke.

Regardless of the distribution–limited or open, the issue would still have to be addressed. I couldn't fix Yoast's code, so I had to remove the strict declaration on the callback, and just call get_post( $post ) over the argument to let WP give me back a guaranteed WP_Post. If I really wanted to keep the strict typing, I could have gone the other route I mentioned (not attaching the callback directly to the filter hook), so that's why I made that suggestion.

[ericmann]

I'm all for defensive coding, but this is a lot of additional boilerplate to add and, as a design pattern/standard it will make the code quite a bit less readable. I'd err on the side of trusting documentation by default and using strict typing, then making case-by-case exceptions for bad members of the community (in this case, Yoast) who violate the API spec.

Because that's what we're talking about here - a spec. The docs applied to filters and actions might be regular docblocks, but they define a specific API. If an integration fails to actually implement or follow that API so be it - we can code around that and protect ourselves. But as a common practice, we should use APIs as they're written, because assuming they're always invalid is going to slow down a lot of developers, make code less readable, and potentially add an unnecessary layer of abstraction atop the API itself.

[dana-ross]

@ericmann I see where you're coming from, but I think we need to take the defensive approach when dealing with WordPress plugins and the Core codebase. WordPress itself even does that.

wp-includes/query.php does exactly what @joshlevinson describes, using get_post to turn a WP_Post-like object into a bonafide WP_Post:

         * @param WP_Post|object|int $post WP_Post instance or Post ID/object.
         * @return true True when finished.
         */
        public function setup_postdata( $post ) {
                global $id, $authordata, $currentday, $currentmonth, $page, $pages, $multipage, $more, $numpages;

                if ( ! ( $post instanceof WP_Post ) ) {
                        $post = get_post( $post );
                }

And it probably does that because WordPress Core set a precedent; it couldn't even be trusted to return a proper WP_Post as recently as 2011. wp-admin/includes/post.php contained code like this, where get_default_post_to_edit returned a WP_Post-like object:

} else {
        $post = new stdClass;
        $post->ID = 0;
        $post->post_author = '';
        $post->post_date = '';
        $post->post_date_gmt = '';
        $post->post_password = '';
        $post->post_type = $post_type;
        $post->post_status = 'draft';
        $post->to_ping = '';
        $post->pinged = '';
        $post->comment_status = get_option( 'default_comment_status' );
        $post->ping_status = get_option( 'default_ping_status' );
        $post->post_pingback = get_option( 'default_pingback_flag' );
        $post->post_category = get_option( 'default_category' );
        $post->page_template = 'default';
        $post->post_parent = 0;
        $post->menu_order = 0;
    }
    $post->post_content = apply_filters( 'default_content', $post_content, $post );
    $post->post_title   = apply_filters( 'default_title',   $post_title, $post   );
    $post->post_excerpt = apply_filters( 'default_excerpt', $post_excerpt, $post );
    $post->post_name = '';
    return $post;

(Someone added $post = new WP_Post( $post ); before the return since then)

My preference here would be to use type declarations for code that only needs to interface with other code we write. Anyplace we're dealing with hooks or otherwise interact with 3rd party code through a hook/API, we'd use a wrapper to coerce the arguments into what we really want. That way, our code stays "correct" and easier to test, and the coercion is isolated from the rest of our logic.

add_filter('the_post', 'filter_the_post');
function filter_the_post($post) {
    $post = new WP_Post($post);
    return _filter_the_post($post);
}

function _filter_the_post(WP_Post $post) {
    // do stuff
    return $post;
}

Is it extra work, maybe even boilerplate after a while? Sure. But I don't think the WordPress ecosystem gives us much choice.

Or, we forgo using type declarations completely and give up the benefits they bring us.

[tlovett1]

I really agree with @ericmann on this one. Defensive coding is great especially when dealing with third party APIs. However, it's really easy to take it too far. At some point documentation just needs to be trusted. I do think defensively checking parameters makes sense in certain situations (i.e. passing a post/WP_Query object). Not sure there is much to add to the Best Practices here.