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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jenlampton’s picture

Status: Active » Needs review
Issue tags: +Novice
FileSize
588 bytes
623 bytes

patches for review.

droplet’s picture

There 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.

rbayliss’s picture

This 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.

jenlampton’s picture

@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.

BTMash’s picture

Status: Needs review » Needs work

Conceptually, 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).

jenlampton’s picture

@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.

jenlampton’s picture

Title: Remove #maxlength property from form elements where #maxlength is >= 128 » Remove maxlength HTML attribute from form elements where #maxlength is >= 128

Ah, maybe the title was misleading :)

BTMash’s picture

I 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.

BTMash’s picture

Status: Needs work » Needs review

I 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.

jenlampton’s picture

Status: Needs review » Needs work

@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?

jenlampton’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests

Doh.

rbayliss’s picture

@jenlampton: I used something like:

$form['test'] = array(
    '#type' => 'textfield',
    '#title' => 'Test',
    '#default_value' => $values['test'],
    '#maxlength' => 400,
  );
  $form['test_machine'] = array(
    '#type' => 'machine_name',
    '#default_value' => $values['test_machine'],
    '#maxlength' => 128,
    '#machine_name' => array(
      'exists' => 'mytest_exists_callback',
      'source' => array('test'),
    )
  ); 

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.

xjm’s picture

Hmmm, 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.

Status: Needs review » Needs work

The last submitted patch, remove_maxlength_when_long-1360466-1-D7.patch, failed testing.

monil-dupe’s picture

I can't install patches too :(

shumer’s picture

Status: Needs work » Needs review
Issue tags: +dcdn-sprint
FileSize
610 bytes

Patch for review

Status: Needs review » Needs work

The last submitted patch, 16: remove_maxlength_when_long-1360466-15.patch, failed testing.

shumer’s picture

Status: Needs work » Needs review
FileSize
610 bytes

Patch for review

kalanh’s picture

Assigned: Unassigned » kalanh

@DrupalCon Austin Sprint

jlyon’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed and tested.

kalanh’s picture

Assigned: kalanh » Unassigned

Patch applied cleanly, agree with jlyon.

catch’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs usability review

I 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'.

Status: Needs review » Needs work

The last submitted patch, 18: remove_maxlength_when_long-1360466-16.patch, failed testing.

Tebro’s picture

Status: Needs work » Needs review
FileSize
615 bytes

Redid 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.

Tebro’s picture

FileSize
757 bytes

Moved code to correct place in function.

Patrick Storey’s picture

I 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

Bojhan’s picture

Isn'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.

Patrick Storey’s picture

Issue tags: -Novice

Doh! Looks like I didn't remove the tag in comment #27. Remedying that now.

Bojhan’s picture

Issue tags: -Needs usability review

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

gambry’s picture

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

So 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.