Problem/Motivation
This issue became evident after fixing #1015916: Image field "title" allows more data than database column size..
If there is a textfield with a maxlength of say 128, it's easy for a user to paste in text and not realize that the browser is silently trimming off the end of the text pasted into the form element. The user hits save, and is surprised that what was entered is not displayed on the page. (The user may check the database, and also be surprised that what was entered didn't make it into the db either.)
Wouldn't it be better if we were able to display a message letting the user know that the text entered was longer than what is allowed?
The #maxlength property is really valuable when form elements are short, for example a field for year may only be 4 characters long. But when we start using longer textfields, the user shouldn't be expected to know the length of the field. Let's use validation to let users know if they do something wrong, rather than assuming they would know that a specific field is 128, 512, or 1024 characters in length.
Proposed resolution
In theme_textfield() I propose that we remove the #maxlength HTML property from form elements where maxlength is greater than 128. Form validation will still protect users from entering values that are too long.
Remaining tasks
- update theme_textfield() to remove maxlength when it's so large it's not useful in the HTML anymore.
User interface changes
- none
API changes
- none
Comment | File | Size | Author |
---|---|---|---|
#26 | 1360466-26.patch | 757 bytes | Tebro |
#18 | remove_maxlength_when_long-1360466-16.patch | 610 bytes | shumer |
#16 | remove_maxlength_when_long-1360466-15.patch | 610 bytes | shumer |
#1 | remove_maxlength_when_long-1360466-1.patch | 623 bytes | jenlampton |
#1 | remove_maxlength_when_long-1360466-1-D7.patch | 588 bytes | jenlampton |
Comments
Comment #1
jenlamptonpatches for review.
Comment #2
droplet CreditAttribution: droplet commentedThere is no limits for maxlength in W3C, fixed limit in Drupal is broken its flexibility.
http://www.w3.org/TR/html4/interact/forms.html#adef-maxlength
for example, Google input's maxlength is 2048.
Comment #3
rbayliss CreditAttribution: rbayliss commentedThis patch breaks the machine_name element's ability to auto-truncate if the #maxlength is more than 128 characters. I can't see that being a common problem, but it is kind of confusing. Otherwise, looks good.
Comment #4
jenlampton@droplet - I think we can do better than google, or W3C. If we care about usability, notice a problem, and fix it, I dont see anything wrong with that :) Why no D7 backport?
@rbayliss - We could make an exception for machine name, somehow... if you think the user doesn't need to be notified about that field. I think you're right though, since we're not asking them for input on that field, we should just allow the browser to trim it down silently. I'll see if I can work on the exception.
Comment #5
BTMash CreditAttribution: BTMash commentedConceptually, I am fond of the idea. Moving away from maxlengths would be great as you mention, copy/pasting text it and providing the user that there might be an error in the amount of data that is trying to get pasted makes sense. However, the big issue I see in all of this is with the flexibility of the fields. Right now, various types of fields (specifically, the same image fields from the issue you posted) have hardcoded values for the length of the fields and it is a pain to alter the schema for those fields. But lets say in the future (well...hopefully soon future if #691932: Add hook_field_schema_alter() can get resolved), fields can have their schemas altered. We need a way for other modules that are implementing the alters to also be able to alter the validation hooks on a field (the situation is messed up due to http://api.drupal.org/api/drupal/modules--field--field.api.php/function/... / http://api.drupal.org/api/drupal/core--modules--field--field.api.php/fun... as it effectively means the values can be hardcoded in there and if your schema alters the length to be longer in the future, then it will still not validate).
Comment #6
jenlampton@BTMash
I'm not sure I see how your concern applies to this patch. Altering the schema won't automatically affect a form element, or a validation handler, or the HTML.
If someone wanted to increase the length for an image's title field, for example, they woud need to both do a hook_schema_alter as well as a hook_form_alter. The schema alter wouldn't do any good on its own, the dev would also need to alter the form element to increase the maxlentgh there, too. The form's validation handler would still check the value a user entered against the form element's maxlength (just like it should, and does now). The only difference here, is that the user's browser won't truncate the value in that form field before it reaches validation (causing confusion for the user).
All this is, is a change to the theme function; a change to the HTML that represents that form element. #maxlength is still present on the form element itself.
Comment #7
jenlamptonAh, maybe the title was misleading :)
Comment #8
BTMash CreditAttribution: BTMash commentedI think we're both on the same page on removing the maxlength html attribute. But fields that currently have a maxlength will need to implement some form of hook_field_validate to ensure the text can fit into the length for that field (the image field would need to implement this hook to ensure data can safely fit into the field). Text fields currently implement this but images do not. So the patch needs to expand on any such fields in core to account for this (or we'll end up with PDOexceptions).
What I was also trying to say in this, however, is we need to figure out a flexible way to ensure that hook_field_validate for these fields respects any alterations other modules may do to the field schema. Right now, if we take a look at structure of the field element, it actually doesn't list the set of functions invoked via hook_field_validate that will be called for validation. Therefore, hook_field_validate is run by default for the field if it exists. So even if we had a module that alters the field lengths to something else, it may fail the validation on what is supposed to be valid data.
Comment #9
BTMash CreditAttribution: BTMash commentedI didn't realize that this was *only* for text fields (I should have paid close attention to the part where it says
theme_textfield
. The patch seems sound since the text field does perform its own validation. Looking through the text tests, we might need a test to ensure saving content will call on that correctly.Comment #10
jenlampton@rbayliss re #3: This patch only affects textfields, and the machine name field is still silently truncated correctly by my browser. Can you tell me how I can reproduce the problem?
Comment #11
jenlamptonDoh.
Comment #12
rbayliss CreditAttribution: rbayliss commented@jenlampton: I used something like:
The key being that the maxlength for the machine name has to be 128 or longer. Like I said, the chances that a machine name will ever be this long are pretty slim.
Comment #13
xjmHmmm, this issue does not really make sense to me. I understand the usability concern @jenlampton describes, but isn't this sort of throwing the baby out with the bathwater? It seems very arbitrary to unset
#maxlength
silently at a certain length; that seems like much more of a usability concern than folks pasting long values in a field in their browsers.As droplet alludes to in #2, I think it is up to the user agent to determine the correct behavior in this situation, not to Drupal.
Comment #15
monil-dupe CreditAttribution: monil-dupe commentedI can't install patches too :(
Comment #16
shumer CreditAttribution: shumer commentedPatch for review
Comment #18
shumer CreditAttribution: shumer commentedPatch for review
Comment #19
kalanh CreditAttribution: kalanh commented@DrupalCon Austin Sprint
Comment #20
jlyon CreditAttribution: jlyon commentedReviewed and tested.
Comment #21
kalanh CreditAttribution: kalanh commentedPatch applied cleanly, agree with jlyon.
Comment #22
catchI agree with xjm's comment in #13, not sure why 128 is a more sensible cut-off point than any other length. Tagging 'needs usability review'.
Comment #25
Tebro CreditAttribution: Tebro commentedRedid this change on 8.0.x as a re-roll wasn't possible after a year.
Edit: Not going to take a stand on the discussion if this is the right thing to do.
Comment #26
Tebro CreditAttribution: Tebro commentedMoved code to correct place in function.
Comment #27
Patrick Storey CreditAttribution: Patrick Storey at Hook 42 commentedI am removing the Novice tag from this issue until there is a consensus on whether this is something Drupal should be handling or not as @catch, @xjm, and @droplet brought up concerns.
I’m using this documentation as a source: https://www.drupal.org/core-mentoring/novice-tasks#avoid
Comment #28
Bojhan CreditAttribution: Bojhan as a volunteer commentedIsn't Drupals lack of inline validation, the reason this issue occurs? Won't fixing this, does not solve the user problem. But the user problem is created by the browser doing magic, without explaining it.
Comment #29
Patrick Storey CreditAttribution: Patrick Storey at Hook 42 commentedDoh! Looks like I didn't remove the tag in comment #27. Remedying that now.
Comment #30
Bojhan CreditAttribution: Bojhan as a volunteer commentedComment #34
gambrySo I'm assuming this can be closed as 3 between Release, Framework and Usability Managers flagged this issue creates more problems than the ones trying to fix.