Open klonos opened 2 years ago
Thanks @klonos, interesting suggestion here. This does trip me up from time to time. In reviewing the code, it seems a little magical that the values are trimmed before the #element_validate
callbacks are called. Though with proper documentation of the new FormAPI #trim
property I don't think that should be a hold-up.
Sorry @quicksketch ...the PR is not ready for review yet. I've been cleaning up trim()
incrementally (module-by-module or file-file), so to be able to fix any failing tests easily as they may come up. I'm not done with that yet.
In the PR, you've asked: "Why no #trim
on $types['password']
?". Well I have intentionally left that out for a couple of reasons:
The FormTest on the
password_confirm
field... I don't know. Perhaps we should simply remove the#trim
from#type
'password'
for now, and look into that later? That would mean we'd have to revert all relatedtrim()
removals from the user register/login form validation handlers. Personally, I'd be more than happy with ignoring that for now (i.e., reverting#trim
on#type
password). The massive simplifications elsewhere across the code base are worth to have regardless of those password-specifics. :)
...by the way, did some further research on passwords and password fields, and I've found that we are in fact trimming leading/trailing whitespace in places like:
UserStorageController::preSave()
:
// Update the user password if it has changed.
$new_pass = isset($entity->pass) && strlen(trim($entity->pass)) > 0 ? trim($entity->pass) : NULL;
password_confirm_validate()
:
$pass1 = trim($element['pass1']['#value']);
$pass2 = trim($element['pass2']['#value']);
if (strlen($pass1) > 0 || strlen($pass2) > 0) {
if (strcmp($pass1, $pass2)) {
form_error($element, t('The specified passwords do not match.'));
}
}
Still, I'll wait for more feedback before adding auto-trimming to password form elements and password validation/submission handlers.
did some further research on passwords and password fields, and I've found that we are in fact trimming leading/trailing whitespace in places like:
Good to hear that we're already doing trimming on passwords.
I wanted to get more feedback re the possibility of legitimate cases of passwords having leading/trailing spaces. If we trimmed those out, we'd break the login process for user accounts that intentionally use spaces that way. So thoughts on that anyone?
I don't think we should consider this a legitimate use-case. Empty spaces happen accidentally all the time when copy/pasting and we will help a lot more users by trimming the input that we would allowing it for the oddball case that intentionally wants a space at the beginning or end of their password (spaces in the middle are still fine). Worst-case scenario for such users would be they would need to do a password reset.
That's fine then @quicksketch 👍🏼 ...I've implemented the default trimming to form elements of '#type' => 'password'
and '#type' => 'password_confirm'
with backdrop/backdrop@111d2c3
(#3845) + more cleanup coming in other commits for module-specific form submissions/validations (such as in user.admin.inc and user.module).
Still WIP, as I'm going through each module one at a time, fixing any test failures along the way.
I have started working on this again (need to fix remaining test failures), but in the meantime I wanted to ask this: besides apparent input fields (those exposed to actual user input), should we also be automatically trimming the following types of fields?
item
hidden
value
markup
help
token
I think so, but I'd like feedback from others.
How about making the value of #trim
the name of the trimming function. Then it could be trim
for ordinary trimming, but if someone had a custom trimmer (like, removing leading or trailing /
from a path) they could use the name of that function.
Great suggestion @bugfolder! Thanks 👍🏼
Noting that trim()
without any additional parameters removes whitespace (regular spaces, tabs, line returns etc.), but you can pass a series of characters as an additional pararameter. See: https://www.php.net/manual/en/function.trim.php
So, in the case of wanting to trim /
s from the start/end of paths, you can do trim($path, '/')
.
This may be getting too baroque, but could this property also accept array('trim', '/')
(more generally, array($func, $args)
to accomplish the latter? Or do we just ask the user to create a function path_trim()
(or we provide backdrop_path_trim()
) and then they use that.
Yes, we have already existing patterns like #validate
and #validation_arguments
(although that one is for internal usage). So we could have something similar for trimming.
besides apparent input fields (those exposed to actual user input), should we also be automatically trimming the following types of fields?
I don't think we should be trimming whitespace on anything that can't be edited. We're trying to help people who put in the spaces accidentally, and you can't have that accident if you can't edit the form element. :)
This is a follow-up to #5348 and the respective of https://www.drupal.org/project/drupal/issues/67769