WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#19988 closed enhancement (duplicate)

wp-comments-post ineffciency

Reported by: allarem Owned by:
Milestone: Priority: normal
Severity: minor Version: 3.3.1
Component: Comments Keywords: has-patch
Focuses: 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 2 years ago.

Download all attachments as: .zip

Change History (7)

comment:1 follow-up: ocean902 years 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: allarem2 years 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 SergeyBiryukov2 years 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.

comment:4 SergeyBiryukov2 years ago

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).

comment:5 nacin2 years ago

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.

comment:6 nacin2 years ago

  • 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.