WordPress.org

Make WordPress Core

Opened 9 months ago

Last modified 6 weeks ago

#44723 assigned defect (bug)

The user ID in a `WP_User_Request` is not an integer as stated

Reported by: desrosj Owned by: desrosj
Milestone: Future Release Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-patch has-unit-tests needs-refresh
Focuses: Cc:

Description

Discovered this while debugging something in #43985. The value of WP_User_Request->user_id is a string, not an integer as stated by the inline documentation. This was causing some unexpected behavior in get_user_locale(), which performs a strict comparison for 0 and was causing the condition to incorrectly evaluate as false.

Attachments (3)

44723.diff (565 bytes) - added by desrosj 9 months ago.
44723-unittests.diff (830 bytes) - added by andizer 7 months ago.
Unit test
44723-refresh.diff (565 bytes) - added by andizer 6 weeks ago.

Download all attachments as: .zip

Change History (17)

@desrosj
9 months ago

#1 @desrosj
9 months ago

  • Keywords needs-unit-tests added

44723.diff type casts the property as an integer. But, this could be a potentially breaking change. It may be better to change the documentation to indicate the value is a string.

#2 follow-up: @garrett-eclipse
9 months ago

Interesting enough but if you go down the rabbit hole although the codex states WP_Post's post_author is 'A numeric string, for compatibility reasons' it's default is the numeric 0 and in wp_insert_post it's expecting an (int) but if not provided uses the return of get_current_user_id which is an int.

$post_author in WP_Post - https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/class-wp-post.php#L32
wp_insert_post Codex - https://developer.wordpress.org/reference/functions/wp_insert_post/
$user_id in wp_insert_post - https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/post.php#L3139
get_current_user_id Codex - https://developer.wordpress.org/reference/functions/get_current_user_id/

So it's weird it seems it's always an int down the chain so not sure why it's saying it's a numeric string.

#3 in reply to: ↑ 2 @SergeyBiryukov
9 months ago

Replying to garrett-eclipse:

So it's weird it seems it's always an int down the chain so not sure why it's saying it's a numeric string.

See the discussion in #22324 and #25092.

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


9 months ago

#5 @garrett-eclipse
9 months ago

Thanks @SergeyBiryukov I appreciate the references, I think that should equip @desrosj to continue on here. Cheers

#6 @javorszky
9 months ago

I think this is related here: #43752

Last edited 8 months ago by SergeyBiryukov (previous) (diff)

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


8 months ago

#8 @TZ Media
8 months ago

Taking the other tickets mentioned into account, I believe changing the documentations is more in alighment with how such issues were handled in the past. And as long as WP does not use PHP7 strict type hinting, return value types can't be trusted anyway and must be checked/sanitized.

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


7 months ago

@andizer
7 months ago

Unit test

#10 @andizer
7 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#11 @pento
7 months ago

  • Milestone changed from 4.9.9 to Future Release

This ticket was mentioned in Slack in #core-privacy by webdevlaw. View the logs.


7 months ago

#13 @garrett-eclipse
3 months ago

  • Keywords needs-refresh added
  • Owner set to desrosj
  • Status changed from new to assigned

#14 @andizer
6 weeks ago

I've added a new patch with the same fix. I suppose this is what meant with a refresh.

Note: See TracTickets for help on using tickets.