WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 4 years ago

#20633 closed defect (bug) (fixed)

wp-includes/comment.php:738 - Undefined property: WP_Query::$comments

Reported by: markjaquith Owned by: wonderboymusic
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.3.2
Component: Comments Keywords: has-patch needs-unit-tests 3.8-early
Focuses: Cc:

Description

get_comment_pages_count() can throw a notice. We use $wp_query->comments without checking it first.

Attachments (5)

20633.diff (638 bytes) - added by markjaquith 6 years ago.
20633.patch (716 bytes) - added by kitchin 5 years ago.
20633.2.patch (717 bytes) - added by DrewAPicture 5 years ago.
spacing
getCommentsPagesCount.php (6.8 KB) - added by mdbitz 5 years ago.
Unit Tests of get_comment_pages_count function
20633.2.diff (7.5 KB) - added by wonderboymusic 4 years ago.

Download all attachments as: .zip

Change History (20)

#1 @markjaquith
6 years ago

  • Version set to 3.3.2

@markjaquith
6 years ago

#2 @markjaquith
6 years ago

Make sure it's not empty.

#3 @SergeyBiryukov
6 years ago

  • Keywords has-patch added

#4 follow-up: @kitchin
5 years ago

The patch is not correct. It does this:

- if ( !$comments || !is_array($comments) ) 
+ if ( ( !$comments || !is_array($comments) ) && !empty( $wp_query->comments ) ) 
    $comments = $wp_query->comments;
  if ( empty($comments) )
    return 0;

The patch defeats the is_array() check. For example, $comments=true, $wp_query->comments=null will not return 0.

Suggest:

- if ( !$comments || !is_array($comments) )
+ if ( empty($comments) || !is_array($comments) ) {
+   if (empty( $wp_query->comments ) )
+     return 0;
    $comments = $wp_query->comments;
+ }
- if ( empty($comments) )
-    return 0;

#5 in reply to: ↑ 4 @DrewAPicture
5 years ago

  • Keywords dev-feedback added

Replying to kitchin:

Suggest:

- if ( !$comments || !is_array($comments) )
+ if ( empty($comments) || !is_array($comments) ) {
+   if (empty( $wp_query->comments ) )
+     return 0;
    $comments = $wp_query->comments;
+ }
- if ( empty($comments) )
-    return 0;

Would you like to submit an updated patch?

@kitchin
5 years ago

#6 @kitchin
5 years ago

Easiest place to see this bug is before the Loop. I put a note about that in the Codex, since then the function returns 0 with or without the bug fix. http://codex.wordpress.org/index.php?title=Function_Reference%2Fget_comment_pages_count&diff=128919&oldid=126895

A test for this bug is to set WP_DEBUG to true. In wp-content/themes/twentythirteen/single.php insert

  <?php printf('<pre>get_comment_pages_count=%s</pre>',
     get_comment_pages_count()); ?>

before and after the loop starts, at

  <?php comments_template(); ?>

And look at the Hello World post. Expect 0 then 1, with 1 warning before the bug and no warnings after. Another test is to make sure

  <?php printf('<pre>get_comment_pages_count=%s</pre>',
     get_comment_pages_count($GLOBALS['wp_query']->comments)); ?>

still works after the Loop. Expect 1.

But I didn't see anything similar in the unit tests as far as I could tell.

#7 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 3.6

@DrewAPicture
5 years ago

spacing

#8 @nacin
5 years ago

  • Keywords needs-unit-tests added; dev-feedback removed
  • Milestone changed from 3.6 to Future Release

I would feel much better about these awkward logic changes if get_comment_pages_count() had some basic unit tests for various values and types of $comments and $wp_query->comments.

#9 @kitchin
5 years ago

As an alternative, here's a one-line fix to stop the WP_DEBUG warning. It does not change behavior.

-		$comments = $wp_query->comments;
+		$comments = isset( $wp_query->comments ) ? $wp_query->comments : null;

I'll make a patch if there's any interest.

#10 @SergeyBiryukov
5 years ago

  • Keywords 3.8-early added

@mdbitz
5 years ago

Unit Tests of get_comment_pages_count function

#11 @mdbitz
5 years ago

  • Cc matt@… added

Hi, I've attached a set of unit tests for the get_comment_pages_count function. I'd appreciate some feedback on it especially concerning the $wp_query portion and ensuring the tests do not modify any of the objects for other tests.

Also please let me know if you feel any logic was overlooked, hopefully this will let us close this ticket using one of the methods above. I'm in favor of the logic by Kitchin as it does not modify return logic.

#12 @SergeyBiryukov
5 years ago

  • Milestone changed from Future Release to 3.9

#13 @nacin
4 years ago

  • Component changed from Warnings/Notices to Comments

#14 @wonderboymusic
4 years ago

20633.2.diff updates the proposed Unit Tests to actually trigger the error described in the ticket. The portion Mark provided fixes them. I also removed the $wp_query logic in setUp/tearDown - most of the tests themselves override the entirety of $wp_query by running new queries. $wp_query is not expected to be maintained between tests.

#15 @wonderboymusic
4 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 27055:

Add Unit Tests for get_comment_pages_count(). Fix a notice caused when $wp_query->comments is not set in that function.

Props mdbitz, markjaquith.
Fixes #20633.

Note: See TracTickets for help on using tickets.