Add a sniff for suppress_filters in get_posts

[tomjn]

Consider these 2 queries snippets:

$args = [
    'post_type' => 'page'
];

$results = get_posts( $args ); // 1
$q = new WP_Query( $args ); // 2

2 is faster, despite 1 using 2 internally. The reason for this is a difference in default arguments, specifically suppress_filters. In WP_Query it is false by default, but in get_posts it is true.

In this example, the performance difference is resolved:

$args = [
    'post_type' => 'page',
    'suppress_filters' => false
];

$results = get_posts( $args ); // 1
$q = new WP_Query( $args ); // 2

With suppress_filters set to true, any caching mechanisms are unable to do their job. Setting it to false fixes this. It's particularly noticeable on sites with any kind of object cache or effective caching plugins.

We flag this at VIP, and as a result a PR for a new VIP sniff was submitted in #1041 (comment) but this is not VIP specific, and falls into the cached vs uncached core APIs bucket

[jrfnl]

So a sniff for this should flag any equivalent of suppress_filters = true whether it is passed to WP_Query or to get_posts() and also get_posts() when called without suppress_filters set.

However, if a plugin very deliberately sets suppress_filters = true when calling either, I believe this should be considered a conscious choice of the developer and should probably not be flagged.

So that just leaves sniffing for get_posts() when called without suppress_filters set.

This can be done when the $args are set within the same scope (function) as the call to get_posts(), but as soon as the $args are passed as a function parameter or taken from a class property, sniffing for this will be highly inaccurate.

This would mean that if its "undetermined", the sniff should not flag the code.

function plugin_get_posts( $args ) {
    return get_posts( $args ); // OK.
}

class plugin_class {
    private $args = array(); // Set via constructor or other method.

    public function get_posts() {
        return get_posts( $this->args ); // OK.
    }
}

And what about code like this:

function plugin_get_posts() {
    $args = [
        'post_type' => 'page',
        'suppress_filters' => false
    ];
    $args = apply_filter( 'plugin_filter_hook', $args );

    return get_posts( $args ); // ???
}

Or along those same lines:

function plugin_get_posts() {
    $args = [
        'post_type' => 'page',
    ];
    $args = apply_filter( 'plugin_filter_hook', $args );

    return get_posts( $args ); // ???
}

It's particularly noticeable on sites with any kind of object cache or effective caching plugins.

In other words, this is only valid in a limited number of all cases as by far not all sites are running with caching (plugins or otherwise).

When there is no caching plugin active, calling WP_Query/get_posts() with suppress_filters = true should be faster, though the results may be different.

All in all, for this to be added to WPCS, I would like to see some more discussion about this.

[jrfnl]

P.S.: @tomjn Thanks for opening the issue 😉

[dingo-d]

The problem with sniffing this, as I see it, is that arguments can be passed to the get_posts() function in various ways that can be nearly impossible to sniff correctly (in addition to what Juliette mentioned already).

1. Passing it as a static method from a class (missing context to know what the argument contains, similar to what the apply_filter call would do)
2. Passing a global argument array (please never do this 😅)

We could detect only if we have some array in the file where the get_posts function is, containing the 'suppress_filters' => true item. And even that could get complex if we take class context into it.

Just flagging every get_posts as a warning could cause a bit of noise.

[jrfnl]

Good points from @dingo-d

I'm also just now wondering if the recent changes to WP_Query in WP 6.1 have any impact on this and if so, whether this issue is still relevant.

As I haven't done a deep dive into the WP 6.1 changes, I'm pinging @spacedmonkey and @felixarntz for an opinion.

[spacedmonkey]

WP_Query is now cached by default. So this sniff is much less important now.

[dingo-d]

@spacedmonkey Does this mean that the supress_filters in the get_posts function is no longer relevant?

[spacedmonkey]

@spacedmonkey Does this mean that the supress_filters in the get_posts function is no longer relevant?

Not sure what you mean by relevant. There is no longer a need to pass supress_filters => false, to ensure that caching plugins remain working. Core does this out of the box now. The sniff should be for cache_results => false.

[tomjn]

@spacedmonkey is cache_results set to false by default for get_posts or is it only if the user decides to use it?

[spacedmonkey]

@spacedmonkey is cache_results set to false by default for get_posts or is it only if the user decides to use it?

Set to true by default.

[jrfnl]

Given the above, I get the impression a sniff for this is no longer relevant. Is that correct ? If so, I suggest we close the issue.

If a sniff would still be relevant, it needs to be hashed out in detail what the conditions should be for the sniff to flag something and I would like to see that input in this issue.

[tomjn]

I concur with closing this

…

On Fri, 9 Dec 2022 at 15:05, Juliette ***@***.***> wrote: Given the above, I get the impression a sniff for this is no longer relevant. Is that correct ? If so, I suggest we close the issue. If a sniff would still be relevant, it needs to be hashed out in detail what the conditions should be for the sniff to flag something and I would like to see that input in this issue. — Reply to this email directly, view it on GitHub <#1416 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAOLZYTYRJGZOXPLXENGQLWMNDELANCNFSM4FI2FBVA> . You are receiving this because you were mentioned.Message ID: ***@***.***>