Since #maxlength can be defined for textfields, form validation should automatically verify that for submitted data. This would be a quick patch to _form_validate in includes/form.inc.

CommentFileSizeAuthor
#6 form.inc_34.patch961 bytesyched
#4 form_maxlength_1.patch1.22 KBChrisKennedy
#2 form_maxlength_0.patch1.16 KBChrisKennedy
#1 form_maxlength.patch1.15 KBChrisKennedy

Comments

ChrisKennedy’s picture

ChrisKennedy commented
Version: 6.x-dev » 5.x-dev
Status: Active » Needs review
StatusFileSize
new form_maxlength.patch1.15 KB

Attached patch implements #maxlength validation.

To test, remove the maxlength attribute from the subject field for comments using the following jquery: '$("#edit-subject").attr("maxlength", "")' then submit a comment with a subject of more than 64 characters.

ChrisKennedy’s picture

ChrisKennedy commented
StatusFileSize
new form_maxlength_0.patch1.16 KB

Updated to use drupal_strlen() per chx's suggestion.

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

This is a very fine idea. The usage of !$name is OK.

ChrisKennedy’s picture

ChrisKennedy commented
StatusFileSize
new form_maxlength_1.patch1.22 KB

Updated not to assume that #title has been set per chx/Heine's suggestion.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD.

yched’s picture

yched commented
Status: Fixed » Needs review
StatusFileSize
new form.inc_34.patch961 bytes

this implementation treats '#maxlength' => '' as maxlength = 0 and rejects any entry

Attached patch adds an is_numeric check.

ChrisKennedy’s picture

ChrisKennedy commented
Status: Needs review » Reviewed & tested by the community

Fair enough.

chx’s picture

chx commented
Status: Reviewed & tested by the community » Needs work

Huh? #maxlength is not user data, I do not see the need. If you want any check then isset( maxlength ) && maxlength

ChrisKennedy’s picture

ChrisKennedy commented

I would prefer not to do a lazy evaluation of maxlength's boolean value and maintain the ability to set it to zero. It is handy to be able to set #maxlength to 0 and have this be enforced - for example it would be a great way to create a trap for automated form spambots (create a hidden URL form element with maxlength=0).

I'm mostly ambivalent towards the is_numeric change. On the one hand #maxlength should be unset() programmatically rather than setting it to an empty string, but on the other hand it is understandable for a programmer to not expect that '' would result in the test always failing.

chx’s picture

chx commented
Status: Needs work » Closed (won't fix)

OK then, if 0 is a desired setting then this is a won't fix: the motto is "we do not babysit broken code". Setting a numeric setting to an empty string is broken code.

yched’s picture

yched commented
Status: Closed (won't fix) » Active

Not babysitting suits me fine.
But then maybe this should be mentioned in the "4.7 to 5.0 module update" page, since this breaks code that was working OK in 4.7.
I'm re-opening the issue for this.

ChrisKennedy’s picture

ChrisKennedy commented
Status: Active » Closed (won't fix)

No need to re-open this issue for a separate task, just post a comment to the 5.x module version page (or edit it if you have access).

yched’s picture

yched commented

Done - thanks

ChrisKennedy’s picture

ChrisKennedy commented
Status: Closed (won't fix) » Needs review

After looking at yched's example I realized that it does make sense to support '', because this is used to initialize variables when seeking E_ALL compliance. I think my "E_ALL compliance during installation" patch does this also. Since 0 is a valid setting it can't be used to initialize maxlength to a neutral value, thus the is_numeric() check is worthwhile (or !== '').

RobRoy’s picture

RobRoy commented

But, shouldn't we just be initializing #maxlength to NULL?

chx’s picture

chx commented
Status: Needs review » Closed (won't fix)

#maxlength is initialized to 128 in system_elements. What are we talking about...?