Make WordPress Core

Opened 8 years ago

Last modified 4 years ago

#34475 reviewing defect (bug)

get_comment_link incorrect page number in returned url.

Reported by: property118's profile Property118 Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version: 4.3.1
Component: Comments Keywords: reporter-feedback needs-unit-tests has-patch
Focuses: Cc:

Description

Since 4.3 core update the get_comment_link function returns incorrect page numbers for paginated comments. Have tried using default twentyfifteen theme with no plugins enabled and issue still persists.

Attachments (1)

34475.1.patch (1.3 KB) - added by Mista-Flo 8 years ago.
Fix parent arg strange behavior with comments pagination

Download all attachments as: .zip

Change History (20)

#1 @boonebgorges
8 years ago

  • Keywords reporter-feedback added

Hi Property118 - Can you please give more details? What are the "incorrect" numbers, and what do you expect them to be? What are your pagination settings on Settings > Discussion?

#2 @Property118
8 years ago

Hi,

The incorrect numbers as in saying page 154 when should be 416, the link given is:
http://www.property118.com/budget-2015-landlords-reactions/comment-page-154/#comment-64655

pagination settings:
Comment author must fill out name and e-mail - YES
Users must be registered and logged in to comment - NO
Automatically close comments on articles older than 14 Days - NO
Enable threaded (nested) comments 10 levels Deep - NO
Break comments into pages with 10 top level comments per page and the First page displayed by default - YES
Comments should be displayed with the 'OLDER' comments at the top of each page.

The code used to call the function is: $link = get_comment_link($comment->comment_ID, array('per_page'=>10));

Not sure if this also helps.

Thanks

Last edited 8 years ago by Property118 (previous) (diff)

#3 follow-up: @boonebgorges
8 years ago

Thanks for the details. Comments appearing on http://www.property118.com/budget-2015-landlords-reactions/76164/comment-page-154 *do* appear to have their comment permalinks (the timestamps) pointing to the correct page. Where are the incorrect comment links being generated? In the sidebar widget? Are you calling get_comment_link() somewhere in a custom template?

#4 in reply to: ↑ 3 @Property118
8 years ago

Replying to boonebgorges:

Thanks for the details. Comments appearing on http://www.property118.com/budget-2015-landlords-reactions/76164/comment-page-154 *do* appear to have their comment permalinks (the timestamps) pointing to the correct page. Where are the incorrect comment links being generated? In the sidebar widget? Are you calling get_comment_link() somewhere in a custom template?

Hi boonebgorges,

I am testing at the moment using custom code as below:

<?php
function commenttest(){
        $commentstotal = count(get_comments('status=approve&post_id=76164'));
        echo "commentstotal: ".$commentstotal."<br/>";
        $comments = get_comments('status=approve&number=5');
        $output = '<ul style="font-size:5px">';
    foreach ($comments as $comment) { 
        
        $output .= '<li style="font-size:10px;">
                                <div style="float:left;margin-right:3px">';
                                        $output .= get_avatar( $comment, '35' );
                                $output .= '</div>
                                        <em style="font-size:12px">';
                                    $output .= strip_tags($comment->comment_author); 
                                    $output .= '</em> (<a href="';
                                    $output .= get_option('home'); 
                                    $output .= '/?p=';
                                    $output .= $comment->comment_post_ID;
                                    $output .= '/#comment-';
                                    $output .= $comment->comment_ID;
                                    $output .= '">link</a>)<br>';
                                                $output .= $comment->comment_content;
                                    $output .= wp_html_excerpt( $comment->comment_content, 35 ); 
                                    $link = get_comment_link($comment->comment_ID, array('per_page'=>10));
                                    $output .= '...<br/>                                    LINK: '.$link.'
                                    <div style="clear:both;"></div>
        </li>';
     }  
        $output .= '</ul>';
        echo $output;
}

However the call to the get_comments_link is the same on the comment notifier plugin (and both the comment notifier plugin and this function give exactly the same results).

Comment Notifier Plugin hasnt been updated in two years by the dev but i have checked through all the code and it is retrieving the correct comment ids which are being passed to get_comment_link then giving the incorrect page number in the output url.

The function above is run on the following page if this helps?

