Flag 'exclude' args in get_posts()

[rebeccahum]

What problem would the enhancement address for VIP?

Since WordPressVIPMinimum.Performance.WPQueryParams.PostNotIn checks for post__not_in, we should also check for the 'exclude' parameter being passed into get_posts() which in turn calls post__not_in.

Describe the solution you'd like

What code should be reported as a violation?

get_posts( [ 'exclude' => $post_ids ] ); // Warning.

What could should not be reported as a violation?

$exclude = [ 1, 2, 3 ];

Additional context

Codex: https://developer.wordpress.org/reference/functions/get_posts/

[jrfnl]

I'm looking at this sniff now and am wondering if there was a particular reason why this sniff wasn't based on the AbstractArrayAssignmentRestrictionsSniff which is specifically set up for this kind of thing ?

I'd happy to switch it over. The only downside of that would be that the error codes would change as the abstract doesn't allow for custom error codes, which would be a BC-break.

The error code change would be along the lines of:

• WPQueryParams.PostNotIn -> WPQueryParams.PostNotIn_post__not_in
• WPQueryParams.SuppressFiltersTrue -> WPQueryParams.SuppressFilters_suppress_filters

Alternatively, I could implement the use of the abstract anyway, but only for the new array key detection.

For the existing two checks, the switch-over could be targetted for the VIPCS 3.0.0 release.

[jrfnl]

Also note that exclude is quite a generic array key name, so I suspect adding this key as one to be detected by this sniff will yield lots of false positives for exclude being used in other contexts than get_posts().

At the same time, hard linking the key detection to get_posts() is also not a realistic option as these kind of parameters are often set up outside of the function call / in class properties / class constants etc.

[rebeccahum]

I'm looking at this sniff now and am wondering if there was a particular reason why this sniff wasn't based on the AbstractArrayAssignmentRestrictionsSniff which is specifically set up for this kind of thing ?

@jrfnl I don't think that was a consideration at the time: 5d86203#diff-8568a7057376eaec476ec111d5469c9d

Also note that exclude is quite a generic array key name, so I suspect adding this key as one to be detected by this sniff will yield lots of false positives for exclude being used in other contexts than get_posts().

Agreed, but I do still think it's something worth flagging.

[jrfnl]

Well, the abstract sniff was already available at the time, so that doesn't really answer my first question.

Either way - what about my suggestion for switching it over ? Either completely or partially to prevent the BC-break and finishing the switch over off in 3.0 ?

[jrfnl]

@rebeccahum @GaryJones Any opinions on my suggestion ?

[rebeccahum]

@jrfnl Yes, this sounds great, I will work on getting it switched out!

[jrfnl]

@rebeccahum I actually got the changes for this mostly ready already. I just need a decision on how we want to handle the error code BC-break before I can finish it off and pull it.

So do we mind a BC-break for those two error code in a minor or not ?
If we find this a small enough break to be acceptable in a minor, I can do the complete switch over now.
Alternatively, I implement the switch over for the new check and we open a new issue to switch over the existing checks in the next major.

[rebeccahum]

@jrfnl Oh awesome! I personally think the BC-break for those two error codes are minor enough to warrant the change. @GaryJones thoughts?

[GaryJones]

We've been caught out recently with a BC break of error codes going into a minor, so I'd much prefer having it partially addressed now, and doing the BC-break in the major please.