_form_validate sends null to drupal_strlen triggering deprecation notice

Thanks for reporting this @DeanThomas and for providing a patch!

I think we should add a test for this behavior. Then we can consider if we are going to fix this as proposed in patch #2, or use more global approach and fix the problem directly in drupal_strlen() by casting the $text to string (as we have done in few others, like drupal_substr(), see: #3241422: [PHP 8.1] Passing `null` to internal functions deprecation fixes → ).

As mentioned in my late comments on a closed issue ( https://www.drupal.org/project/drupal/issues/3270881 🐛 [D7 PHP 8.1] strpos(): Passing null to parameter #1 ($haystack) of type string is deprecated in text_summary() Fixed ), it seems to me that the right, developer-friendly answer is to make drupal_strlen() behave as it does in PHP 7 when passed a NULL value - it should simply return 0.

I'm not sure what the right answer should be when handling other variable types: arrays, objects, scalar values. Casting to string can result in odd behavior - in particular the function will return 5 if given (string) $array, which just seems wrong and unhelpful. But perhaps that's a question for another day, as the vast majority of errors seem likely to be the result of passing NULL.

In short, can we just do https://www.drupal.org/project/drupal/issues/3270881#comment-15027276 🐛 [D7 PHP 8.1] strpos(): Passing null to parameter #1 ($haystack) of type string is deprecated in text_summary() Fixed and call it a day? I'm happy to produce a patch or fork if that seems like the right answer. I'm not sure where a test should go - in UnicodeUnitTest?

I think we should not change the behavior for other data types like array, objects, ..., as it would mask the problem for developers/users in case they are passing a non-string here. The most safe approach is, as you have mentioned, to check if the string is NULL and if yes, then return 0.

For the test - yes, we can add a NULL key to the UnicodeUnitTest::helperTestStrLen() to test the drupal_strlen() itself, but I was thinking about testing the the usecase mentioned in the issue summary -

When $elements['#value'] is NULL, and $elements['#maxlength']

. This would be more suited to the form.test file.

Thanks!

@poker10
I have added the fix, a unicode test (easy), and a form validation test (harder - as there was no existing test for maxlength validation).

Thanks! Triggering tests on the new merge request.

I have added some minor comments to the MR. In overall it looks good!

I have tried the test-only patch and I suppose this should fail?

    $edit['textfield'] = NULL;
    $edit['password'] = NULL;
    $edit['machine_name'] = NULL;
    list($processed_form, $form_state, $errors) = $this->formSubmitHelper($form, $edit);
    $this->assertTrue(empty($errors), 'Form with NULL inputs did not return errors.');

But for some reason, it is not failing for me (in test-only patch).

Thanks!

After the recent changes, the MR is failing because the machine_name element is missing some required properties. I suggest that we skip testing the machine_name element and keep only password and textfield here.

Anyway, I am still not able to get the error from the IS

mb_strlen() expects parameter 1 to be string, array given in .../drupal/includes/unicode.inc on line 482.

with the test-only patch (using the NULLs) even after removing the machine_name element.

Adding to 📌 [meta] Priorities for 2023-12-06 release of Drupal 7 Active

I have implemented my own feedback from the MR :) Also updated the MR so that the test-only pipeline can be run.

Test-only job failed as expected: https://git.drupalcode.org/project/drupal/-/jobs/1306529

Test summary
------------
Unicode handling 179 passes, 0 fails, and 2 exceptions
Test run duration: 0 sec
Detailed test results
---------------------
---- UnicodeUnitTest ----
Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Exception Deprecated unicode.inc        482 drupal_strlen()                    
    mb_strlen(): Passing null to parameter #1 ($string) of type string is
    deprecated
Exception Deprecated unicode.inc        486 drupal_strlen()                    
    preg_replace(): Passing null to parameter #3 ($subject) of type array|string
    is deprecated

Fix+test pipeline is green as expected: https://git.drupalcode.org/project/drupal/-/pipelines/144322

Moving to Needs review, so hopefully we can finish this.

Adding a tag for the final review.

I'm a bit surprised that there's apparently no existing test of Form API's maxlength but indeed.. I don't see one.

This looks like a good change / BC layer, thanks all!

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