Make WordPress Core

Opened 2 years ago

Closed 3 months 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:


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 2 years ago.
20633.patch (716 bytes) - added by kitchin 13 months ago.
20633.2.patch (717 bytes) - added by DrewAPicture 12 months ago.
getCommentsPagesCount.php (6.8 KB) - added by mdbitz 4 months ago.
Unit Tests of get_comment_pages_count function
20633.2.diff (7.5 KB) - added by wonderboymusic 3 months ago.

Download all attachments as: .zip

Change History (20)

comment:1 markjaquith2 years ago

  • Version set to 3.3.2

markjaquith2 years ago

comment:2 markjaquith2 years ago

Make sure it's not empty.

comment:3 SergeyBiryukov2 years ago

  • Keywords has-patch added

comment:4 follow-up: kitchin13 months 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.


- 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;

comment:5 in reply to: ↑ 4 DrewAPicture13 months ago

  • Keywords dev-feedback added

Replying to kitchin:


- 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?

kitchin13 months ago

comment:6 kitchin13 months 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.

comment:7 SergeyBiryukov13 months ago

  • Milestone changed from Awaiting Review to 3.6

DrewAPicture12 months ago


comment:8 nacin10 months 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.

comment:9 kitchin7 months 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.

comment:10 SergeyBiryukov7 months ago

  • Keywords 3.8-early added

mdbitz4 months ago

Unit Tests of get_comment_pages_count function

comment:11 mdbitz4 months 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.

comment:12 SergeyBiryukov4 months ago

  • Milestone changed from Future Release to 3.9

comment:13 nacin3 months ago

  • Component changed from Warnings/Notices to Comments

wonderboymusic3 months ago

comment:14 wonderboymusic3 months 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.

comment:15 wonderboymusic3 months 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.