WordPress.org

Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#7890 closed defect (bug) (fixed)

Internal server error when submitting comment without name or email

Reported by: gadlen Owned by: Libor Jelinek
Milestone: 2.7 Priority: normal
Severity: normal Version: 2.6.1
Component: Comments Keywords: 2nd-opinion dev-feedback has-patch commit
Focuses: Cc:
PR Number:

Description

In WP 2.6.2, running Internet Explorer 6
If you submit a comment without a name or email address, Internet Explorer returns an error. This does not happen in Firefox 3 or Chrome. The error returned is:

http://lee.org/blog/wp-comments-post.php
The page cannot be displayed 
There is a problem with the page you are trying to reach and it cannot be displayed. 
HTTP 500 - Internal server error 
Internet Explorer

hotkee suggests "The bug is IE related I think it treats it as internal error if the returned page is less 512Bytes."

That appears to be correct :-). If the error messages are made longer, the Server Error messages don't appear.

Here are some proposed changes:

line 61 of wp-comments-post.php was:

wp_die( ('Error: please fill the required fields (name, email).') );

change to:

wp_die( ('Sorry, please click the Back button on your browser and fill the required fields (name, email).') );

change line 63 to:

wp_die( ('Sorry, please click the Back button on your browser and enter a valid email address.') );

change line 67 to:

wp_die( ('Sorry, please click the Back button on your browser and enter a comment.') );

change line 54 to:

wp_die( ('Sorry, you must be logged in to post a comment. Please click the Back button on your browser.') );

Attachments (1)

7890.diff (402 bytes) - added by DD32 11 years ago.

Download all attachments as: .zip

Change History (20)

#1 @Libor Jelinek
11 years ago

  • Cc Libor Jelinek added
  • Component changed from General to Comments
  • Owner changed from anonymous to Libor Jelinek
  • Status changed from new to assigned

I highly recommend to change HTTP return code from 500 to 200.

All IE browsers have "friendly HTTP errors" turn on by default and users see only unmeaning typical "Page cannot be displayed" page instead of meaningful warning that they missed to fill all commend form fields...

#2 follow-up: @DD32
11 years ago

  • Keywords hsa-patch added
  • Milestone changed from 2.8 to 2.7

Perhaps instead of making the error messages longer, wp_die() should ensure the document served is > 512Bytes.. See patch, Doesnt take into consideration the rest of the document size, but best to be over than under?

@DD32
11 years ago

#3 @mrmist
11 years ago

See also #7246 which is partially related, insofar as these sort of pages should not be returning 5xx errors at all.

#4 in reply to: ↑ 2 @Libor Jelinek
11 years ago

  • Keywords hsa-patch removed

Upon my testing I can confirm that IE:

Status 500 AND response size <= 512 B = body of response is ignored, IE show default "The website cannot display the page"

Status 500 AND response size > 512 B = body of reponse is echoed to user


Code to simulate HTTP 500 in PHP:

<?php header('HTTP/1.1 500 Internal Server Error'); ?>

#5 follow-up: @Libor Jelinek
11 years ago

  • Keywords 2nd-opinion dev-feedback added

What is your opinion, developers? Send HTTP 500 but > 512 B or send HTTP 200?

I personaly thing that much safer and easier is simply to send normal HTTP 200 status code.

#6 in reply to: ↑ 5 @Libor Jelinek
11 years ago

Because we should primary inform a user, not a browser about some missing filled comment form fields.

#7 @DD32
11 years ago

IMO: send >512 bytes to override IE's stupid functionality regardless. 200 isnt a correct responce for an error message regardless.

#8 @jacobsantos
11 years ago

What we do on the sites I work on, is just put text in a comment (which won't be displayed to the user unless they view the source.

I say, put the same text in a comment for a total of 512 bytes.

#9 @112502
11 years ago

DD32: 200 isn't a correct response for an error message?

Uhh, no offense, but...

The problem is that 500 and 200 are httpd status messages. They have nothing whatsoever to do with Wordpress, excepting the case where a page cannot be served for whatever reason. In this situation, httpd is serving the Wordpress files correctly, therefore there should be no error message.

What we're talking about here is a Client Input Validation error, that has nothing to do with httpd Error Codes. If this was a 404 condition or the like, I could see your point, but definitely not in this case.

A 200 condition is the only logical outcome.

Again, I don't intend it as a flame. It's informational, and it's the way things have been for over fifteen years. Please don't go all crazy trying to change the logic behind status codes. ;-)

#10 @112502
11 years ago

