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
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | 3250397-15.patch | 3.79 KB | alexpott |
| #15 | 3250397-15.test-only.patch | 3.08 KB | alexpott |
| #15 | 12-15-interdiff.txt | 1.99 KB | alexpott |
| #12 | 3250397-12.patch | 3.75 KB | alexpott |
| #12 | 11-12-interdiff.txt | 2.32 KB | alexpott |
Comments
Comment #2
mondrakeComment #3
mondrakeComment #4
daffie commentedIs it possible to add a test?
Comment #5
mondrakeDon't know.
Comment #7
mondrakeActually, 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
which occur when parsing the url returned by
\Drupal::request()->getSchemeAndHttpHost()which in this case ishttp://:that is invalid.Comment #8
mondrakeI 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:
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.Comment #9
dagmarEven 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.
Comment #10
alexpottYou 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.
Comment #11
alexpottHere's where I would fix this.
Comment #12
alexpottAnd 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.
Comment #13
mondrakeLooks good.
Maybe here a comment, explaining why we are checking that no 'Deprecated' is present in the output, wolud be helpful.
Nit:
:voidreturn typehint.Nit: use
LoggerInterface::classsyntax.Same
Comment #14
daffie commentedI 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.
Comment #15
alexpottI'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.
Comment #17
mondrakeThanks!
Comment #18
ressaThanks! 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 75messages 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:
Result:
Comment #22
xjmOh hey, I get these errors all the time when I use the
quick-startcommand! 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
emptythe IP in the context array is, it's highly unlikely, and the bug is very noisy.)I made a small grammatical improvement on commit:
Comment #23
xjm