Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 2 years ago

#49236 closed enhancement (fixed)

Use 'comment' instead of '' for the comment_type db field for comments

Reported by: imath's profile imath Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.5 Priority: normal
Severity: normal Version: 5.4
Component: Comments Keywords: has-patch early commit has-dev-note dev-reviewed
Focuses: Cc:

Description

Following @aaroncampbell and @jeremyfelt suggestions on #35214 this ticket is a first step to bring custom comment types into WordPress. This first step's goal is to start using comment for the comment_type field value of the wp_comments table.

To build the patch I've tried to make sure all functions/requests now use 'comment' instead of ''. I've tested the upgrade routine with some comments.

I haven't set the milestone as I'm unsure if this is something that is doable before 5.4-beta1. I imagine the upgrade routine might need some testing on websites having a lot of comments.

Attachments (6)

49236.patch (7.5 KB) - added by imath 5 years ago.
49236.2.patch (10.3 KB) - added by imath 5 years ago.
49236.3.patch (1.2 KB) - added by ocean90 5 years ago.
49236.3.ut.patch (3.7 KB) - added by imath 5 years ago.
49236.4.patch (3.1 KB) - added by imath 4 years ago.
49236-db-version-update.diff (1.3 KB) - added by desrosj 4 years ago.

Download all attachments as: .zip

Change History (44)

@imath
5 years ago

#1 @dshanske
5 years ago

You might want to have a look at the batch processing of terms introduced in #30261 for a way to split conversion into smaller jobs run by cron.

#2 @imath
5 years ago

Hi @dshanske

Thanks a lot for pointing me to the split terms upgrade ticket. I will look into it once 5.4.0 is released, which means I think it's safer to move this 5.5.0 once this milestone will be created.

This ticket was mentioned in Slack in #core by imath. View the logs.


5 years ago

#4 @imath
5 years ago

  • Keywords needs-refresh added
  • Milestone changed from Awaiting Review to 5.5

@imath
5 years ago

#5 @imath
5 years ago

  • Keywords early added; needs-refresh removed

I've just refreshed the patch using a batch process similar to what was done to split terms. It would be great if we could progress on this ticket early during 5.5.0 dev cycle.

I've tested the patch successfully after using the WP CLI command wp comment generate --count=500 --post_id=1

Thanks in advance for your help.

This ticket was mentioned in Slack in #core by imath. View the logs.


5 years ago

#7 @SergeyBiryukov
5 years ago

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

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


5 years ago

This ticket was mentioned in Slack in #core by imath. View the logs.


5 years ago

#10 @davidbaumwald
5 years ago

I've tested the most recent patch, and it works as intended. I also check that existing comments we slowly batch updated with a comment_type of "comment". I'm hoping this catches a review and moves forward for 5.5.

#11 @SergeyBiryukov
5 years ago

  • Keywords needs-dev-note added

#12 @SergeyBiryukov
5 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 47597:

Comments: Use comment instead of an empty string for the comment_type DB field value in comments table.

This is the first step to bring support for custom comment types into WordPress.

Add a scheduled upgrade routine to update the type value for existing comments, in batches of 100 at a time.

Props imath, aaroncampbell, jeremyfelt, dshanske.
Fixes #49236.

#13 @ocean90
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm seeing two issues in [47597]:

  • wp_insert_comment( [ 'comment_type' => '' ] ); will still insert an empty string due to the isset() check.
  • In WP_Comment_Query::get_comment_ids() the previous $comment_types[ $operator ][] = "''"; line needs to be restored otherwise not yet migrated comments will be missing when using get_comments( [ 'type' => 'comment' ] ).

@ocean90
5 years ago

@imath
5 years ago

#14 @imath
5 years ago

Thanks for catching these issues @ocean90.

About the first one, I suggest to add 2 unit tests (see 49236.3.ut.patch), 1 for wp_handle_comment_submission() and the other one for wp_insert_comment()

I agree we should commit your patch.

#15 @SergeyBiryukov
5 years ago

  • Keywords commit added

#16 @SergeyBiryukov
5 years ago

In 47625:

Comments: Restore inclusion of an empty comment type when building the WHERE clause in WP_Comment_Query::get_comment_ids().

This ensures that get_comments( array( 'type' => 'comment' ) ) still includes comments that have not yet migrated to the comment type.

Follow-up to [47597].

Props ocean90.
See #49236.

#17 @SergeyBiryukov
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 47626:

Comments: Ensure that inserting a comment with an empty type results in correct comment type.

Add unit tests for wp_handle_comment_submission() and wp_insert_comment() receiving an empty type.

Follow-up to [47597].

Props ocean90, imath.
Fixes #49236.

#18 @jeherve
5 years ago

I thought I would jump in to share my experience with this change, when maintaining themes.

In r47597, Twenty Ten had to be updated to take the new type into account and ensure that new comments get displayed properly on sites using that theme. This seems like a small change, but in practice I found that several themes, even popular ones that are actively maintained, use the same switch method as in Twenty Ten and will be impacted by this change.

