Contrib.social

trim(): Passing null to parameter #1 of type string is deprecated

Open on Drupal.org →
Created on 1 February 2023, almost 2 years ago
Updated 17 February 2023, over 1 year ago

We added a question/answer challenge to our login form, based on certain circumstances. The challenge is added during a custom validation function and the form is reloaded (rebuilt) - which essentially makes it a multi-step form. The problem is this code in UserLoginForm->validateAuthentication():

  public function validateAuthentication(array &$form, FormStateInterface $form_state) {
    $password = trim($form_state->getValue('pass'));
    $flood_config = $this->config('user.flood');
    if (!$form_state->isValueEmpty('name') && strlen($password) > 0) {
      ...
    }
  }

On the second submit, after the user answers the challenge question, password is null, so the first line of code in the above function gives this error:

Deprecated function: trim(): Passing null to parameter #1 ($string) of type string is deprecated in Drupal\user\Form\UserLoginForm->validateAuthentication() (line 190 of core/modules/user/src/Form/UserLoginForm.php).

It could be argued that a null password is not valid for a validateAuthentication function, but two lines down from the offending line, it checks strlen($password) > 0, so it is obviously allowing for a null password condition to exist.

I don't really understand why PHP was changed to not allow null to be passed to trim() - it should just return a null string, as it surely must have done before PHP 8 - but, that's the way it is now. So if a test could be added for password being null before being passed to trim(), that would solve the issue.

🐛 Bug report
Status

Needs work

Version

9.5

Component
User system  →

Last updated about 11 hours ago

Created by

🇺🇸United States ExTexan

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Comments & Activities

  • Issue created by @ExTexan
  • Status changed to Needs review almost 2 years ago
  • Comment almost 2 years ago →
    🇮🇳India noorulshameera

    Adding a patch to fix the deprecation issue.

  • Comment almost 2 years ago →
    cilefen

    …so it is obviously allowing for a null password condition to exist.

    I think it is looking for an empty string, not a null.

  • Status changed to Needs work over 1 year ago
  • Comment over 1 year ago →
    🇺🇸United States smustgrave

    This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

    This will need steps to reproduce.

    Also not sure of the current fix. Seems to be fixing a symptom vs the cause.

    Would recommend putting a temporary patch in that when a bad value is passed in to log as much info as possible. Then can help figure out what's going on.

    Will need a test case.

  • Comment over 1 year ago →
    🇺🇸United States ExTexan

    @smustgrave, your comment about "...not sure of the current fix. Seems to be fixing a symptom vs the cause." prompted me to point out one aspect of my issue summary that may be relevent here.

    By adding a challenge question/answer feature to our client's login procedure, we essentially turned the Login form into a multi-step form. When it reloads (with our challenge field added), the password the user originally entered is blank. I'm not sure why, but I'm guessing it's a security measure. In any case, on the subesequent submit, the function in question runs (beyond our control) with a blank/null password.

    The point of this is, I don't see that as a invalid condition (a "symptom" as you put it). It seems to be a valid situation that that function needs to check for, and handle, a blank password. The fix seems perfectly reasonable, in that light.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024