http://www.property118.com/cron-test/

Thanks

Last edited 8 years ago by Property118 (previous) (diff)

#5 follow-up: @boonebgorges
8 years ago

Where are you putting the commenttest() code for testing? And where does Comment Notifier Plugin run its query? Is it inside of a template? There are some flags in get_comment_link() (like $in_comment_loop) that are meant to speed the function up, but could be causing problems with leaky globals if they're called in certain nested ways.

#6 in reply to: ↑ 5 @Property118
8 years ago

Replying to boonebgorges:

Where are you putting the commenttest() code for testing? And where does Comment Notifier Plugin run its query? Is it inside of a template? There are some flags in get_comment_link() (like $in_comment_loop) that are meant to speed the function up, but could be causing problems with leaky globals if they're called in certain nested ways.

the commenttest() function is on a page called cron-test (it was being used for crontasks and iv just reused it for this).
http://www.property118.com/cron-test/

the code in the notifier plugin is:

<?php
$data = new stdClass();
    $data->post_id = $post_id;
    $data->title = $post->post_title;
    $data->link = get_permalink($post_id);
    $data->comment_link = $data->link . '#comment-' . $comment_id;
    $data->comment_link = get_comment_link($comment_id, array('per_page'=>10));

after that the data variable is used to populate an email, nothing out of the ordinary there.

Thanks

#7 follow-up: @boonebgorges
8 years ago

Sorry to be pedantic, but can you tell me exactly where the commenttest() code is? Are you just pasting it into the cron-test post content (and running a plugin that parses the PHP)? Or is this a custom page template? I assume that the "comment notifier" plugin you've referred to is https://wordpress.org/plugins/comments-notifier/; it appears that it runs at 'comment_post'. The exact place in the load order may be important for the purposes of your issue.

Can you verify whether, in these problematic cases, get_comment_link() is calling get_page_of_comment()?

get_page_of_comment() and get_comment_link() logic has been completely overhauled for 4.4. Is there any chance you have a testing environment where you can run the latest 4.4 beta to see if it resolves your issues?

#8 in reply to: ↑ 7 @Property118
8 years ago

Replying to boonebgorges:

Sorry to be pedantic, but can you tell me exactly where the commenttest() code is? Are you just pasting it into the cron-test post content (and running a plugin that parses the PHP)? Or is this a custom page template? I assume that the "comment notifier" plugin you've referred to is https://wordpress.org/plugins/comments-notifier/; it appears that it runs at 'comment_post'. The exact place in the load order may be important for the purposes of your issue.

Can you verify whether, in these problematic cases, get_comment_link() is calling get_page_of_comment()?

get_page_of_comment() and get_comment_link() logic has been completely overhauled for 4.4. Is there any chance you have a testing environment where you can run the latest 4.4 beta to see if it resolves your issues?

No problems at all, the commenttest() code is currently on my dev version in the functions.php file of twentyfifteen, this is the only custom code in that theme, all plugins are disabled (and plugin folder renamed to -plugins for good measure)

Yes that is the exact notifier plugin.

On the live site i gave you the urls for they are in the custom property118 themes functions file, however as that version is live it still has all the plugins enabled etc, the issue persists on both my dev and live sites though regardless of theme used or plugins.

cron-test page is a custom template with the following code:

<?php
<?php /* Template Name: CronTest Page Template */ 
error_reporting(E_ALL);
ini_set('display_errors', 1);
get_header('btl'); ?>
<div style='background:#F9F5F5; margin-top:-20px;margin-bottom:-10px;width:100%;display:inline-block;left:0px;'>
                <section>
                        <div class='wrapper'>
                        <h1>Test Page</h1>
<?php 
commenttest();
?>
                        </div>
                </section>
</div>

sorry to sound silly but how do i check if get_comment_link() is calling get_page_of_comment()?

I have a local development machine with a docker instance for my dev version of the site that yes i can install 4.4 beta onto.

Thanks

Last edited 8 years ago by Property118 (previous) (diff)

#9 @Property118
8 years ago

I have installed 4.4 beta and simply copied the database over and the cronttest function and page template and it still shows as the incorrect page number.

I can also confirm on the 4.4 beta that the get_comment_link function is indeed calling get_page_of_comment().

