Make WordPress Core

Opened 5 years ago

Last modified 5 years ago

#48027 new defect (bug)

WP_Comment fields (e.g. user_id) have wrong types

Reported by: cneumann's profile cneumann Owned by:
Milestone: Awaiting Review Priority: normal
Severity: minor Version:
Component: Comments Keywords: has-patch
Focuses: docs Cc:


WP_Comment objects retrieved e.g. with get_comment have wrong types for some member variables, e.g. user_id.


class TestInsertBug extends \WP_UnitTestCase {
        public function test_insert() {
                $comment_id = wp_insert_comment( [
                        'user_id' => 1,
                ] );
                $comment = get_comment( $comment_id );
                // will fail because $comment->user_id is a string
                $this->assertSame( $comment->user_id, 1 );

The constructor of WP_Comment just applies the data to the variables, without setting/fixing the types (as e.g. WP_User does for the ID).

Attachments (2)

48027.1.diff (4.0 KB) - added by donmhico 5 years ago.
48027.2.diff (422 bytes) - added by donmhico 5 years ago.
Update docs.

Download all attachments as: .zip

Change History (12)

#1 @dkarfa
5 years ago

Hi @cneumann , welcome to WordPress Trac! Thanks for the report.


#2 @SergeyBiryukov
5 years ago

  • Component changed from General to Comments

#3 @donmhico
5 years ago

Thanks for the report @cneumann and welcome to WordPress Trac. I can indeed confirm the issue. I'll try to produce a patch soon.

5 years ago

#4 @donmhico
5 years ago

I've attached a new patch 48027.1.diff, that basically change the user_id property to int before the comment object is returned. Although i'm not if this is good enough. Its also possible to convert it to int before it is added to cache using wp_cache_add. Let me know your thoughts.

I've added some unit tests and as well as change some existing ones to reflect the change. It might be good to create a dev-note regarding this as existing themes / plugins might be using === before and those will break since the patch will change the user_id from string to int.

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

5 years ago

#6 @SergeyBiryukov
5 years ago

Just wanted to note that changing data type for existing objects is not always straightforward and may cause backward compatibility issues, see the discussion in #22324 and #25092 for example.

Also related: #44723.

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

#7 @donmhico
5 years ago

Thanks for the references @SergeyBiryukov. I'll look into them. Actually backward compatibility is one of my reservation regarding this as well. But I think it still best to address this issue one way or the other. From the DocBlock of WP_Comment it states user_id as int.

I suppose for the meantime, it's possible to change it to string with a description stating that it's a numerical value. Just to prevent confusion. But it's best if we can try to see if we can transform it to int.

For the meantime, i'll try to find possible areas affected with this change.

If anyone is available, please feel free to try this out as well. The more test cases, the better. Thanks guys.

#8 @cneumann
5 years ago

As the other similar problems were also fixed with just updating the docs, I guess this would be the way to go. One should also check the other variables of WP_Comments that are documented as being something else than string.

The updated doc would have helped a lot.

#9 @johnbillion
5 years ago

  • Focuses docs added
  • Keywords needs-patch added
  • Version 5.2.3 deleted

Agreed, as per the linked tickets changing the type of a core object property _will_ break third party code that uses strict type checks.

Ensuring all related docs are correct is a better approach.

5 years ago

Update docs.

#10 @donmhico
5 years ago

  • Keywords has-patch added; needs-patch removed

As others recommended and as well as the resolution for #25092 [25086], it seems that updating the docs would be the best course of action.

Note: See TracTickets for help on using tickets.