A dev note (with a cross-post to make/themes) will definitely be useful for theme developers that monitor core changes. However, I worry that some theme authors may not have enough time to implement those changes and get all sites updated before WordPress 5.5 rolls out. This is especially true for themes in the WordPress.org theme repository.

With this in mind, should we attempt to post that dev note as early as possible so theme authors can start working on updates today?

#19 @SergeyBiryukov
4 years ago

In 48225:

Comments: Introduce wp_update_comment_type_batch_size filter for the comment batch size in _wp_batch_update_comment_type().

Follow-up to [47597].

Props dchymko.
Fixes #50513. See #49236.

#20 @SergeyBiryukov
4 years ago

In 48227:

Comments: Correct $wpdb->prepare() usage in _wp_batch_update_comment_type().

Follow-up to [47597], [48225].

See #50513, #49236.

#21 @SergeyBiryukov
4 years ago

In 48405:

Upgrade/Install: Prevent the upgrade routine for updating the comment_type field in the comments table from running twice.

Follow-up to [47597], [48400].

See #50413, #49236.

#23 @dartiss
4 years ago

Do we have any feeling on what impact this will have on sites with a large number of comments that need updating?

So, as an example, I've been asked this by a site owner that currently has around 4.5 million comments. Batching up in 100, do we have an idea of how long this might take and what's the impact whilst this is running?

#24 @westonruter
4 years ago

@imath I have a concern about how the SQL UPDATE will bypass updates to the object cache. I believe it would be preferable follow the model of wp_delete_auto_drafts() which grabs the post IDs and then loops over each to call wp_delete_post(). In the same way, instead of doing:

<?php
$wpdb->query(
        $wpdb->prepare(
                "UPDATE {$wpdb->comments}
                SET comment_type = 'comment'
                WHERE comment_type = ''
                ORDER BY comment_ID DESC
                LIMIT %d",
                $comment_batch_size
        )
);

Should it not rather do:

<?php
$comment_ids = $wpdb->get_col(
        $wpdb->prepare(
                "SELECT comment_ID
                FROM {$wpdb->comments}
                WHERE comment_type = ''
                ORDER BY comment_ID DESC
                LIMIT %d",
                $comment_batch_size
        )
);
foreach ( (array) $comment_ids as $comment_id ) {
        wp_update_comment(
                [
                        'comment_ID'   => $comment_id,
                        'comment_type' => 'comment',
                ]
        );
}

Otherwise, calls to get_comment() will not include the updated comment_type on sites with a persistent/external object cache.

Last edited 4 years ago by westonruter (previous) (diff)

#25 @imath
4 years ago

Hi @westonruter

Thanks a lot for your comment. I understand your concern. I'm fine with what you're suggesting, but I wonder if we could find a way that would require as less as possible database sollicitation.

If it only concerns the get_comment() function what about doing :

