Make WordPress Core

Opened 15 years ago

Closed 9 years ago

Last modified 9 years ago

#10377 closed defect (bug) (fixed)

Comment fields should have max lengths

Reported by: muriloazevedo's profile muriloazevedo Owned by: rachelbaker's profile rachelbaker
Milestone: 4.5 Priority: normal
Severity: normal Version: 2.8
Component: Comments Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

Hello, I don't know if exactly it's a bug or use javascript validation, but i think so there's no
treatment for this, or was fix in the new version.

In the section of comments(Wordpress 2.8), we can insert how many characheters we wants, generating an SQL Exception and breaking the all system.

The solution is simple, use the property maxlenght in the tag
<input /> e limit the characters if will be send to database.

Attachments (8)

10377.patch (4.5 KB) - added by westonruter 11 years ago.
Add maxlength to comment textarea and size check in wp-comments-post.php (extends patch from #10377)
10377.2.patch (1.9 KB) - added by rachelbaker 9 years ago.
Refreshed patch
10377.3.patch (3.0 KB) - added by rachelbaker 9 years ago.
10377.4.diff (7.1 KB) - added by rachelbaker 9 years ago.
with new function wp_get_comment_column_max_length() and unit tests
10377.5.diff (8.9 KB) - added by rachelbaker 9 years ago.
Refresh of 10377.4 with maxlength for all comment template fields and mb_strlen checks where needed
10377.6.diff (1.4 KB) - added by rachelbaker 9 years ago.
Fallback to TEXT column with 65525 length and add unit tests
10377.7.diff (4.5 KB) - added by azaozz 9 years ago.
10377.8.diff (5.3 KB) - added by azaozz 9 years ago.

Download all attachments as: .zip

Change History (51)

#1 follow-up: @dd32
15 years ago

  • Milestone changed from Unassigned to Future Release

The solution is simple, use the property maxlenght in the tag <input /> e limit the characters if will be send to database.

That doesnt stop people from directly posting the data anyway..

The comment_content field is a TEXT field:

BLOB 
TEXT 
A BLOB or TEXT column with a maximum length of 65535 (2^16 - 1) characters. See section 7.7.1 Silent column specification changes. 
MEDIUMBLOB 
MEDIUMTEXT 
A BLOB or TEXT column with a maximum length of 16777215 (2^24 - 1) characters. See section 7.7.1 Silent column specification changes. 
LONGBLOB 
LONGTEXT 
A BLOB or TEXT column with a maximum length of 4294967295 (2^32 - 1) characters. See section 7.7.1 Silent column specification changes.

so theres a limit of 65,535 characters in a comment at present.. Perhaps the comment handler should throw a error upon a longer comment.

#2 @dd32
15 years ago

also note, That MySQL should actually truncate the data if its longer, not throw an error..

#3 in reply to: ↑ 1 @azaozz
15 years ago

Replying to dd32:

... so there's a limit of 65,535 characters in a comment at present.. Perhaps the comment handler should throw a error upon a longer comment.

There's some basic back-end validation already (duplicate comments, non-empty name and email, etc.). Adding max length for all comment fields should be trivial.

Actually this may be good for most POST requests in the admin including AJAX.

#4 @mrmist
14 years ago

  • Summary changed from Problem in comments! to Comment fields should have max lengths

#5 @n3k4
12 years ago

  • Cc hellokane@… added

@westonruter
11 years ago

Add maxlength to comment textarea and size check in wp-comments-post.php (extends patch from #10377)

#6 @westonruter
11 years ago

  • Cc weston@… added
  • Keywords has-patch added; 'sql exception' comments removed

As noted in the attachment comment, in addition to the $comment_content size check in wp-comments-post.php, I've also patched the comment form to add a maxlength to the textarea itself (this extends a patch from #4332). Patch attached, and branch also pushed to GitHub fork: https://github.com/x-team/WordPress/compare/10377-comment-fields-should-have-max-lengths

#7 @chriscct7
9 years ago

  • Keywords needs-refresh added

@rachelbaker
9 years ago

Refreshed patch

#8 @rachelbaker
9 years ago

  • Keywords needs-refresh removed
  • Milestone changed from Future Release to 4.4
  • Owner set to rachelbaker
  • Status changed from new to accepted

#9 @rachelbaker
9 years ago

  • Keywords commit added

Moving for commit consideration.

#10 @wonderboymusic
9 years ago

is this value conducive to utf8mb4? /cc @pento

#11 @pento
9 years ago

Yep, that's fine. comment_content is a text field, which stores 65535 bytes. strlen() also counts the number of bytes, so we're being consistent.

#12 follow-up: @SergeyBiryukov
9 years ago

Instead of a hardcoded value, seems like we should use $wpdb->get_col_length() to get the actual value, in case someone modified the DB schema for a particular install.

See upgrade_430_fix_comments() for reference.

#13 in reply to: ↑ 12 ; follow-up: @chriscct7
9 years ago

Replying to SergeyBiryukov:

Instead of a hardcoded value, seems like we should use $wpdb->get_col_length() to get the actual value, in case someone modified the DB schema for a particular install.

See upgrade_430_fix_comments() for reference.

I don't know if that's worth the extra db call. Someone could just as easily use the filter to modify it if that's the case

#14 in reply to: ↑ 13 @pento
9 years ago

Replying to chriscct7:

I don't know if that's worth the extra db call. Someone could just as easily use the filter to modify it if that's the case

It's not an extra call - the same call will be done later when the data is actually being written to the DB.

I agree with @SergeyBiryukov, $wpdb->get_col_length() is a better way of making sure it's the right length.

#15 @rachelbaker
9 years ago

  • Keywords needs-refresh added

Thank you all for the feedback and direction. Updated patch forthcoming.

#16 @rachelbaker
9 years ago

  • Keywords commit removed

@rachelbaker
9 years ago

#17 @rachelbaker
9 years ago

  • Keywords dev-feedback added; needs-refresh removed

I made an attempt at using $wpdb->get_col_length() in the new function wp_get_comment_content_max_length() (isn't that a mouthful).

#18 follow-up: @SergeyBiryukov
9 years ago

The maxlength attribute is probably redundant (and would cause an extra query on front-end), I think just checking the length in wp-comments-post.php would be enough.

#19 in reply to: ↑ 18 @rachelbaker
9 years ago

Replying to SergeyBiryukov:

The maxlength attribute is probably redundant (and would cause an extra query on front-end), I think just checking the length in wp-comments-post.php would be enough.

The issue I see there is that if a commenter was to leave a 65525+ comment they wouldn't know it was too long until they press submit. Including "max-length" in the comment form template would at least give the commenter feedback that they have reached some sort of limit when they try to type the 65526 character.

#20 @azaozz
9 years ago

Don't think there's a problem to have hard-coded maxlength. 65k characters is a lot of text, about 11,000 words or 44 printed pages :)

We already have a filter 'comment_form_default_fields'. Can add an array with hard-coded lengths for the (very rare) cases when somebody wants comments longer than that. Then run the same filter on checking the POST lengths. Or maybe have a function similar to wp_get_comment_content_max_length() that will filter and return the hard-coded values.

Also: what about maxlength and strlen() for the other comment fields? If we are adding that for the textarea would be good to add everywhere.

Last edited 9 years ago by azaozz (previous) (diff)

This ticket was mentioned in Slack in #core by dd32. View the logs.


9 years ago

#22 @rachelbaker
9 years ago

  • Keywords needs-refresh added; dev-feedback removed

#23 @rachelbaker
9 years ago

  • Milestone changed from 4.4 to Future Release
  • Owner rachelbaker deleted
  • Status changed from accepted to assigned

Punting out of 4.4

#24 @rachelbaker
9 years ago

  • Milestone changed from Future Release to 4.5

@rachelbaker
9 years ago

with new function wp_get_comment_column_max_length() and unit tests

#25 follow-up: @rachelbaker
9 years ago

  • Keywords has-unit-tests dev-feedback added; needs-refresh removed

In 10377.4.diff:

  • hardcoded maxlength attribute on the comment_field, the markup here can be modified via the comment_form_defaults filter (1)
  • added wp_get_comment_column_max_length() which returns the max column length for a given column name, and is filterable (2)
  • added logic in wp_handle_comment_submission() to return a WP_Error when the comment_author, comment_author_url, or comment_content values exceed the max length of their columns
  • added unit tests for the error conditions in wp_handle_comment_submission()
  1. We could add hardcoded attributes like this to the two other comment form inputs. Open to second opinions here.
  2. Is there a good way to convert characters to bytes?

#26 in reply to: ↑ 25 @azaozz
9 years ago

Replying to rachelbaker:

10377.4.diff is starting to look good.

  1. We could add hardcoded attributes like this to the two other comment form inputs.

Thinking this is a must. Having maxlength on the other three ( Name, Email, Website ) text fields will improve UX if these limits are reached.

  1. Is there a good way to convert characters to bytes?

Nothing that is 100% compatible/reliable afaik. If you want to check the byte length of a string, mb_strlen( $string, '8bit' ) should work well. That's the proper way to test it when the "real" mb_strlen() exists. If it doesn't exist, the compat function will return strlen( $string ).

Also this looks like a typo:

} elseif ( is_array( $col_length ) && isset( $col_length['length'] ) && (int) $col_length > 0 ) {

should probably be:

} elseif ( is_array( $col_length ) && isset( $col_length['length'] ) && intval( $col_length['length'] ) > 0 ) {
Version 1, edited 9 years ago by azaozz (previous) (next) (diff)

@rachelbaker
9 years ago

Refresh of 10377.4 with maxlength for all comment template fields and mb_strlen checks where needed

#27 follow-up: @rachelbaker
9 years ago

@azaozz Can you take a look at 10377.5.diff when you have time? I corrected my conditional, added maxlength checks for all comments-template fields, and compare the byte length on tinytext and text columns.

#28 in reply to: ↑ 27 @azaozz
9 years ago

Replying to rachelbaker:

10377.5.diff looks great :)

#29 @rachelbaker
9 years ago

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

In 36272:

Comments: Restrict the maximum characters for input fields within the comments template.

Added hardcoded maxlength attributes on the author, author_email, author_url, and comment_field input markup. These can be modified via the comment_form_defaults filter. Added logic in wp_handle_comment_submission() to return a WP_Error when the comment_author, comment_author_url, or comment_content values exceed the max length of their columns. Introduces wp_get_comment_column_max_length() which returns the max column length for a given column name, and is filterable. Unit tests included for the error conditions in wp_handle_comment_submission()

Fixes #10377.

Props westonruter rachelbaker.

#30 @ocean90
9 years ago

  • Keywords needs-patch needs-unit-tests added; has-patch has-unit-tests dev-feedback removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

[36272] broke comments on w.org because $wpdb->get_col_length() is returning false and falls back to 255. Looking at upgrade_430_fix_comments() the fallback should be 65535.

Not sure yet why it returns false, but the method returns also false in case $wpdb->is_mysql is false.

Edit: $wpdb->is_mysql is false via HyperDB.

Last edited 9 years ago by ocean90 (previous) (diff)

@rachelbaker
9 years ago

Fallback to TEXT column with 65525 length and add unit tests

#31 @rachelbaker
9 years ago

@ocean90 In 10377.6.diff I changed the default fallback to be 65525, 65535 - 10. This matches the logic in upgrade_430_fix_comments(). Unit tests for each text-based column type for the comments table included in test_wp_get_comment_column_max_length().

#32 @rachelbaker
9 years ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

This ticket was mentioned in Slack in #core by ocean90. View the logs.


9 years ago

#34 @rachelbaker
9 years ago

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

In 36325:

Comments: Use TEXT column type in fallback for wp_get_comment_column_max_length().

Fixes #10377.

@azaozz
9 years ago

#35 @azaozz
9 years ago

broke comments on w.org because $wpdb->get_col_length() is returning false and falls back to 255. Looking at upgrade_430_fix_comments() the fallback should be 65535.
...the method returns also false in case $wpdb->is_mysql is false.

Not sure if the default lengths for email, name, URL should be 65525. We should be using "sane" values :)

