WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#32619 closed task (blessed) (fixed)

Introduce WP_Comment

Reported by: wonderboymusic Owned by: wonderboymusic
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: Comments Keywords: has-patch
Focuses: Cc:

Description

Modeling is a thing. WP_Comment can do for comments what WP_Post does for posts.

Attachments (8)

32619.diff (28.7 KB) - added by wonderboymusic 3 years ago.
get_comment.diff (5.9 KB) - added by nacin 3 years ago.
The patch I haven't looked at in a year.
32619.2.diff (28.9 KB) - added by wonderboymusic 3 years ago.
32619.3.diff (30.7 KB) - added by DrewAPicture 3 years ago.
Fixed docs
32619.4.diff (28.1 KB) - added by wonderboymusic 3 years ago.
32619.5.patch (1.1 KB) - added by dimadin 3 years ago.
32619.6.patch (629 bytes) - added by ocean90 3 years ago.
32619.patch (543 bytes) - added by DrewAPicture 3 years ago.

Download all attachments as: .zip

Change History (31)

@wonderboymusic
3 years ago

#1 follow-up: @TimothyBlynJacobs
3 years ago

Oh god, yes please. Currently working on a project where this would be very helpful. Is there a reason the class is marked as final and follows WP_Post rather than WP_User which is much more extensible?

#2 in reply to: ↑ 1 @SergeyBiryukov
3 years ago

Replying to TimothyBlynJacobs:

Is there a reason the class is marked as final and follows WP_Post rather than WP_User which is much more extensible?

Same reason as in comment:3:ticket:24672, I guess.

#3 @rachelbaker
3 years ago

Amazing!

#4 @iamfriendly
3 years ago

If I could add a taco emoji I would, because that is how good this thing is.

#5 @nacin
3 years ago

As discussed over Twitter with @wonderboymusic, I actually wrote this patch in June of 2014, and I still have it, so I'm going to compare them to see if either of us missed anything. Before committing [32769], I resolved to figure this out. (It was also on my hit list for 4.3.)

@nacin
3 years ago

The patch I haven't looked at in a year.

#6 @nacin
3 years ago

(I didn't do metadata support on purpose, but I'll have to think about why a bit more. Probably because I think it's a bit weird.)

#7 @obenland
3 years ago

@wonderboymusic, beta1 is in a week, do you think this will be ready until then?

#8 @wonderboymusic
3 years ago

  • Owner set to nacin
  • Status changed from new to assigned
  • Type changed from enhancement to task (blessed)

@nacin will take care of this

#9 @jorbin
3 years ago

  • Milestone changed from 4.3 to Future Release

I think this is too late for 4.3 at this point. We've already done beta2 and this hasn't had any traction for a few weeks.

#10 @wonderboymusic
3 years ago

  • Milestone changed from Future Release to 4.4

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


3 years ago

#12 @wonderboymusic
3 years ago

  • Keywords commit added

32619.2.diff refreshes my patch after violent amounts of churn. All unit tests still pass. Comments are garbage and are begging for this

#13 @DrewAPicture
3 years ago

  • Keywords commit removed
  • Owner changed from nacin to DrewAPicture
  • Status changed from assigned to reviewing

We need to do a little bit of docs work here before this would be ready for commit.

I'd also probably recommend committing the class separately from the various function changes just so there's a clean break between introducing the class and integrating it.

@DrewAPicture
3 years ago

Fixed docs

#14 @DrewAPicture
3 years ago

  • Keywords commit added
  • Owner changed from DrewAPicture to wonderboymusic

32619.3.diff shores up the docs for WP_Comment:

  • Adds a file header to class-wp-comment.php
  • Adjusts the class DocBlock summary to better describe the purpose of the class rather than focus on what it is
  • Added property descriptions, @since versions, @access tags
  • Changed the type on $comment_author to string from int for the author name
  • Adds legit summaries to all methods, also filled out the DocBlocks with meaningful information
Last edited 3 years ago by DrewAPicture (previous) (diff)

#15 follow-up: @wonderboymusic
3 years ago

In 33891:

Introduce WP_Comment class to model/strongly-type rows from the comments database table. Inclusion of this class is a pre-req for some more general comment cleanup and sanity.

  • Takes inspiration from WP_Post and adds sanity to comment caching.
  • Clarifies when the current global value for $comment is returned. The current implementation in get_comment() introduces side effects and an occasion stale global value for $comment when comment caches are cleaned.
  • Strongly-types @param docs
  • This class is marked final for now

Props wonderboymusic, nacin.

See #32619.

#16 follow-up: @DrewAPicture
3 years ago

In 33893:

Docs: Add complete file, class, property, and method documentation for the new WP_Comment class, introduced in [33891].

It's important for new functionality, especially something as significant as a new class to have complete documentation upon initial commit – not after the fact.

See #32619.

#17 in reply to: ↑ 16 @ryan
3 years ago

Replying to DrewAPicture:

It's important for new functionality, especially something as significant as a new class to have complete documentation upon initial commit – not after the fact.

Let's be wary of resurrecting this bad habit. Providing docs on initial commit is full of virtue. We are sufficiently stockpiled on do-it-laters. ;-)

#18 in reply to: ↑ 15 @dimadin
3 years ago

Replying to wonderboymusic:

You made mistake in wp-includes/feed.php. I've just reverted changes and changed object to WP_Comment.

Although I think that documentation for get_comment() parameter and these functions that use it should be refreshed and synced.

@dimadin
3 years ago

#19 @wonderboymusic
3 years ago

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

In 33913:

Correct the param docs for comment_guid() and get_comment_guid().

Props dimadin.
Fixes #32619.

#20 follow-up: @ocean90
3 years ago

  • Keywords commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

[33891] has changed get_avatar_data() to only support a WP_Comment object and no longer a raw comment object from the database, trunk/src/wp-includes/link-template.php@33891#L3620. This broke the recent comment widget in P2, https://themes.trac.wordpress.org/browser/p2/1.5.5/inc/widgets/recent-comments.php#L143.

I think we should continue to support both, see 32619.6.patch.

@ocean90
3 years ago

@DrewAPicture
3 years ago

#21 in reply to: ↑ 20 @DrewAPicture
3 years ago

Replying to ocean90:

[33891] has changed get_avatar_data() to only support a WP_Comment object and no longer a raw comment object from the database, trunk/src/wp-includes/link-template.php@33891#L3620. This broke the recent comment widget in P2, https://themes.trac.wordpress.org/browser/p2/1.5.5/inc/widgets/recent-comments.php#L143.

I think we should continue to support both, see 32619.6.patch.

Wouldn't it be better to normalize the object as a WP_Comment instance instead of essentially promoting the edge case by checking for a general object alongside the instanceof?

I'd argue that in introducing WP_Comment we'd want to encourage a single pathway going forward wherever possible. See 32619.patch

#22 @wonderboymusic
3 years ago

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

In 34160:

In get_avatar_data(), promote stdClass objects to WP_Comment if passed.

Props DrewAPicture, ocean90.
Fixes #32619.

#23 @wonderboymusic
3 years ago

In 34244:

After [34160], also upgrade objects passed to get_avatar().

See #32619.

Note: See TracTickets for help on using tickets.