Increase default textfield #maxlength=128 to 256

I'd agree with either solution a) or b), where setting the limit to 256 would be more explicit, as you wouldn't usually need a higher char count for labels etc. anyway. But removing the maxlength entirely could help with the remaining 1% needing a higher char count.

So if it wouldn't have any negative side effects, I'd vote for solution a), otherwise b).

EDIT: The failing tests seem unrelated.

Out of interest I looked through git history and found that we set maxlength to 128 for textfields in #35644: Change defaults on form elements to most common values - previously it was an arbitrary value of 70 instead.

I am not sure what the best option is here. I don't really like any of them except the simplest one: remove the default maxlength entirely - but I also share the BC concerns. There is definitely a risk of introducing bugs if we just drop the length, because of things like database column sizes being overflowed where previously this was not possible.

We could (in theory) deprecate it by detecting when it's not set, warning users that #maxlength default will change in the next major, etc, but is it worth it? I am not entirely sure of the benefit of changing it now given that form API elements can just override this value if they need to.

Adding framework manager review tag to get more visibility on this.

I faced that issue in an views exposed filter on an entity reference field using the autocomplete widget.
One of my users tried to search for a referenced entity which title is 138 chars long (+ double quotes and ID between parenthesis). The autocomplete works well and the field is filled when selecting the right result but after submitting the form, they gets an error and no results.

Thinking about the deprecation, I'm not sure we can do that, unless we want to force everyone to set a maxlength, otherwise there's no way to remove the deprecation message for modules that don't want it set.

There is definitely a risk of introducing bugs if we just drop the length, because of things like database column sizes being overflowed where previously this was not possible.

This is true but I feel like for entities this would already be a problem due to JSON:API. The database will either truncate the value or throw an exception, the exception could be jarring, but it would only be for someone who previously would not have successfully submitted that form with the same values.

I think this is probably OK to do in a minor release, but with quite a loud change record/release note.

It's also the sort of thing we could make 'major version only', generally we've avoided that as much as possible, but this might be a case where that would work.

I am doing triage on the core RTBC queue.

I read the issue summary and the most importantly the proposed resolution is a set of options. I then read the comments to figure that out. The comment that set this to RTBC supported 2 of the options. Since that is not clear I am tagging for an IS update and setting to NW.

In #15 catch asked for a CR. I am adding that tag. Of course, that shouldn't be made until this has a definite resolution. He also considered that this might be done in a major release. Therefor, I am tagging for RM review as well.

I am setting to NR for discussing and selecting the option to implement.

For the issue summary update and change record.

Let's simply elevate the max length to 256. This should be enough. Removing it entirely might have further consequences.

EDIT: Sorry, a bit too fast on the submit button here.

According to the discussion here, we should proceed with MR !4008 as @catch confirmed the approach in #15.

I'll create the change record.

Ok, I created a Change Record! Back to "RTBC".

1 test fails.

OK, the last failing test is simply an unrelated racing condition problem. I don't think, this should be fixed in this issue, as it is completely unrelated.

Setting this back to "Needs work" as I don't think any core maintainer would commit an MR containing failed tests. Maybe someone can help with #3396907: testCreateViewWizard fails, when removing the "#maxlength" property of the textfield form element. I have no idea, what makes the test fail and can not reproduce it through manual testing.

Alright, back to needs review! Special thanks to @lendude, who provided the "_populate" fix on Slack! Thanks again! 👍
Please credit him manually!

Pipeline is green! https://git.drupalcode.org/issue/drupal-3331028/-/pipelines/39305

Are there any existing forms in core (entity labels? Machine names?) where we might be relying on the default 128 #maxlength and need to add an explicit value in those forms?

@Anybody can we look at core usages of #type 'textfield' where it doesn't have #maxlength?

Following up @Anybody if you had a chance to look at #37.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I did a search in core (granted, it was 10.2), for '#type' => 'textfield', and stopped counting at 10 when I found files that rely on the default value.

Also, it seems like relying on the default value is kind of OK, which is why it's there.

Config offers a schema constraint of Length.max, so it seems like a config form would use that value if it's present, only using the form element default if there was no such constraint. Ideally this would happen in some automatic or semi-automatic way to make it easy to build config forms. Maybe as a method on ConfigFormBase that does magic for you.

It seems like there would be some other issue where this is happening, but I don't see it, other than in initiatives to make sure all config has constraints: #2869792: [meta] Add constraints to all config entity types It also seems like it would slot right into this CR's purpose: https://www.drupal.org/node/3373502

Re-rolling the MR as a patch to apply to 10.4.0

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Created a new branch targeting 11.x (3331028-increase-or-remove-maxlength-remove-11.x).

I did a search in core (granted, it was 10.2), for '#type' => 'textfield', and stopped counting at 10 when I found files that rely on the default value.

@mile23, meaning at least 10 form elements without #maxlength set? Should we manually set the `maxlength` attribute there?

Couldn't we just proceed and merge this? I understand, that this could potentially lead to problems, but we are using this patch in production for a year now and never had any issues with it.

@mile23, meaning at least 10 form elements without #maxlength set? Should we manually set the `maxlength` attribute there?

I think it would be good to at least investigate whether there are high-usage forms (anything visitor/editor facing mainly) that might be relying on the default max length that need updating to have it set explicitly before merging this. Untagging for RM/FM review since I've looked at this twice now.

I did a search through core 10.4.3 and came up with 475 different '#type' => 'textfield' instances. Some that I saw had maxlength set, but a lot did not, including fields for things like database names and URL paths in Views forms.

I don't know how many problems it would cause by removing the default limit, or by having the limit at 256 instead of 128. But it feels like having some default limit is a good idea. I can update the MR to increase the default limit to 256, instead of removing it, if folks are on board.

Most of issues last years (starting with node title length) are all about 128 to 256 transition
Database fields mostly also have 256 for text so the remains is easy to check

I updated the MR to restore the #maxlength and increase it to 256. I think this is the best we can do, given the large number of existing fields without any limit set.

If the text input is too long, I think it's a better user experience to have it identified at the form, rather than the database layer. I agree with @anybody that the limit really exists at the database layer, so it could be handled there. But DB limit errors are harder for end users to understand.

Could the summary be cleaned up about what solution was decided please

I updated the summary to note we're going with option B, increasing the default limit to 256.

Looking at the MR increasing to 256 makes sense but that doesn't line up with the change to core/modules/views_ui/js/views-admin.js. What's the scenario when maxlength isn't there? Shouldn't we have test coverage for that scenario?

I'm just running into this issue and it seems like increasing the max-length to 256 would be an improvement over setting the default max-length to 128 as it currently is.

In terms of the views-admin.js change it may make sense to just remove that change if it makes merging this easier unless that is specifically one of the goals of this MR.

It seems like that was likely created in-line with the idea of removing the default max-length altogether and since we are simply changing the default we shouldn't need to touch this check.

Another reason to go ahead with this change is the default field length of a text field created using the UI is 255. So setting the default max-length to 128 can create some issues for instance I ran into an issue in the conditional_fields module where they just create a text-field and it has the default max-length of 128 and so I was unable to input the required value.