WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 4 weeks ago

Last modified 3 days ago

#49236 closed enhancement (fixed)

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

Reported by: imath Owned by: SergeyBiryukov
Milestone: 5.5 Priority: normal
Severity: normal Version: 5.4
Component: Comments Keywords: has-patch early needs-dev-note commit
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 (4)

49236.patch (7.5 KB) - added by imath 4 months ago.
49236.2.patch (10.3 KB) - added by imath 3 months ago.
49236.3.patch (1.2 KB) - added by ocean90 5 weeks ago.
49236.3.ut.patch (3.7 KB) - added by imath 5 weeks ago.

Download all attachments as: .zip

Change History (22)

@imath
4 months ago

#1 @dshanske
4 months 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
4 months 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.


4 months ago

#4 @imath
3 months ago

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

@imath
3 months ago

#5 @imath
3 months 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.


2 months ago

#7 @SergeyBiryukov
2 months 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.


6 weeks ago

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


6 weeks ago

#10 @davidbaumwald
6 weeks 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 weeks ago

  • Keywords needs-dev-note added

#12 @SergeyBiryukov
5 weeks 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 weeks 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 weeks ago

@imath
5 weeks ago

#14 @imath
5 weeks 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 weeks ago

  • Keywords commit added

#16 @SergeyBiryukov
4 weeks 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
4 weeks 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
3 days 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?

Note: See TracTickets for help on using tickets.