- 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
Instead of changing all of these inputs to textareas, why don't we increase the maxlength to something less restrictive?
AFAICS Drupal's formapi doesn't have a maximum value for maxlength which is likely because there is no maximum in the spec:
- https://html.spec.whatwg.org/multipage/input.html#the-maxlength-and-minl...
- https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#...
- https://stackoverflow.com/questions/26469152/why-is-the-default-max-leng...
Per that last thread, it looks like browsers implement a very high limit in practice, but seems like we could - e.g. - add a zero to the end of the maxlength without breaking anything.
I am not a UX expert, but I'd think it best not to add 13 textareas to the already very busy admin form.
I did have a look at using a textarea but adding some attributes to keep the form element roughly the same size by default, but I think we'd then need to get into messing with CSS (e.g. textareas seem to default to width: 100% and the grippie div below them matches).
Example:
// CSP default-src directive. $form['seckit_xss']['csp']['default-src'] = array( - '#type' => 'textfield', - '#maxlength' => 1024, + '#type' => 'textarea', + '#attributes' => array('rows' => 1, 'cols' => 68, 'style' => 'width: auto'), '#default_value' => $options['seckit_xss']['csp']['default-src'],
That seems to work with default "Seven" admin theme, but the grippie div would need some extra CSS, and it'd almost certainly be better not to add a style attribute to the form element.
It's not too hard to get the CSS to work for Seven etc.. but we risk making a mess in other admin themes.
On the other hand, just changing all the fields to textarea makes quite a visual mess IMHO.
All that said, I'm quite inclined to just add that zero to the #maxlength for each textfield.
Any objections?
- 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
Had to give the textarea + CSS route a try.
I'm fairly happy with this in Seven, but haven't looked at any other admin themes.
There are other textareas in the seckit admin form that we could add the rows=1 attribute to for consistency.
Increasing the maxlength would be a simpler change than this, but it's not very friendly UX.
- 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
Tweaked to make the same change to most of the other textfields / textareas in the admin form.
I've not changed the textfields which are "max" values expecting small integers.
- 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
Added a test for a long value being submitted for a CSP directive in the admin form.
I'll commit this patch if tests pass.
- 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
Per #4 one concern here is that users may enter line breaks into textareas and that's likely to cause problems with most if not all of the fields we're changing here.
We could add validation to raise an error if any of the textareas contain line breaks?
That might be better than adding to the submit hook to silently remove line breaks - although that's what D7 does to textfields:
https://git.drupalcode.org/project/drupal/-/blob/7.94/includes/form.inc#...
- 🇧🇪Belgium Dozz
Hi, I believe allowing line breaks would be great advantage especially if this change would also be ported to the D8+ versions.
It would allow people to group entries on different lines but more importantly, make it possible to have entries on separate lines in the configuration files.
Especially on larger sites, where multiple features are developed and tested simultaneously, multiple feature-branches often include a different change on the same line (e.g. adding an item to script-src) in the configuration file (seckit.settings.yml) which results in harder to fix merge conflicts.
- 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
Okay, but we'd then have to remove the newlines when processing the directives, unless I'm missing something?
I can see the advantage for D9+ when checking config into version control, but I think that's out of scope for what we're looking at here.
I'm trying to get this "small" change done in an attempt to prepare a new D7 release :)
- 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
Here's some validation for the textareas that shouldn't contain newlines (assuming their values are going straight into http response headers).
Plus a test of that validation.
I think I'm done for now; will commit if tests pass.
@dunx your suggestion of allowing newlines in some fields likely makes sense especially in the D9+ branch. I think that should be a followup issue though. Thanks!
- Status changed to Fixed
almost 2 years ago 3:05pm 3 February 2023 - 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
Marking this as fixed for the 7.x-1.x branch. Thanks!
A followup issue should be filed for 2.x (and perhaps that should accept + process newlines in some of the inputs).
- 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
Created ✨ Text fields not big enough Fixed
Automatically closed - issue fixed for 2 weeks with no activity.