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

[imath]

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.

[dshanske]

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.

[imath]

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.

[imath]

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.

[davidbaumwald]

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.

[SergeyBiryukov]

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.

[ocean90]

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' ] ).

[imath]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[jeherve]

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?

[SergeyBiryukov]

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.

[SergeyBiryukov]

In 48227:

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

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

See #50513, #49236.

[SergeyBiryukov]

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.

[audrasjb]

Dev note published: ​https://make.wordpress.org/core/2020/07/29/miscellaneous-developer-focused-changes-in-wordpress-5-5/

[dartiss]

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?

[westonruter]

@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.

[imath]

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

[westonruter]

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

@@ -186 +186 @@
                                 return false;
                         }
 
+                        if ( empty( $_comment->comment_type ) ) {
+                                $_comment->comment_type = 'comment';
+                        }
+
                         wp_cache_add( $_comment->comment_ID, $_comment, 'comment' );
                 }

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

@@ -3840 +3840 @@
         $comment_batch_size = (int) apply_filters( 'wp_update_comment_type_batch_size', 100 );
 
         // Update the `comment_type` field value to be `comment` for the next batch of comments.
-        $wpdb->query(
+        $comment_ids = $wpdb->get_col(
                 $wpdb->prepare(
-                        "UPDATE {$wpdb->comments}
-                        SET comment_type = 'comment'
+                        "SELECT comment_ID
+                        FROM {$wpdb->comments}
                         WHERE comment_type = ''
                         ORDER BY comment_ID DESC
                         LIMIT %d",
@@ -3851 +3851 @@
                 )
         );
 
+        $in_comment_ids = implode( ',', wp_parse_id_list( $comment_ids ) );
+
+        $wpdb->query(
+                $wpdb->prepare(
+                        "UPDATE {$wpdb->comments}
+                        SET comment_type = 'comment'
+                        WHERE comment_type = '' AND comment_ID IN ({$in_comment_ids})"
+                )
+        );
+
+        clean_comment_cache( $comment_ids );
+
         delete_option( $lock_name );
 }

[imath]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

Reopening for the 5.5 branch.

[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.

Other than that, this looks good to backport. Marking dev-reviewed pending feedback on my point above. (I'll update with a patch)

[SergeyBiryukov]

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!

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[Ov3rfly]

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.

[TimothyBlynJacobs]

#38200 was marked as a duplicate.