How about we go back to the idea of having an array with the default lengths and override it with the actual lengths from the DB (if we can get them)? Something like 10377.7.diff (needs to fix the unit tests).

#36 @rachelbaker
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@azaozz 10377.7.diff makes more sense. Reopened this.

@azaozz
9 years ago

#37 @azaozz
9 years ago

In 10377.8.diff:

  • Don't check for type when the column length is not an array.
  • Remove unneeded test. Now we always have lengths.

#38 @DrewAPicture
9 years ago

In 36499:

Docs: Add a changelog entry for the introduction of maxlength character limits for the 'author', 'email', and 'url' fields in comment_form().

Introduced in [36272].

See #10377. See #32246.

#39 @rachelbaker
9 years ago

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

In 36514:

Comments: Change wp_get_comment_column_max_length() function to wp_get_comment_fields_max_lengths() for consolidation and better fallbacks.

Instead of returning a value for each of the related table column lengths, return an array of all of the column lengths used in the comment form.
Better fallback handling, where each field falls back to the expected max_length instead of an arbitrary number.

Props azaozz.

Fixes #10377.

#40 @rachelbaker
9 years ago

In 36515:

Comments: Unit test for `wp_get_comment_fields_max_lengths().

This adds tests for the comment form field lengths returned by wp_get_comment_fields_max_lengths(). Replaces unit test removed in r36514.

See #10377.

#41 @azaozz
9 years ago

In 36542:

Comments: look for wp_error when checking whether $wpdb->get_col_length() has failed.

See #10377.

#42 @DrewAPicture
9 years ago

In 36920:

Docs: Improve the DocBlock summary for wp_get_comment_fields_max_lengths(), introduced in [36514].

See #10377. See #35986.

#43 @DrewAPicture
9 years ago

In 36921:

Docs: Improve syntax for the $lengths parameter in the hook doc for the wp_get_comment_fields_max_lengths filter, introduced in [36272].

See #10377. See #35986.

Note: See TracTickets for help on using tickets.