WordPress.org

Make WordPress Core

Opened 2 months ago

Last modified 2 months ago

#48027 new defect (bug)

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

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

Description

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

Attachments (2)

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

Download all attachments as: .zip

Change History (12)

#1 @dkarfa
2 months ago

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

Reviewing.

#2 @SergeyBiryukov
2 months ago

  • Component changed from General to Comments

#3 @donmhico
2 months 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.

@donmhico
2 months ago

#4 @donmhico
2 months 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.


2 months ago

#6 @SergeyBiryukov
2 months 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.

Also related: #44723.

Version 0, edited 2 months ago by SergeyBiryukov (next)

#7 @donmhico
2 months 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. 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.

#8 @cneumann
2 months 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
2 months 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.

@donmhico
2 months ago

Update docs.

#10 @donmhico
2 months 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.