-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Incorporating foundation of "Notes" feature #10280
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
004f7fb
to
e08eea2
Compare
src/wp-includes/rest-api/endpoints/class-wp-rest-comments-controller.php
Outdated
Show resolved
Hide resolved
Ok, I believe backporting PHP should be all done. Next, I'll try to fix the failing tests. |
I think we're missing the Notes-specific unit tests. |
$post_id_in = "'" . implode( "', '", $post_id_array ) . "'"; | ||
|
||
$pending = $wpdb->get_results( "SELECT comment_post_ID, COUNT(comment_ID) as num_comments FROM $wpdb->comments WHERE comment_post_ID IN ( $post_id_in ) AND comment_approved = '0' GROUP BY comment_post_ID", ARRAY_A ); | ||
$pending = $wpdb->get_results( "SELECT comment_post_ID, COUNT(comment_ID) as num_comments FROM $wpdb->comments WHERE comment_post_ID IN ( $post_id_in ) AND comment_approved = '0' AND comment_type != 'note' GROUP BY comment_post_ID", ARRAY_A ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's another PR for get_pending_comments_num
changes - #9869.
Just noting for props.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of hardcoding comment types. Long term we should think about using a filterable list of comment types to exclude or using (an improved) WP_Comment_Query
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100%. I think a long-term solution is to provide proper support for custom comment types, so we can avoid hardcoded patches like we've introduced with the current implementation.
48d0ba7
to
b351ddb
Compare
src/wp-includes/comment.php
Outdated
*/ | ||
$dupe_id = apply_filters( 'duplicate_comment_id', $dupe_id, $commentdata ); | ||
|
||
// Allow duplicate notes for resolution purposes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this should happen after the apply_filters
call. Seems like we'd want to let developers customize behavior here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, I'd prefer we keep this filterable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a43a2b0
src/wp-includes/comment.php
Outdated
* | ||
* @since 6.9.0 | ||
*/ | ||
function wp_register_notes_metadata() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd model this closer to wp_create_initial_post_meta
, so wp_create_initial_comment_meta
. Future additions would be added into that one function, instead of splitting each registration into it's own function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 48cb687
src/wp-includes/comment.php
Outdated
'auth_callback' => function ( $allowed, $meta_key, $object_id ) { | ||
return current_user_can( 'edit_comment', $object_id ); | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already the default behavior applied by map_meta_cap
, I believe this is just resulting in a duplicate check,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 278d274
Ditto all the feedback about wishing we had a fleshed out comment types API about now. But I think this looks pretty reasonable. |
Co-authored-by: Timothy Jacobs <[email protected]>
Co-authored-by: Timothy Jacobs <[email protected]>
'count' => true, | ||
'update_comment_meta_cache' => false, | ||
'orderby' => 'none', | ||
'type__not_in' => array( 'note' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, does this work well with WP-CLI out of the box when dealing with comments? |
Do you have any particular concerns, @swissspidy? I've only used WP-CLI to clean up test data or migrate notes after the type change. It worked perfectly. @t-hamano, what are the remaining blockers here? |
I think I've addressed all the feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
Left one small comment feedback
Co-authored-by: Adam Silverstein <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems all the feedback has been addressed.
@t-hamano or @adamsilverstein, are you planning to handle the commit?
I haven't committed to core yet, so I'd be happy if someone could do that 😅 That said, I'd like to set up an SVN environment, understand the core committer workflow, and try my hand at committing within the 6.9 cycle. |
This PR updates the core codebase to allow the Notes feature to work properly.
Trac ticket: https://core.trac.wordpress.org/ticket/64096
Note
This PR also includes #9869. Don't forget to add the following prop when committing to core: