#19988 closed enhancement (duplicate)

wp-comments-post ineffciency

Reported by: allarem Owned by:
Priority: normal Milestone:
Component: Comments Version: 3.3.1
Severity: minor Keywords: has-patch
Cc:

Description

In wp-comments-post we use "isset" to check every thing in $_POST and needs to check twice whether it's the email format or etc.
I think we should use filter_var to do such jobs to save some memory, therefore, this patch is born.

Index: wp-comments-post.php
===================================================================
--- wp-comments-post.php	(版本 19862)
+++ wp-comments-post.php	(工作副本)
@@ -47,10 +47,10 @@
 	do_action('pre_comment_on_post', $comment_post_ID);
 }
 
-$comment_author       = ( isset($_POST['author']) )  ? trim(strip_tags($_POST['author'])) : null;
-$comment_author_email = ( isset($_POST['email']) )   ? trim($_POST['email']) : null;
-$comment_author_url   = ( isset($_POST['url']) )     ? trim($_POST['url']) : null;
-$comment_content      = ( isset($_POST['comment']) ) ? trim($_POST['comment']) : null;
+$comment_author       = filter_var($_POST['author'],FILTER_SANITIZE_STRING);
+$comment_author_email = filter_var($_POST['email'],FILTER_VALIDATE_EMAIL);
+$comment_author_url   = filter_var($_POST['url'],FILTER_VALIDATE_URL);
+$comment_content      = filter_var($_POST['comment'],FILTER_SANITIZE_SPECIAL_CHARS);
 
 // If the user is logged in
 $user = wp_get_current_user();
@@ -74,9 +74,9 @@
 $comment_type = '';
 
 if ( get_option('require_name_email') && !$user->ID ) {
-	if ( 6 > strlen($comment_author_email) || '' == $comment_author )
+	if ( 6 > strlen($comment_author_email) || !$comment_author )
 		wp_die( __('<strong>ERROR</strong>: please fill the required fields (name, email).') );
-	elseif ( !is_email($comment_author_email))
+	elseif ( !$comment_author_email )
 		wp_die( __('<strong>ERROR</strong>: please enter a valid email address.') );
 }
 
@@ -97,3 +97,4 @@
 
 wp_redirect($location);
 exit;
+?>

wp-comments-post.php.patch

Attachments (1)

wp-comments-post.php.patch (1.5 KB) - added by allarem 16 months ago.

Download all attachments as: .zip

Change History (7)

comment:1 follow-up: ↓ 2   ocean9016 months ago

Please don't re-add the closing PHP tag.

Related to email check: #17433

I think we should use filter_var to do such jobs to save some memory

Any stats?

comment:2 in reply to: ↑ 1 ; follow-up: ↓ 3   allarem16 months ago

Replying to ocean90:

Please don't re-add the closing PHP tag.

hmm, I can't find that closing PHP tag in my svn repo(revision 19862)..

$location = apply_filters('comment_post_redirect', $location, $comment);
wp_redirect($location);
exit;
EOF

Related to email check: #17433

I think we should use filter_var to do such jobs to save some memory

Any stats?

here is the result of

var_dump(memory_get_usage());

old:
int(21705640)
new:
int(21700664)

there is about 5KB less.

comment:3 in reply to: ↑ 2   SergeyBiryukov16 months ago

Replying to allarem:

hmm, I can't find that closing PHP tag in my svn repo(revision 19862)..

It was intentionally removed in #12307.

Related: #16867

As far as I can see, we don't use filter_var() anywhere in core yet, with one exception being PHPMailer (with functions_exists() check). I wonder if some odd hosts may disable Filter extension, like JSON (#18015).

Correct, we don't yet use filter_var() anywhere, and I don't think it is worth considering for 5KB of memory usage. We've thought about using it for validation and sanitization of emails, URLs, etc., to replace our own regular expressions and what not, but that seems outside the scope of this ticket. Safe to close as a duplicate of #16867.

  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from new to closed

It is indeed possible to compile PHP with --disable-filter. We could provide compatibility pretty easily, but it'll need to be something we're willing to support across WordPress.

Note: See TracTickets for help on using tickets.