WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 3 years ago

#30989 closed task (blessed) (fixed)

Additional audit for recent documentation changes

Reported by: SergeyBiryukov Owned by: DrewAPicture
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.1
Component: General Keywords: dev-feedback
Focuses: docs Cc:
PR Number:

Description (last modified by 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.

Attachments (1)

30989.diff (1.2 KB) - added by DrewAPicture 3 years ago.
ssh2 chown() and run_command()

Download all attachments as: .zip

Change History (19)

#1 follow-up: @SergeyBiryukov
5 years ago

  • Keywords dev-feedback added

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?

#2 @SergeyBiryukov
5 years ago

  • Description modified (diff)

This ticket was mentioned in Slack in #core by drew. View the logs.


5 years ago

#4 @DrewAPicture
4 years ago

  • Milestone changed from Awaiting Review to Future Release

#5 @DrewAPicture
4 years ago

  • Milestone changed from Future Release to 4.4
  • Owner set to DrewAPicture
  • Status changed from new to accepted
  • Type changed from defect (bug) to task (blessed)

I'm working on this.

#6 in reply to: ↑ 1 @DrewAPicture
4 years ago

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.

#7 @DrewAPicture
4 years ago

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.

#8 @DrewAPicture
4 years ago

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.

#9 @DrewAPicture
4 years ago

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.

#10 @DrewAPicture
4 years ago

  • Milestone changed from 4.4 to Future Release

#11 @DrewAPicture
3 years ago

In 37263:

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

See [32613]. See #30989.

#12 @DrewAPicture
3 years ago

In 37264:

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

See [32613]. See #30989.

#13 @DrewAPicture
3 years ago

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
3 years ago

ssh2 chown() and run_command()

#14 follow-up: @DrewAPicture
3 years ago

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().

#15 @DrewAPicture
3 years ago

  • Milestone changed from Future Release to 4.6

#16 in reply to: ↑ 14 ; follow-up: @dd32
3 years ago

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.

#17 in reply to: ↑ 16 @DrewAPicture
3 years ago

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.

#18 @DrewAPicture
3 years ago

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

In 37270:

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

Fixes #30989.

Note: See TracTickets for help on using tickets.