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

[markjaquith]

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

[markjaquith]

Make sure it's not empty.

[kitchin]

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;

[DrewAPicture]

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]

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.

[DrewAPicture]

spacing

[nacin]

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.

[kitchin]

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.

[mdbitz]

Unit Tests of get_comment_pages_count function

[mdbitz]

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.

[wonderboymusic]

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.

[wonderboymusic]

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.