Additional audit for recent documentation changes

[SergeyBiryukov]

Some of the documentation changes in #30224 need an additional audit for consistency:

• [30673] added null as a possible @return value for set_user_setting() and delete_user_setting(), but it's not explained in the comment when it can be returned.
• [30674] removed int as a possible @return value for get_the_time(), get_post_time(), and get_post_modified_time(), but they all depend on mysql2date() and can return an integer if 'U' or 'G' is passed. [30682] added int back, but only for get_post_time().
• [30678] added string as a possible @return value for WP_Filesystem_SSH2::chown(), but it's not explained in the comment when it can be returned.
• There are probably more, these are just the ones that stood out to me.

[SergeyBiryukov]

If a function returns a string on success and false on failure, does it make a difference if string comes first in the @return value? I thought string|bool is preferred, but was confused by some of the changes in [30674].

Looks like some of the recent changes attempted to clarify bool and changed it to false.

Should we standardize on string|false in that case?

[DrewAPicture]

I'm working on this.

[DrewAPicture]

Replying to SergeyBiryukov:

If a function returns a string on success and false on failure, does it make a difference if string comes first in the @return value? I thought string|bool is preferred, but was confused by some of the changes in [30674].

The first type in the list should always be the expected type, followed by any outliers. Beyond a certain number (usually 3 or 4) we just mixed then explain all possibles in the description.

So a common pattern in core is something like * @return int|WP_Error Description.. int is the expected type, WP_Error is the outlier.

Looks like some of the recent changes attempted to clarify bool and changed it to false.

Should we standardize on string|false in that case?

I think so, yes, because I assume string is the expected type.

[DrewAPicture]

In 34493:

Docs: Add a summary, version, and parameter and return descriptions to the DocBlock for WP_Filesystem_FTPext::get_contents().

Also reverses the return types as string is expected, false is the outlier.

See [30978].

See #30989. See #32246.

[DrewAPicture]

In 34495:

Docs: Add a summary, version, and parameter and return descriptions to the DocBlock for WP_Filesystem_ftpsockets::get_contents().

Also reverses the return types as string is expected, false is the outlier.

See [30978].

See #30989. See #32246.

[DrewAPicture]

In 34497:

Docs: Mark the optional $upgrader parameter as such and add a description in the DocBlock for Language_Pack_Upgrader::async_upgrade().

See [32655].

See #30989. See #32246.

[DrewAPicture]

In 37263:

Docs: Clarify parameter and return descriptions in the DocBlock for wp_set_all_user_settings().

See [32613]. See #30989.

[DrewAPicture]

In 37264:

Docs: Clarify return descriptions in the DocBlocks for set_user_setting() and delete_user_setting().

See [32613]. See #30989.

[DrewAPicture]

In 37265:

Docs: Clarify the return descriptions for get_the_time(), get_post_time(), and get_post_modified_time() to specify when an integer in the form of a Unix timestamp should be expected.

See [30674]. See #30989.

[DrewAPicture]

ssh2 chown() and run_command()

[DrewAPicture]

Have asked @dd32 if he could please give feedback on 30989.diff​ which attempts return descriptions for WP_Filesystem_SSH2::chown() and WP_Filesystem_SSH2::run_command().

[dd32]

Replying to DrewAPicture:

Have asked @dd32 if he could please give feedback on 30989.diff​ which attempts return descriptions for WP_Filesystem_SSH2::chown() and WP_Filesystem_SSH2::run_command().

Seems sane. I'm not sure when chown() returns a a string though, I doubt it'd return anything on a success, it'd more likely be a failure.. but I can't test it.

[DrewAPicture]

Replying to dd32:

Replying to DrewAPicture:

Have asked @dd32 if he could please give feedback on 30989.diff​ which attempts return descriptions for WP_Filesystem_SSH2::chown() and WP_Filesystem_SSH2::run_command().

Seems sane. I'm not sure when chown() returns a a string though, I doubt it'd return anything on a success, it'd more likely be a failure.. but I can't test it.

You're correct. In re-reading the source for ->chown(), the second parameter, $returnbool is always specified as true. Thus, ->run_command() can only return a boolean. On the other hand, ->run_command() could return a string in the case where $returnbool was false and the stream_get_contents() call returned something (was successful).

I'll fix it on commit.

[DrewAPicture]

In 37270:

Docs: Add missing return descriptions for WP_Filesystem_SSH2::chown() and WP_Filesystem_SSH2::run_command().

Fixes #30989.