(Tests for) Ugly message when signature is too long [#399248]

Testing Drupal 7: when the siggie is too long, I get the following ugly message:

PDOException: UPDATE {users} SET name=:db_update_placeholder_0, mail=:db_update_placeholder_1, signature=:db_update_placeholder_2, status=:db_update_placeholder_3, timezone=:db_update_placeholder_4, picture=:db_update_placeholder_5, data=:db_update_placeholder_6 WHERE (uid = :db_condition_placeholder_8) - Array ( [:db_update_placeholder_0] => Dandy [:db_update_placeholder_1] => (emailremoved) [:db_update_placeholder_2] => She was not quite what you would call refined. She was not quite what you would call unrefined. She was the kind of person that keeps a parrot. Mark Twain - Following the Equator <a href="http://africangreyparrot.info">Visit African Grey Parrot Info</a> [:db_update_placeholder_3] => 1 [:db_update_placeholder_4] => America/Dawson_Creek [:db_update_placeholder_5] => 3 [:db_update_placeholder_6] => a:2:{s:5:"block";a:0:{}s:7:"contact";i:1;} [:db_condition_placeholder_8] => 4 ) SQLSTATE[22001]: String data, right truncated: 1406 Data too long for column 'signature' at row 1 in drupal_write_record() (line 3987 of /home/sophia/public_html/d7/includes/common.inc).

Comment	File	Size	Author
#10	399248_add_signature_length_validation_wo_function_v2.patch	1.09 KB	deekayen
#10
#8	399248_add_signature_length_validation_wo_function_fixed.patch	1.06 KB	deekayen
#8
#6	399248_add_signature_length_validation_wo_function.patch	1.05 KB	deekayen
#6
#2	399248_add_signature_length_validation_w_tests.patch	2.7 KB	deekayen
#2
#1	399248_add_signature_length_validation.patch	1.2 KB	deekayen
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

deekayen CreditAttribution: deekayen commented 10 May 2009 at 07:33

Status:

Active

» Needs review

File	Size
399248_add_signature_length_validation.patch	1.2 KB

Patch adds validation to the user submit form to make sure the length of the signature doesn't exceed the size of the signature column in the schema definition (currently 255).

Comment #2

deekayen CreditAttribution: deekayen commented 11 May 2009 at 07:24

File	Size
399248_add_signature_length_validation_w_tests.patch	2.7 KB

Update adds some new tests.

Comment #3

Dries CreditAttribution: Dries commented 11 May 2009 at 07:27

Any reason you created that function user_validate_signature()? It is only used once -- I think it could be rolled into the main validate function.

Comment #4

deekayen CreditAttribution: deekayen commented 11 May 2009 at 07:30

Yes, it could be, but I thought it'd be nice for it to be available to be re-usable. Never know what kinds of crazy needs will pop up in contrib...

Comment #5

11 May 2009 at 10:20

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #6

deekayen CreditAttribution: deekayen commented 11 May 2009 at 16:14

Status:

Needs work

» Needs review

File	Size
399248_add_signature_length_validation_wo_function.patch	1.05 KB

This version rolls the validation into the main user_user_validate() function.

Comment #7

deekayen CreditAttribution: deekayen commented 11 May 2009 at 16:17

Status:

Needs review

» Needs work

Comment #8

deekayen CreditAttribution: deekayen commented 11 May 2009 at 16:21

Status:

Needs work

» Needs review

File	Size
399248_add_signature_length_validation_wo_function_fixed.patch	1.06 KB

same as #6 with a corrected signature variable

Comment #9

catch

he/him

English

CreditAttribution: catch commented 11 May 2009 at 17:16

The two inline comments should be rolled into one above the if statement - we don't usually put comments inline with brackets like that

What about making the signature field a text field and adding maxlength to it instead?

Comment #10

deekayen CreditAttribution: deekayen commented 11 May 2009 at 18:26

File	Size
399248_add_signature_length_validation_wo_function_v2.patch	1.09 KB

Comments adjusted.

I think making it a text field would break more than an insignificant number of signatures that have line breaks. -1

Comment #11

Dries CreditAttribution: Dries commented 12 May 2009 at 09:07

Status:

Needs review

» Fixed

I committed this to CVS HEAD. Thanks deekayen and catch.

We can follow up with a new patch if we want to consider changing it to a textfield. I don't think it would be better either.

Comment #12

Dries CreditAttribution: Dries commented 12 May 2009 at 09:10

Status:

Fixed

» Needs work

Actually, I'm going to set this to 'code needs work' because it would be great to have a test for this.

Comment #13

catch

he/him

English

CreditAttribution: catch commented 12 May 2009 at 09:12

Title:

Ugly message when signature is too long

» (Tests for) Ugly message when signature is too long

textfield wasn't a good idea, forgot about line breaks. Maybe I just don't like signatures...

Comment #14

Damien Tournoud CreditAttribution: Damien Tournoud commented 12 May 2009 at 09:18

Two questions about that patch:

- why don't we change the users.signature column type to 'text'?
- why don't we make textarea accept #maxlength too (it could be useful elsewhere), instead of adding cruft into the validation handler of the form itself?

Comment #15

deekayen CreditAttribution: deekayen commented 12 May 2009 at 13:37

I like where Damien's thinking is wandering to. I don't think an unrestricted text field is good because people will abuse it with pages of signature URLs, job titles, and spam. A new #maxlength would just substr() or would it drupal_set_error? Then I assume that will eventually grow into adding a Javascripty limit assist?