Make WordPress Core

Opened 6 years ago

Last modified 3 years ago

#44723 assigned defect (bug)

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

Reported by: desrosj's profile desrosj Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: needs-patch
Focuses: docs 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 6 years ago.
44723-unittests.diff (830 bytes) - added by andizer 6 years ago.
Unit test
44723-refresh.diff (565 bytes) - added by andizer 6 years ago.

Download all attachments as: .zip

Change History (23)

@desrosj
6 years ago

#1 @desrosj
6 years 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
6 years 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
6 years 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.


6 years ago

#5 @garrett-eclipse
6 years ago

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

#6 @javorszky
6 years ago

I think this is related here: #43752

Last edited 6 years ago by SergeyBiryukov (previous) (diff)

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


6 years ago

#8 follow-up: @TZ Media
6 years 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.


6 years ago

@andizer
6 years ago

Unit test

#10 @andizer
6 years ago

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

#11 @pento
6 years ago

  • Milestone changed from 4.9.9 to Future Release

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


6 years ago

#13 @garrett-eclipse
6 years ago

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

#14 @andizer
6 years ago

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

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


5 years ago

#16 @birgire
5 years ago

  • Milestone changed from Future Release to 5.3

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


5 years ago

#18 @desrosj
5 years ago

  • Milestone changed from 5.3 to Future Release
  • Owner desrosj deleted

I have not been able to dig into this during this release cycle. I'm also thinking that if this is changed, it should happen early just in case this creates any unforeseen problems.

Removing myself so someone else can dig into this if they'd like in the mean time.

#19 @desrosj
5 years ago

  • Keywords early added

#20 in reply to: ↑ 8 @johnbillion
3 years ago

  • Focuses docs added
  • Keywords needs-patch added; has-patch has-unit-tests needs-refresh early removed

Replying to TZ Media:

I believe changing the documentations is more in alignment with how such issues were handled in the past.

Agreed, let's fix the documentation and retain the string ID property.

Note: See TracTickets for help on using tickets.