Skip to content

Comment content can be string with zero#221

Closed
cawa-93 wants to merge 2 commits intoWordPress:masterfrom
cawa-93:patch-1
Closed

Comment content can be string with zero#221
cawa-93 wants to merge 2 commits intoWordPress:masterfrom
cawa-93:patch-1

Conversation

@cawa-93
Copy link
Copy Markdown

@cawa-93 cawa-93 commented Apr 14, 2020

Trac ticket: https://core.trac.wordpress.org/ticket/49903#ticket


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@aduth
Copy link
Copy Markdown
Member

aduth commented Apr 14, 2020

empty may have been anticipating one or both of not set and empty string. Personally, I think rather than handling the exceptions, it could be framed as more targeted to the specific anticipated problem values.

if ( ! isset( $prepared_comment['comment_content'] ) || $prepared_comment['comment_content'] === '' ) {

Or, if comment_count is always set, and doesn't need to be checked:

if ( $prepared_comment['comment_content'] === '' ) {

@TimothyBJacobs
Copy link
Copy Markdown
Member

Yeah, I think it would be better to do an isset check and looking for an empty string exactly. That matches wp_handle_comment_submission.

@cawa-93
Copy link
Copy Markdown
Author

cawa-93 commented Apr 26, 2020

@TimothyBJacobs I agree with you. I didn't think about matching the behavior in the wp_handle_comment_submission. Then, perhaps, should the same hook be added to allow empty comments?

$allow_empty_comment = apply_filters( 'rest_allow_empty_comment', false, $prepared_comment );
$missing_content = ! isset( $prepared_comment['comment_content'] ) || '' === $prepared_comment['comment_content']
	
if ( $missing_content && ! $allow_empty_comment ) {
	return new WP_Error(
		'rest_comment_content_invalid',
		__( 'Invalid comment content.' ),
		array( 'status' => 400 )
	);
}

@TimothyBJacobs
Copy link
Copy Markdown
Member

Yeah I think it'd be great to make sure we realign ourselves with wp_handle_comment_submission.

@TimothyBJacobs
Copy link
Copy Markdown
Member

Another related ticket: https://core.trac.wordpress.org/ticket/43177

@TimothyBJacobs
Copy link
Copy Markdown
Member

Fixed in 1e030c4.

@cawa-93 cawa-93 deleted the patch-1 branch October 25, 2020 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants