Skip to content
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

Address some of things found in #633 #634

Merged
merged 2 commits into from
Jan 5, 2024
Merged

Address some of things found in #633 #634

merged 2 commits into from
Jan 5, 2024

Conversation

janboddez
Copy link
Contributor

@janboddez janboddez commented Jan 5, 2024

Fixes some of the things found in #633

Proposed changes:

  • So, wp_update_comment() doesn't actual return a comment ID.
  • So I made it so that Interactions::update_comment() takes this into account and returns the $commentdata array, but only when the update was successful.
  • If the update somehow failed, Interactions::update_comment() either returns false or 'invalid' (just like before) or whatever is returned by wp_update_comment() (which can be an error, or 0, or even false). Actually, in this case, the function behaves just like it did before.
  • Subsequently, in the "update handler," I replaced \get_comment( $state ) with something that I think makes a bit more sense (as mentioned, $state isn't actually a comment ID).

Other information:

  • Have you written new tests for your changes, if applicable?

Testing instructions:

The way I tested this, is I created a callback in a separate mu-plugin like so:

add_action( 'activitypub_handled_update', function( $activity, $second_param, $state, $reaction ) {
	if ( $reaction instanceof \WP_Comment ) {
		wp_set_comment_status( $reaction, 'hold' );
	}
}, 99, 4 );

Thing is, and this might be another bug but I left it untouched for now, I have no clue what the second param really is.

Prior to this PR, $reaction would always be null or the comment with ID 1 (but, as mentioned, the 1 returned by wp_update_comment() isn't actually a comment ID).

With this PR, $reaction is either a comment object or null. And $state basically the result of the wp_update_comment() call, kind of like before.

Either way, this short callback addresses #633. So if this PR were merged, site admins or developers would be able to much more easily force re-approval of updated comments.

@janboddez
Copy link
Contributor Author

janboddez commented Jan 5, 2024

So the important thing is this change attempts to take into account all the different return types of Interactions::update_comment().

And then sets $state and $reaction (which in the original code would always be null or probably-the-wrong-comment) based on that.

And it's those changes that eventually allow us to use the action hook as (I assume) was intended.

Of course one could opt to have Interactions::update_comment() return something else than the $commentdata (or one of all those other return types when something goes wrong), like either a WP_Error or WP_Comment, but I tried to stay close to the current implementation. :-)

Copy link
Member

@pfefferle pfefferle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; Thanks :)

@pfefferle pfefferle merged commit 6e5cb57 into Automattic:master Jan 5, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants