Make WordPress Core

Opened 2 years ago

Last modified 17 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 2 years ago.

Download all attachments as: .zip

Change History (17)

#1 @costdev
2 years ago

  • Type changed from defect (bug) to enhancement

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

  • Type changed from enhancement to task (blessed)

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

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


2 years ago
#7

  • Keywords has-patch added

#8 @SergeyBiryukov
2 years ago

  • Milestone changed from Awaiting Review to 5.9

Related: #40143

#9 in reply to: ↑ 4 @dd32
2 years 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"

Version 2, edited 2 years ago by dd32 (previous) (next) (diff)

#10 @costdev
2 years 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
2 years 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.


22 months ago

#14 @costdev
22 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
17 months ago

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

#16 @hellofromTonya
17 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.