close

Make WordPress Core

Opened 4 weeks ago

Closed 3 weeks ago

#64813 closed defect (bug) (fixed)

Docs: Fix `@param` type for `get_posts()` and other functions using `wp_parse_args()`

Reported by: rodrigosprimo's profile rodrigosprimo Owned by: westonruter's profile westonruter
Milestone: 7.0 Priority: normal
Severity: normal Version:
Component: Posts, Post Types Keywords: has-patch needs-unit-tests commit
Focuses: docs Cc:

Description

While working on a WPCS PR, I noticed that the get_posts() function documents its $args parameter as @param array $args, but since $args is passed through wp_parse_args(), it also accepts a query string (e.g., 'numberposts=5&post_type=page'). The @param type should be string|array to reflect this.

Other functions that also pass their $args through wp_parse_args() already correctly document string|array:

Other discussion points

More functions with array type

Besides get_posts(), there is a significant number of other functions that document just array for parameters passed to wp_parse_args(). Here is a list of the ones I found on a quick search (there may be more):

Should those be addressed as well?

Should object be included?

wp_parse_args() itself is documented as @param string|array|object $args. None of the functions that pass $args through wp_parse_args() that I checked include object in their @param type. They all use string|array or just array. I wonder if object should also be documented for these functions for completeness, or if there is a reason why object is not included?

Change History (14)

Image

This ticket was mentioned in PR #11179 on WordPress/wordpress-develop by @rodrigosprimo.


4 weeks ago
#1

  • Keywords has-patch added

The $args parameter is passed through wp_parse_args(), which accepts both arrays and query strings. This updates the @param type from array to array|string to reflect this, consistent with other functions like wp_list_categories() and get_terms().

#2 Image @rodrigosprimo
4 weeks ago

I created a PR fixing just get_posts() to use array|string instead of just array. Happy to update the other functions if needed once a decision is made regarding the questions that I left in the ticket description.

#3 Image @westonruter
4 weeks ago

  • Milestone changed from Awaiting Review to 7.0
  • Owner set to westonruter
  • Status changed from new to accepted

#4 in reply to: ↑ description Image @westonruter
4 weeks ago

  • Component changed from General to Posts, Post Types
  • Keywords commit added

Replying to rodrigosprimo:

Should those be addressed as well?
...

Should object be included?

Just seeing this part of the ticket after reviewing the PR.

Yes, get_posts() works when an object is passed thanks to wp_parse_args(). However, I'm not sure we want to necessarily encourage that. Just because you can pass something doesn't mean you should. The same could be said for string.

But I do see quite a few plugins passing a string to get_posts(): https://wpdirectory.net/search/01KJZMCFA2DP2B22MGFCQMZ5DN

So that seems clearly like it should be documented as being supported.

As for the other functions listed, it seems strings are extremely rarely passed: https://wpdirectory.net/search/01KJZMG5EH7HXX70XR28CV3H53

The only example (barely) is wp_get_recent_posts().

So I think sticking just with adding string as an allowed type for the param for get_posts() is good for now.

#5 Image @mukesh27
4 weeks ago

Do we need follow-up ticket for other function that need the update?

#6 Image @westonruter
4 weeks ago

I don't think it's necessary given what I found above. Just because you can pass something doesn't mean you necessarily should.

#7 follow-up: Image @rodrigosprimo
4 weeks ago

Thanks for the review and for running the wpdirectory searches, @westonruter. I'm happy to keep this ticket limited to adding string for get_posts().

Just a thought on the broader question: my understanding is that @param types should describe what a function actually accepts, regardless of whether we want to encourage a particular usage pattern. But I can see the pragmatic argument for not documenting object here since it's not used in practice. Is there an established practice around this in Core? I wonder if an alternative approach could be to document the three accepted types, array|object|string, but note in the parameter description that passing an object is discouraged or something like that?

#8 in reply to: ↑ 7 Image @westonruter
3 weeks ago

  • Keywords changes-requested needs-unit-tests added; commit removed

@rodrigosprimo I guess the I'm thinking of being more conservative. Once we document something as being supported, then we're more-or-less locked in. I'd rather document what should be passed in or what is expected to be passed in, rather than everything that could be passed in. Using usage data from WPdirectory Veloria I think would help inform from actual usage whether we should document something as being officially supported. It's a sort of “pave the cowpaths” approach.

Also: We should have tests in place for anything we document as being supported. In Tests_Post_GetPosts I don't see any tests present for passing a query string. So we should be sure to add a test for that as part of this, and any functions we update to indicate an array or query string is supported as the argument.

#9 Image @westonruter
3 weeks ago

  • Owner changed from westonruter to rodrigosprimo
  • Status changed from accepted to assigned

Image

@westonruter commented on PR #11179:


3 weeks ago
#10

@rodrigoprimo will you add a test as well for passing a string to get_posts()? I can add one otherwise.

Image

@rodrigosprimo commented on PR #11179:


3 weeks ago
#11

@westonruter, I added a commit adding a simple test showing a single case of passing a string to get_posts(). Is this more or less what you had in mind?

#12 Image @rodrigosprimo
3 weeks ago

Thanks for the explanation, @westonruter. I understand the reasoning behind the conservative approach, and I'm happy to proceed that way.

I'll note that I believe that @param types should precisely describe what the code actually accepts, since a mismatch between what the docs say and what the code does can undermine trust in the documentation. That's actually what prompted me to create this ticket when at first I thought get_posts() only accepted an array and then realized it also accepts a string (and an object). And if we don't want to encourage a given behavior, like passing an object to get_posts(), I think the ideal approach would be to explicitly deprecate it rather than leave it undocumented. But I understand that's more time-consuming and probably not a priority here.

In any case, I'm on board with keeping this limited to array|string for get_posts().

I pushed a commit to the PR adding a test for passing a query string to get_posts().

#13 Image @westonruter
3 weeks ago

  • Keywords commit added; changes-requested removed
  • Owner changed from rodrigosprimo to westonruter
  • Status changed from assigned to accepted

#14 Image @westonruter
3 weeks ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 62041:

Docs: Indicate that get_posts() can take a query string in addition to an array of query vars.

A string is supported by virtue of wp_parse_args() being used on the supplied arguments. While a string is not currently passed to get_posts() in core, a significant number of plugins are doing so. So in addition to documenting actual ecosystem usage, this also adds a test to ensure that supplying a query string continues to be supported in the future.

Developed in https://github.com/WordPress/wordpress-develop/pull/11179

Follow-up to r11528.

Props rodrigosprimo, westonruter, shailu25, mukesh27.
See #64224, #10047.
Fixes #64813.

Note: See TracTickets for help on using tickets.