Contrib.social

PHP 8 compatibility - Deprecated function: substr(): Passing null to parameter #1

Open on Drupal.org →
Created on 24 July 2023, over 1 year ago
Updated 31 October 2023, about 1 year ago

Problem/Motivation

On PHP 8.1, when a wrong password is attempted, and the Global password field is empty, the following exception is thrown:

Deprecated function: substr(): Passing null to parameter #1 ($string) of type string is deprecated in Drupal\Core\Password\PhpassHashedPassword->check() (line 224 of core/lib/Drupal/Core/Password/PhpassHashedPassword.php).
Drupal\Core\Password\PhpassHashedPassword->check('1', NULL) (Line: 178)
Drupal\protected_pages\Form\ProtectedPagesLoginForm->validateForm(Array, Object)
call_user_func_array(Array, Array) (Line: 82)
Drupal\Core\Form\FormValidator->executeValidateHandlers(Array, Object) (Line: 275)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object, 'protected_pages_enter_password') (Line: 118)
Drupal\Core\Form\FormValidator->validateForm('protected_pages_enter_password', Array, Object) (Line: 593)
Drupal\Core\Form\FormBuilder->processForm('protected_pages_enter_password', Array, Object) (Line: 325)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 73)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 580)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 163)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 74)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 692)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

Steps to reproduce

Attempt a wrong password on a protect page, where the Global password isn't configured.

Proposed resolution

Ensure that a string is always passed into the password check method.

Remaining tasks

User interface changes

N/A

🐛 Bug report
Status

RTBC

Version

1.0

Component

Code

Created by

codebymikey

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @codebymikey
  • @codebymikey opened merge request.
  • Status changed to Needs review over 1 year ago
  • Comment over 1 year ago →
    codebymikey
  • Comment about 1 year ago →
    🇧🇬Bulgaria pfrenssen Sofia

    Can you give some context about the changes made in the MR? It seems there are unrelated changes included which are not necessary to fix the bug. This is increasing the time and mental load needed to review the patch, so it would be beneficial to explain why they are included.

    1. I understand that the settings file export is adhering to the updated format of the symfony/yaml component. This is out of scope but a good improvement.
    2. The config values are being cast to strings every time they are read. This looks like a valid fix but it also looks brittle - developers need to be told somehow that they are not to read this config value without always casting it to a string. I am wondering if it would be better to declare that the field is not nullable and provide an update hook to update existing config from NULL to an empty string? Then we do not need to forcefully cast it every time it is read.
    3. Is the replacing of \r\n line endings with \n a fix for another known issue?
  • Status changed to RTBC about 1 year ago
  • Comment about 1 year ago →
    🇧🇬Bulgaria pfrenssen Sofia

    To answer my own questions: the casting of "\r\n" to "\n" is to ensure the descriptions can be saved in config in the new YAML format and can be edited through the settings form without causing a change in the line endings.

    After a more in-depth review I'm also OK with the casting to strings now. The global password is optional, so it would not be right to set it to be non-nullable. Casting to a string is a bit "dirty", but cleaner solutions like using null coalescing operators are PHP 8 only and the module still officially supports Drupal 8.8 (and that means it should run on PHP 7.4). The proposed solution in the MR with casting to strings is fully compatible with PHP 7 and PHP 8.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024