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

[cneumann]

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

Example:

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

[dkarfa]

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

Reviewing.

[donmhico]

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

[donmhico]

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.

[SergeyBiryukov]

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.

[donmhico]

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. https://core.trac.wordpress.org/browser/tags/5.2/src/wp-includes/class-wp-comment.php#L133

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.

[cneumann]

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.

[johnbillion]

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.

[donmhico]

Update docs.

[donmhico]

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.

[SergeyBiryukov]

Thanks for the ticket! This appears to be resolved as part of [48941] / #50768.