Thanks

Last edited 8 years ago by Property118 (previous) (diff)

#10 @Property118
8 years ago

After a little more investigation, the issue appears to come from the get_page_of_comment in comment-functions.php.

<?php
// Find this comment's top level parent if threading is enabled
                if ( $args['max_depth'] > 1 && 0 != $comment->comment_parent )
                        return get_page_of_comment( $comment->comment_parent, $args );

                $comment_args = array(
                        'type'       => $args['type'],
                        'post_id'    => $comment->comment_post_ID,
                        'fields'     => 'ids',
                        'count'      => true,
                        'status'     => 'approve',
                        'parent'     => 0,
                        'date_query' => array(
                                array(
                                        'column' => "$wpdb->comments.comment_date_gmt",
                                        'before' => $comment->comment_date_gmt,
                                )
                        ),
                );

if the parent option in the comment args is not set then the function

<?php
$comment_query = new WP_Comment_Query();
$older_comment_count = $comment_query->query( $comment_args );

will actually return the correct page number (in this case 416), as soon as that option is set then the function ignores any nested comments and churns out incorrect page numbers.

Thanks

#11 @Property118
8 years ago

A further update, i can also confirm that on 4.3 changing the below solves the issue.

From

<?php
$comtypewhere = ( 'all' != $args['type'] && isset($allowedtypes[$args['type']]) ) ? " AND comment_type = '" . $allowedtypes[$args['type']] . "'" : '';

        // Count comments older than this one
        $oldercoms = $wpdb->get_var( $wpdb->prepare( "SELECT COUNT(comment_ID) FROM $wpdb->comments WHERE comment_post_ID = %d AND comment_parent = 0 AND comment_approved = '1' AND comment_date_gmt < '%s'" . $comtypewhere, $comment->comment_post_ID, $comment->comment_date_gmt ) );

To

<?php
$comtypewhere = ( 'all' != $args['type'] && isset($allowedtypes[$args['type']]) ) ? " AND comment_type = '" . $allowedtypes[$args['type']] . "'" : '';

        // Count comments older than this one
        $oldercoms = $wpdb->get_var( $wpdb->prepare( "SELECT COUNT(comment_ID) FROM $wpdb->comments WHERE comment_post_ID = %d AND comment_approved = '1' AND comment_date_gmt < '%s'" . $comtypewhere, $comment->comment_post_ID, $comment->comment_date_gmt ) );

So basically again removing the parent=0 clause fixes the issue.

Thanks

#12 @Property118
8 years ago

  • Keywords reporter-feedback removed

#14 @boonebgorges
8 years ago

  • Keywords reporter-feedback needs-patch needs-unit-tests added

Hi @Property118 - I'm circling back around to this ticket. Sorry for the delay while we worked on 4.4.

It's a bit perplexing that changing the "parent" parameter for the comment query would make a difference for you. You originally stated that threaded comments are disabled on your site. This should mean that all comments on the site have parent=0. This suggests that, at some point in the past, you had enabled threaded comments on your site. Can you verify?

For cases like this one, maybe there should be a switch in get_page_of_comment() that omits the parent param when comment threading is disabled.

This ticket was mentioned in Slack in #core-comments by rachelbaker. View the logs.


8 years ago

#16 @Mista-Flo
8 years ago

  • Keywords has-patch added; needs-patch removed

Hi, I have made a patch according to your observation @boonebgorges
Maybe I could use ternary operator directly in the arg array to avoid extra lines with $parent var.

It still needs unit tests to prevent any strange behavior.

Last edited 8 years ago by Mista-Flo (previous) (diff)

@Mista-Flo
8 years ago

Fix parent arg strange behavior with comments pagination

#17 @Mista-Flo
5 years ago

@SergeyBiryukov If you could have a look to this patch please :)

#18 @SergeyBiryukov
5 years ago

  • Milestone set to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#19 @davidbaumwald
4 years ago

  • Milestone changed from 5.3 to Future Release

This ticket hasn't seen any movement this cycle. With 5.3 RC1 approaching, this is being moved to Future Release. If a committer feels strongly about this one being in 5.3 and has the time to see it through, it can be moved back up.

Note: See TracTickets for help on using tickets.