Temporary Solution: drop wp_head() into it.

Long Term Suggestion: handle this issue with a more traditional method of form validation instead of serving up ugly, out-of-character messages from wp-admin.

#11 follow-up: @DD32
11 years ago

DD32: 200 isn't a correct response for an error message?
Uhh, no offense, but...

My point was, That the HTTP status explains to the browser OR to the crawler that the request was not 100% correct. A 404 is sent for pages which do not exist AND a helpful message is sent for the human, A 403 is sent for authentication failures AND a helpful message is sent for humans.

A 200 "All OK" response with a error message is OK for a human but you're telling the browser, this request has completed successfully, no problems, Now that might not seem like a great problem, and it isnt, Its just returning the correct status, for the correct operations.

How does a 404 error differ from a input validation error? They're both exactly the same thing, The client has inputted an incorrect piece of data, whether its come from a form, or the URL doesnt matter.

The much better way would be for the theme to handle this stuff correctly, ie. redirect back to commenting form with the form pre-filled with the right details, and a note to fill in the email/name.

#12 @112502
11 years ago

You're correct, however, you're missing that there is no httpd status message to correspond to a Field Validation Error, thus 200 is the correct response.

About the closest you could come to an "informative" httpd status code is a 400 error... but then...

You're redirecting back to the offending page (as suggested, by using traditional form validation), and you don't want the error following you --- Especially consider that in this case, where you're doing a POST %{SELF} it doesn't make any sense to attach an httpd status code to a page which "is" displaying correctly --- even though there's a form validation error.

Hopefully, that makes sense... Written communication doesn't seem to be my strong point today. ;-)

#13 in reply to: ↑ 11 @Libor Jelinek
11 years ago

Hi DD32,
I can't imagine how your idea how theme can handle form validation errors? Can you shortly figure out me your idea?

Me, I am voting for return 200 :-)

Replying to DD32:

The much better way would be for the theme to handle this stuff correctly, ie. redirect back to commenting form with the form pre-filled with the right details, and a note to fill in the email/name.

#14 @Libor Jelinek
11 years ago

Help me friends, I am new at WP contributing.

Who will decide what to do with this issue?

a) return 200

b) return 500 but make error HTML message longer > 512 kB to prevent IE from displaying "page cannot be displayed"?

c) (worst) keep current 500 displaying IE's "page cannot be displayed"?

Because we are only talking what or what not for more then month but I would like to have some result and then I will send solution in a diff patch.

#15 follow-up: @DD32
11 years ago

  • Keywords has-patch commit added

my vote: Keep it as 500, but return a >512 byte error. a 500char comment on the wp_die() page would suffice, The attached patch of mine works, and only adds the ammount of spaces needed, but theres no reason it couldnt be simplified to just dump 500 spaces onto the page.

Its too late to change the error HTTP status type, but something needs to happen to prevent IE being IE, Regardless of your(refering to all readers), the wp_die() function needs to support sending of 500 errors (Primarily due to 200 errors being cached by search engines and the alike, which was the original intention of the 500 error IIRC) as thats what its been doing.

#16 in reply to: ↑ 15 @westi
11 years ago

Replying to DD32:

my vote: Keep it as 500, but return a >512 byte error. a 500char comment on the wp_die() page would suffice, The attached patch of mine works, and only adds the ammount of spaces needed, but theres no reason it couldnt be simplified to just dump 500 spaces onto the page.

That is a horrible solution albeit one that MS recommends - http://support.microsoft.com/kb/294807

At present the wp_die page on trunk for this error is 529 bytes based on my count with wc -c

So is this change still needed?

If so I think we can come up with a cleaner solution which gets us over the limit - i.e. add some more markup to the page.

#17 @DD32
11 years ago

At present the wp_die page on trunk for this error is 529 bytes based on my count with wc -c

I do agree that its ugly, And yes, It seems that its just scraping over the 512byte limit under current trunk. As long as the message and the site URL are >50char combined, theres no problem. (It looks like the Comment error's were lengthened at one stage)

Adding an extra line of markup somewhere would probably be cleaner.. are there any no-cache meta tags that can be added perhaps?

#18 @112502
11 years ago

Keep in mind the DTD doesn't count towards the 512 bytes. It's 512 bytes from the HTML tag, so if memory serves, it's 112 bytes short.

#19 @markjaquith
11 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [10105]) Pad wp_die() error messages to be over 512 chars, so IE always displays our text. props DD32. fixes #7890

Note: See TracTickets for help on using tickets.