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

Provide issue fork/patch.

User interface changes

N/A

Issue fork protected_pages-3376507

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

codebymikey created an issue. See original summary.

codebymikey opened merge request !15

codebymikey’s picture

codebymikey commented
Issue summary: View changes
Status: Active » Needs review
pfrenssen’s picture

pfrenssen
Location Sofia
commented

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?
pfrenssen’s picture

pfrenssen
Location Sofia
commented
Status: Needs review » Reviewed & tested by the community

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.

  • oksana-c committed cc15bc14 on 8.x-1.x authored by codebymikey 
    Issue #3376507: PHP 8 compatibility - Deprecated function: substr():...
oksana-c’s picture

oksana-c commented

Merged. Thank you all!

oksana-c’s picture

oksana-c commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.