Problem/Motivation

Getting a lot of this in CLI context

PHP Deprecated: mb_substr(): Passing null to parameter #1 ($string) of type string is deprecated in /home/runner/work/drudbal/drudbal/core/modules/dblog/src/Logger/DbLog.php on line 75

Steps to reproduce

Use a non-interactive script to install drupal on PHP 8.1 via CLI.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Comments

mondrake created an issue. See original summary.

mondrake’s picture

StatusFileSize
new645 bytes
mondrake’s picture

Status: Active » Needs review
daffie’s picture

Is it possible to add a test?

mondrake’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Don't know.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mondrake’s picture

Issue summary: View changes

Actually, there is more to it: the problem is that when running Drupal code via the CLI, the current Request object does not carry host/port (since it's not executed via a web server). So I am also incurring in problems like

TypeError: array_keys(): Argument #1 ($array) must be of type array, bool given in array_keys() (line 456 of /home/runner/work/drudbal/drudbal/core/lib/Drupal/Component/Utility/Html.php).
#1 /home/runner/work/drudbal/drudbal/core/lib/Drupal/Core/Mail/MailManager.php(288): Drupal\Component\Utility\Html::transformRootRelativeUrlsToAbsolute()
#2 /home/runner/work/drudbal/drudbal/core/lib/Drupal/Core/Mail/MailManager.php(180): Drupal\Core\Mail\MailManager->doMail()
...
...
...

which occur when parsing the url returned by \Drupal::request()->getSchemeAndHttpHost() which in this case is http://: that is invalid.

mondrake’s picture

Status: Needs work » Active
Issue tags: -Needs tests

I solved both #7 and the OP by adding this to my CLI script, at the very beginning prior to the Request object being determined by Drupal's kernel:

// Some minimal values for $_SERVER.
$_SERVER['SERVER_NAME'] = 'localhost';
$_SERVER['SERVER_PORT'] = 80;
$_SERVER['HTTP_HOST'] = 'localhost';
$_SERVER['REMOTE_ADDR'] = '127.0.0.1';

AFAICU, using Request::createFromGlobals() does not provide defaults for missing keys of the $_SERVER PHP core environment variable, which is the case when a script is run from CLI. Request::create() would, but that's not what Drupal uses. Now, so far before PHP 8.1 that was not much of a problem since there was not tight check on this, but now in CLI we will start seeing TypeErrors being thrown. I do not know whether this is an issue for Drupal core itself, or should rather be addressed outside. Maybe in any case some docs would help.

dagmar’s picture

Even with the fix outside the dblog module, I think this issue is still valid. Since potentially we are calling a function that expect a string with an invalid value.

alexpott’s picture

Status: Active » Needs review
StatusFileSize
new739 bytes

You get this when running the quickstart command on Drupal 10 - so I think we can add test coverage for it.

Here's how.

For me the fix is in the wrong place.

alexpott’s picture

StatusFileSize
new1.43 KB

Here's where I would fix this.

alexpott’s picture

StatusFileSize
new2.32 KB
new3.75 KB

And here's an additional test in a more appropriate place. I think we should continue to test deprecations in quickstart because that's embrassing when they occur.

mondrake’s picture

Looks good.

  1. +++ b/core/tests/Drupal/Tests/Core/Command/QuickStartTest.php
    @@ -116,6 +116,7 @@ public function testQuickStartCommand() {
    +    $this->assertStringNotContainsString('Deprecated', $process->getErrorOutput());
    

    Maybe here a comment, explaining why we are checking that no 'Deprecated' is present in the output, wolud be helpful.

  2. +++ b/core/tests/Drupal/Tests/Core/Logger/LoggerChannelTest.php
    @@ -97,6 +97,39 @@ public function testSortLoggers() {
    +  public function testNullIp() {
    

    Nit: :void return typehint.

  3. +++ b/core/tests/Drupal/Tests/Core/Logger/LoggerChannelTest.php
    @@ -97,6 +97,39 @@ public function testSortLoggers() {
    +    $logger = $this->createMock('Psr\Log\LoggerInterface');
    

    Nit: use LoggerInterface::class syntax.

  4. +++ b/core/tests/Drupal/Tests/Core/Logger/LoggerChannelTest.php
    @@ -97,6 +97,39 @@ public function testSortLoggers() {
    +    $request_mock = $this->getMockBuilder('Symfony\Component\HttpFoundation\Request')
    

    Same

daffie’s picture

Status: Needs review » Needs work

I am not sure, but I thought that every test method should have at least one assertion and the added test does not have one.

Changing the status back to NW for comment #13.

alexpott’s picture

Priority: Normal » Major
Status: Needs work » Needs review
StatusFileSize
new1.99 KB
new3.08 KB
new3.79 KB

I'm going to make this a major bug because quickstart is one of the recommended ways to first experience Drupal and if people try this on Drupal 10 today on the only supported PHP version they'll see a lot of deprecation warnings.

@daffie settings an expectation in a mock is an assertion. If that expectation is not fulfilled then the test fails. In this instance the test will fail if the ip context is NULL. I'll add a test only patch.

The last submitted patch, 15: 3250397-15.test-only.patch, failed testing. View results

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

ressa’s picture

Thanks! I agree this is pretty big, the command line is getting filled up with PHP Deprecated: mb_substr(): Passing null to parameter #1 ($string) of type string is deprecated in /home/ras/drupal/core/modules/dblog/src/Logger/DbLog.php on line 75 messages under PHP 8.1 ...

I can confirm that the patch fixes it in both D9.4 and D10, and the installation completes in a nice and quiet fashion:

(
$ git clone --branch 10.0.x https://git.drupalcode.org/project/drupal.git && cd drupal && composer install
$ wget -q -O - https://www.drupal.org/files/issues/2022-02-25/3250397-15.patch | git apply -
$ php -d memory_limit=256M ./core/scripts/drupal quick-start standard
)

Result:

Cloning into 'drupal'...
remote: Enumerating objects: 843731, done.
[...]
Packages already clean.
phpstan/extension-installer: Extensions installed
> mglaman/phpstan-drupal: installed
17/17 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓]
Congratulations, you installed Drupal!

  • xjm committed 5948cfb on 10.0.x
    Issue #3250397 by alexpott, mondrake, ressa, daffie, xjm: DbLog triggers...

  • xjm committed 21b92bd on 9.4.x
    Issue #3250397 by alexpott, mondrake, ressa, daffie, xjm: DbLog triggers...

  • xjm committed 1755962 on 9.3.x
    Issue #3250397 by alexpott, mondrake, ressa, daffie, xjm: DbLog triggers...
xjm’s picture

Version: 9.4.x-dev » 9.3.x-dev

Oh hey, I get these errors all the time when I use the quick-start command! Thanks for the fix.

Committed to 10.0.x and cherry-picked to 9.4.x. I also think this is probably safe for 9.3.x backport. (While it's theoretically possible that some code somewhere is relying on the exact kind of empty the IP in the context array is, it's highly unlikely, and the bug is very noisy.)

I made a small grammatical improvement on commit:

   /**
-   * Tests if Request::getClientIp() returns NULL, $context['ip'] is a string.
+   * Tests that $context['ip'] is a string even when the request's IP is NULL.
    */
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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