Make WordPress Core

Opened 21 months ago

Last modified 8 months ago

#54149 assigned defect (bug)

Audit `get_comment()` response checks.

Reported by: costdev's profile costdev Owned by: hellofromtonya's profile hellofromTonya
Milestone: Future Release Priority: normal
Severity: normal Version: 5.9
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 21 months ago.

Download all attachments as: .zip

Change History (17)

#1 @costdev
21 months ago

  • Type changed from defect (bug) to enhancement

#2 @costdev
21 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
21 months ago

  • Type changed from enhancement to task (blessed)

#4 in reply to: ↑ description ; follow-ups: @jrf
21 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
21 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
21 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 21 months ago by costdev (previous) (diff)

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


21 months ago
#7

  • Keywords has-patch added

#8 @SergeyBiryukov
20 months ago

  • Milestone changed from Awaiting Review to 5.9

Related: #40143

#9 in reply to: ↑ 4 @dd32
19 months 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 19 months ago by dd32 (previous) (diff)

#10 @costdev
19 months 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.

#11 @hellofromTonya
18 months ago

  • Milestone changed from 5.9 to 6.0

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

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


13 months ago

#14 @costdev
13 months ago

  • Milestone changed from 6.0 to 6.1

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.

#15 @davidbaumwald
8 months ago

  • Type changed from task (blessed) to defect (bug)

#16 @hellofromTonya
8 months ago

  • Milestone changed from 6.1 to Future Release
  • Owner set to hellofromTonya
  • Status changed from new to assigned

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.

Note: See TracTickets for help on using tickets.