function get_comment( &$comment = null, $output = OBJECT ) {
	// beginning of the code of the function
	if ( ! $_comment ) {
		return null;
	}

	if ( '' === $_comment->comment_type ) {
		$_comment->comment_type = 'comment';
	}

	/**
	 * Fires after a comment is retrieved.

Otherwise, what about doing:

$comment_ids = $wpdb->get_col(
        $wpdb->prepare(
                "SELECT comment_ID
                FROM {$wpdb->comments}
                WHERE comment_type = ''
                ORDER BY comment_ID DESC
                LIMIT %d",
                $comment_batch_size
        )
);

$in_comment_ids = implode( ',', wp_parse_id_list( $comment_ids ) );

$wpdb->query(
        $wpdb->prepare(
                "UPDATE {$wpdb->comments}
                SET comment_type = 'comment'
                WHERE comment_ID IN ({$in_comment_ids})
        )
);

clean_comment_cache( $comment_ids );

#26 @westonruter
4 years ago

Changing get_comment() like so wouldn't be enough since it wouldn't account for people using WP_Comment directly. So it would need to be done at a lower level, like:

  • src/wp-includes/class-wp-comment.php

    a b final class WP_Comment { 
    186186                                return false;
    187187                        }
    188188
     189                        if ( empty( $_comment->comment_type ) ) {
     190                                $_comment->comment_type = 'comment';
     191                        }
     192
    189193                        wp_cache_add( $_comment->comment_ID, $_comment, 'comment' );
    190194                }

So I think I prefer the second alternative you proposed, where UPDATE is done followed by clean_comment_cache(), although with one adjustment: add a safeguard in the WHERE to double-confirm that the comment_type is indeed empty. Like so:

  • src/wp-includes/comment.php

    a b function _wp_batch_update_comment_type() { 
    38403840        $comment_batch_size = (int) apply_filters( 'wp_update_comment_type_batch_size', 100 );
    38413841
    38423842        // Update the `comment_type` field value to be `comment` for the next batch of comments.
    3843         $wpdb->query(
     3843        $comment_ids = $wpdb->get_col(
    38443844                $wpdb->prepare(
    3845                         "UPDATE {$wpdb->comments}
    3846                         SET comment_type = 'comment'
     3845                        "SELECT comment_ID
     3846                        FROM {$wpdb->comments}
    38473847                        WHERE comment_type = ''
    38483848                        ORDER BY comment_ID DESC
    38493849                        LIMIT %d",
    function _wp_batch_update_comment_type() { 
    38513851                )
    38523852        );
    38533853
     3854        if ( count( $comment_ids ) > 0 ) {
     3855                $in_comment_ids = implode( ',', wp_parse_id_list( $comment_ids ) );
     3856
     3857                $wpdb->query(
     3858                        $wpdb->prepare(
     3859                                "UPDATE {$wpdb->comments}
     3860                                SET comment_type = 'comment'
     3861                                WHERE comment_type = '' AND comment_ID IN ({$in_comment_ids})"
     3862                        )
     3863                );
     3864
     3865                clean_comment_cache( $comment_ids );
     3866        }
     3867
    38543868        delete_option( $lock_name );
    38553869}
Last edited 4 years ago by westonruter (previous) (diff)

#27 @SergeyBiryukov
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@imath
4 years ago

#28 @imath
4 years ago

Hi,

49236.4.patch is using the second alternative with @westonruter's improvements. I've also included the unit test I used to build the patch just in case.

This ticket was mentioned in Slack in #core by imath. View the logs.


4 years ago

#30 @SergeyBiryukov
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 48748:

Comments: Update comment cache in the upgrade routine for changing the comment_type DB field value in comments table.

This ensures that comment object cache is cleared after changing the comment type to comment instead of an empty string.

Add a unit test for _wp_batch_update_comment_type().

Follow-up to [47597], [47626], [48225], [48227].

Props imath, westonruter.
Fixes #49236.

#31 @SergeyBiryukov
4 years ago

  • Keywords dev-feedback added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for the 5.5 branch.

#32 follow-up: @desrosj
4 years ago

  • Keywords dev-reviewed added; dev-feedback removed

Looking at [48748], I'm wondering if we should bump the DB version to point to this commit in the upgrade routine. I don't think there is any benefit to a site that is running a development version (the upgrade routine will "run" again, but nothing will be updated and the caches will not clear because no comment IDs will be found), but may be good for accuracy.

Other than that, this looks good to backport. Marking dev-reviewed pending feedback on my point above.

Version 0, edited 4 years ago by desrosj (next)

#33 in reply to: ↑ 32 @SergeyBiryukov
4 years ago

Replying to desrosj:

Looking at [48748], I'm wondering if we should bump the DB version to point to this commit in the upgrade routine. I don't think there is any benefit to a site that is running a development version (the upgrade routine will "run" again, but nothing will be updated and the caches will not clear because no comment IDs will be found), but may be good for accuracy.

Yes, I was thinking of that and came to the same conclusion :) 49236-db-version-update.diff looks good, thanks!

#34 @SergeyBiryukov
4 years ago

In 48751:

Comments: Update DB version number used to trigger the upgrade routine for changing the comment_type DB field value in comments table.

Follow-up to [47597], [47626], [48225], [48227], [48748].

Props desrosj.
See #49236.

#35 @SergeyBiryukov
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 48752:

Comments: Update comment cache in the upgrade routine for changing the comment_type DB field value in comments table.

This ensures that comment object cache is cleared after changing the comment type to comment instead of an empty string.

Add a unit test for _wp_batch_update_comment_type().

Follow-up to [47597], [47626], [48225], [48227].

Props imath, westonruter.
Reviewed by desrosj, SergeyBiryukov.
Merges [48748] and [48751] to the 5.5 branch.
Fixes #49236.

#36 @Ov3rfly
4 years ago

Posting this here as this ticket was refered to in #51082 by @jeremyfelt

.. Twenty Ten had to be updated to take the new type into account and ensure that new comments get displayed properly ..

Actually Twenty Ten and some other themes out there did it right.

They check the actual value of comment_type, an empty string is a valid value.

Any new comment type would be not shown - as expected.

Most other themes including Twenty Eleven, Twenty Twelve and hundreds more https://wpdirectory.net/search/01EG85WWS35NAC1V7GH59XN2MW are doing it wrong.

They use callbacks with this code:

switch ( $comment->comment_type ) :
	case 'pingback'  :
	case 'trackback' :
		break;
	default :
		// print normal comment here...
		break;
endswitch;

These themes still "work" with the new behaviour of WP 5.5 but that's accidentally only and can/will lead to unexpected results if ever a new comment_type will be introduced - each and every theme will have to be updated.

The correct way to implement this custom comment enhancement would be:

  • Add a deprecated notice if callback param is used, keep rest of code for the time being.
  • Add new param callback_new which can be used by themes to handle all including new comment types in future.
  • Provide example code with callback_new in Twenty XXX which checks actual value of comment_type, including a check for empty string, and has no/empty default case.

There imho was no need at all to change the default value of comment_type and now mess around in years old rows in huge databases with unknown side effects and still get unexpected results with above mentioned themes without extra updates there...

Would urgently suggest to rethink this whole approach.

#37 @TimothyBlynJacobs
4 years ago

#38200 was marked as a duplicate.

This ticket was mentioned in Slack in #buddypress by imath. View the logs.


2 years ago

Note: See TracTickets for help on using tickets.