WordPress.org

Make WordPress Core

Opened 2 months ago

Last modified 13 days ago

#54149 new task (blessed)

Audit `get_comment()` response checks.

Reported by: costdev Owned by:
Milestone: 5.9 Priority: normal
Severity: normal Version: trunk
Component: Comments Keywords: has-patch
Focuses: coding-standards Cc:

Description

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

Attachments (1)

get_comment_.code-search (20.4 KB) - added by costdev 2 months ago.

Download all attachments as: .zip

Change History (11)

#1 @costdev
2 months ago

  • Type changed from defect (bug) to enhancement

#2 @costdev
2 months ago

@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?

#3 @jrf
2 months ago

  • Type changed from enhancement to task (blessed)

#4 in reply to: ↑ description ; follow-ups: @jrf
2 months ago

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.

#5 in reply to: ↑ 4 @costdev
2 months ago

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?

#6 @costdev
2 months ago

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 ) ) ) {}

Last edited 2 months ago by costdev (previous) (diff)

This ticket was mentioned in PR #1685 on WordPress/wordpress-develop by costdev.


2 months ago

  • Keywords has-patch added

#8 @SergeyBiryukov
2 months ago

  • Milestone changed from Awaiting Review to 5.9

Related: #40143

#9 in reply to: ↑ 4 @dd32
3 weeks ago

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)

Last edited 3 weeks ago by dd32 (previous) (diff)

#10 @costdev
13 days ago

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.

Note: See TracTickets for help on using tickets.