Make WordPress Core

Opened 12 years ago

Closed 10 years ago

#20633 closed defect (bug) (fixed)

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

Reported by: markjaquith's profile markjaquith Owned by: wonderboymusic's profile 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 12 years ago.
20633.patch (716 bytes) - added by kitchin 11 years ago.
20633.2.patch (717 bytes) - added by DrewAPicture 11 years ago.
spacing
getCommentsPagesCount.php (6.8 KB) - added by mdbitz 11 years ago.
Unit Tests of get_comment_pages_count function
20633.2.diff (7.5 KB) - added by wonderboymusic 10 years ago.

Download all attachments as: .zip

Change History (20)

#1 @markjaquith
12 years ago

  • Version set to 3.3.2

@markjaquith
12 years ago

#2 @markjaquith
12 years ago

Make sure it's not empty.

#3 @SergeyBiryukov
12 years ago

  • Keywords has-patch added

#4 follow-up: @kitchin
11 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
11 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
11 years ago

#6 @kitchin
11 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
11 years ago

  • Milestone changed from Awaiting Review to 3.6

@DrewAPicture
11 years ago

spacing

#8 @nacin
11 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
11 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
11 years ago

  • Keywords 3.8-early added

@mdbitz
11 years ago

Unit Tests of get_comment_pages_count function

#11 @mdbitz
11 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
11 years ago

  • Milestone changed from Future Release to 3.9

#13 @nacin
10 years ago

  • Component changed from Warnings/Notices to Comments

#14 @wonderboymusic
10 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
10 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.