Audit `get_comment()` response checks.

[costdev]

There are currently 164 calls to get_comment() across 36 files in the codebase (see attached file), with more pending with at least one upcoming PR.

Some of these calls check the response of get_comment() in one of the following ways:

<?php
$comment = get_comment( $comment_id );

if ( $comment ) {...
if ( ! $comment ) {...

$comment ?...
! $comment ? ...

if ( empty( $comment->comment_ID ) {...
if ( ! empty( $comment->comment_ID ) {...

Some do not check the response at all. A ​discussion on Slack between myself and @jrf led to the suggestion that we audit the use of get_comment().

@hellofromtonya suggested two alternative checks on the response:

<?php
if ( ! $comment instanceof WP_Comment ) {...

if ( null === $comment ) {...

[costdev]

@SergeyBiryukov I've heard many a tale passed down through the ages of your impeccable memory. Do you know when the last audit of get_comment() was done?

[jrf]

Replying to costdev:

@hellofromtonya suggested two alternative checks on the response:

<?php
if ( ! $comment instanceof WP_Comment ) {...

if ( null === $comment ) {...

When validating data and given the choice between checking what you want and checking what you don't want, it's best practice to always use the most specific check (which still doesn't break BC), which in this case means that (variations of) if ( $comment instanceof WP_Comment ) {} should be the preferred check.

[costdev]

Replying to jrf:

Replying to costdev:

@hellofromtonya suggested two alternative checks on the response:

<?php
if ( ! $comment instanceof WP_Comment ) {...

if ( null === $comment ) {...

When validating data and given the choice between checking what you want and checking what you don't want, it's best practice to always use the most specific check (which still doesn't break BC), which in this case means that (variations of) if ( $comment instanceof WP_Comment ) {} should be the preferred check.

Thanks!

As none of the calls to get_comment() make use of a direct check like if ( $comment instanceof WP_Comment ) {}, it looks like all of the above mentioned files will be changed.

Is there a preference about how to go about submitting changes for this?

• Should it be in one PR?
• Separate .diff/.patch for each file or call?
• Other?

[costdev]

The ​Docs state:

Return
(WP_Comment|array|null) Depends on $output value.

The default is OBJECT (WP_Comment).

Should an $output value of ARRAY_A or ARRAY_N be supplied (this currently only seems to exist once outside of the tests though...), I'll change:

<?php

    if ( $comment instanceof WP_Comment ) {}

to:

<?php

    if ( is_array( $comment ) && ! empty( $comment ) ) {}

    // Or would this be a better option to protect against
    // any current/future possible `array( false )` output?

    if ( is_array( $comment ) && ! empty( array_filter( $comment ) ) ) {}

Trac ticket: https://core.trac.wordpress.org/ticket/54149

[SergeyBiryukov]

Related: #40143

[dd32]

Replying to jrf:

When validating data and given the choice between checking what you want and checking what you don't want, it's best practice to always use the most specific check (which still doesn't break BC), which in this case means that (variations of) if ( $comment instanceof WP_Comment ) {} should be the preferred check.

Respectfully, I disagree that $comment instanceof WP_Comment should be the preferred check - especially in older code or anything that isn't using it in any overly strict manner.
When you're dealing with the response of get_comment() you're going to end up with several variations:

• false/null - falsy values
• WP_Comment / array - the usual expected value, truthy.
• Something else from a function filtering it. These break down to:
  • Code errors returning true, half-filled arrays or malformed arrays, or objects that are not at all like WP_Comments, these are code errors on the developers that shouldn't need to be cared about
  • Something Comment-like, I've seen plugins return "fake" comments that are just stdClass (from before WP_Comment existed) that for all intents and purposes are comment-like enough.

Based on a truth-table of that, you effectively are dealing with three variations:

• Truthy valid values = array / stdClass / WP_Comment
• Falsy values = null/false
• Invalid junk that a plugin has returned, that we shouldn't need care about, let it throw whatever PHP Notice/Warning/Error here and the developer of the plugin or code can fix it.

While I get it, that $comment instanceOf WP_Comment or is_object( $comment ) are the most "correct and strict" checks, if ( $comment ) is for all intents and purposes "enough" for checking if the comment is valid within WordPress code IMHO.

Replying to costdev:

The ​Docs state:

Return
(WP_Comment|array|null) Depends on $output value.

The default is OBJECT (WP_Comment).

Should an $output value of ARRAY_A or ARRAY_N be supplied

That just re-enforces this to me, having to do if ( $comment instanceOf WP_Comment || ( is_array( $comment ) && ! empty( $comment ) ) ) {} is ridiculous, when if you trust those who use the filters, if ( $comment ) {} is far more readable and "safe"

(edit: To be clear though, I agree that the above is an over-the-top.. if you request a object from get_comment() it should return an object, but there's no filtering in the function to ensure the object it returns is a WP_Comment or even an object after the filter has been run)

[costdev]

If we do decide to change the guard, at least it'll be easier to find/replace in the patch now that they're consistently set to (!) $comment instanceof WP_Comment. Hopefully we'll confirm the path forward soon and get this committed.

[hellofromTonya]

As discussions are ongoing and 5.9 Beta 1 is < 19 hours away, moving this ticket to 6.0.

[davidbaumwald]

Related: #54607.

[costdev]

This ticket was discussed in the bug scrub. As we don't yet have consensus, the patch involves quite a few changes, and we're into the late stages of the 6.0 cycle, I'm moving this ticket to the 6.1 milestone.

[hellofromTonya]

Work on this ticket has not progressed since 5.9. Currently, there's not consensus on the approach to increase data type checks. Moving this one to Future Release. Once there's consensus, then the ticket can move back into a milestone.

Also self-assigning ownership to help shepherd progress forward.

[hellofromTonya]

As of today, I'm no longer a sponsored contributor and am starting a month or more AFK break. To not stand in the way, I'm clearing me as the ticket owner, which opens the ticket for someone to step in to shepherd this ticket forward to resolution.