Passing null to non-nullable internal function parameters - Updating Existing Code Base to php 8.1
Asked Answered
F

4

8

I am just getting started on upgrading my code to be php 8.1 compatible. I have many pieces of code where I am passing potentially null values to internal functions.

if (strlen($row) > 0) {
   ...
} 

Where $row comes from a source that may have null values (e.g. a query). This can generate a deprecation warning; in this case:

Deprecated: strlen(): Passing null to parameter #1 ($string) of type string is deprecated

I'm looking for the easiest most time efficient way to handle upgrading this code, for example fixes where global search and replaces are possible. It seems that type casting the variable I pass to an internal function works without changing functionality.

error_reporting(E_ALL);
$row = null;

if (strlen((string) $row) > 0) {
   ...
}

Aside from the moral aspects of coding this way, are there issues with this approach for internal functions? Are there better ways (other than completely rewriting the code and handling nulls differently)? I prefer this solution backwards compatible to v7.4, although I could possibly live with 8.0 compatibility.

I am aware my user defined functions have other choices.

Fogarty answered 4/1, 2022 at 4:18 Comment(0)
P
6

If you're explicitly trying to handle the case of null, then a slightly cleaner fix would be strlen($row ?? '') using the "null coalescing operator".

In most cases, the two are probably equivalent but with strict_types=1 in effect, they behave differently if the value is some other type that can be cast to string:

declare(strict_types=1);
$row = 42;
echo strlen($row); // TypeError: must be of type string, int given
echo strlen((string) $row); // Succeeds, outputting '2'
echo strlen($row ?? ''); // TypeError: must be of type string, int given

On the other hand, note that the ?? operator is based on isset, not === null, so an undefined variable will behave differently:

declare(strict_types=1);
$row = [];
echo strlen($row['no_such_key']); // Warning: Undefined array key; TypeError: must be of type string, null given
echo strlen((string) $row['no_such_key']); // Warning: Undefined array key; outputs '0'
echo strlen($row['no_such_key'] ?? ''); // No warning, just outputs '0'

If you care about that case, the most directly equivalent code to the old behaviour is rather more verbose:

echo strlen($row === null ? '' : $row);
Plaid answered 5/1, 2022 at 10:6 Comment(7)
Thanks for the detailed use case scenario. I noticed that without declare(strict_types=1); passing an int did not throw a warning with 8.1. Do you know why? It seems that php 8 is not enforcing strict typing in all cases.Fogarty
@Fogarty That's exactly what the strict_types declaration is for - if the value can be safely cast to a string, it's allowed. A better name would be "scalar_types=cast" for the default mode, and "scalar_types=error" for the "strict" mode. That hasn't changed in 8.1, only the handling of null, which was never affected by that setting.Plaid
However, with strict_types declared, I get Fatal error: Uncaught TypeError: strlen(): Argument #1 ($str) must be of type string, int given. Does this mean 42 is not able to be safely cast to a string?Fogarty
It seems, with strlen anyway, I only need to worry about NULL values as it works without warnings or errors with integers. Is this accurate?Fogarty
@Fogarty Again, that's what the strict_types setting is for - in the default mode, it casts int 42 to string '42' and carries on; in strict_types=1 mode, it throws an error. That's why it's called "strict", because it's stricter about what you are allowed to do.Plaid
Thanks. My confusion is my understanding over php 8 enforcing strict types by default. It seems it is not always so. phpWatch says: In PHP 8, internal function parameters have types and value validations enforced, and will throw \TypeError or \ValueError exceptions if the expected type or value is not allowed. Fogarty
@Fogarty No idea what that quote is referring to, but AFAIK, the fundamental behaviour of scalar types hasn't changed, and still depends on the strict_types setting like it has since 7.0. The recent change, and the subject of this question, is the handling of null values, not integers to strings etc. This isn't the right place to discuss anything else, so if you have a different question, ask it on a new page.Plaid
M
11

To answer the bit about the "easiest most time efficient way to handle upgrading this code".

In short, you can't.


First, some background...

Roughly 15% of developers use strict_types=1, so you're in the majority of developers that don't.

You could ignore this problem (deprecation) for now, but PHP 9.0 will cause a lot of problems by making this a Fatal Type Error.

That said, you can still concatenate a string with NULL:

$name = NULL;
$a = 'Hi ' . $name;

And you can still compare NULL to an empty string:

if ('' == NULL) {
}

And you can still do calculations with NULL (it's still treated as 0):

var_dump(3 + '5' + NULL); // Fine, int(8)
var_dump(NULL / 6); // Fine, int(0)

And you can still print/echo NULL:

print(NULL);
echo NULL;

And you can still pass NULL into sprintf() and have that coerced to an empty string with %s, e.g.

sprintf('%s', NULL);

And you can still coerce other values (following the rules), e.g.

strlen(15);
htmlspecialchars(1.2);
setcookie('c', false);

NULL coercion has worked like this since, I assume the beginning, and is also documented:

  • To String: “null is always converted to an empty string.”
  • To Integer: “null is always converted to zero (0).”
  • To Float: “For values of other types, the conversion is performed by converting the value to int first and then to float”
  • To Boolean: “When converting to bool, the following values are considered false [...] the special type NULL”

Anyway, to fix... the first part it trying to find the code you will need to update.

This happens any time where NULL could be passed to one of these function parameters.

There are at least 335 parameters affected by this.

There is an additional 104 which are a bit questionable; and 558 where NULL is problematic, where you should fix those, e.g. define(NULL, 'value').

Psalm is the only tool I can find that's able to help with this.

And Psalm needs to be at a very high checking level (1, 2, or 3).

And you cannot use a baseline to ignore problems (a technique used by developers who have introduced static analysis to an existing project, so it only checks new/edited code).

If you haven't used static analysis tools before (don't worry, it's suggested only 33% of developers do); then expect to spend a lot of time modifying your code (start at level 8, the most lenient, and slowly work up).

I could not get PHPStan, Rector, PHP CodeSniffer, PHP CS Fixer, or PHPCompatibility to find these problems (results); and Juliette has confirmed that getting PHPCompatibility to solve this problem will be "pretty darn hard to do" because it's "not reliably sniffable" (source).


Once you have found every single problem, the second part is editing.

The least likely place to cause problems is by changing the sinks, e.g.

example_function(strval($name));
example_function((string) $name);
example_function($name ?? '');

Or, you could try tracking back to the source of the variable, and try to stop it being set to NULL in the first place.

These are some very common sources of NULL:

$search = (isset($_GET['q']) ? $_GET['q'] : NULL);
 
$search = ($_GET['q'] ?? NULL); // Fairly common (since PHP 7)
 
$search = filter_input(INPUT_GET, 'q');
 
$search = $request->input('q'); // Laravel
$search = $request->get('q'); // Symfony
$search = $this->request->getQuery('q'); // CakePHP
$search = $request->getGet('q'); // CodeIgniter
 
$value = mysqli_fetch_row($result);
$value = json_decode($json); // Invalid JSON, or nesting limit.
$value = array_pop($empty_array);

Some of these functions take a second parameter to specify what the default should be, or you could use strval() earlier... but be careful, your code may specifically check for NULL via ($a === NULL), and you don't want to break that.

Many developers will not be aware that some of their variables can contain NULL - e.g. expecting a <form> (they created) to always submit all of the input fields; that might not happen due to network issues, browser extensions, user editing the DOM/URL in their browser, etc.


I've been looking at this problem for the best part of a year.

I started writing up two RFC's to try and address this problem. The first was to update some of the functions to accept NULL (wasn't ideal, as it upset the developers who used strict_types); and the second RFC was to allow NULL to continue being coerced in this context... but I didn't put it to the vote, as I just got a load of negative feedback, and I didn't want that rejection to be quoted in the future as to why this problem cannot be fixed (while the original change was barely discussed, this one would be).

It seems NULL is treated differently because it was never considered a "scalar value" - I don't think many developers care about this distinction, but it comes up every now and again.

With the developers I've been working with, a most have just ignored this one (hoping it will be resolved later, which is probably not the best idea); e.g.

function ignore_null_coercion($errno, $errstr) {
  // https://github.com/php/php-src/blob/012ef7912a8a0bb7d11b2dc8d108cc859c51e8d7/Zend/zend_API.c#L458
  if ($errno === E_DEPRECATED && preg_match('/Passing null to parameter #.* of type .* is deprecated/', $errstr)) {
    return true;
  }
  return false;
}
set_error_handler('ignore_null_coercion', E_DEPRECATED);

And one team is trying to stick strval() around everything, e.g. trim(strval($search)). But they are still finding problems over a year later (they stated testing with 8.1 alpha 1).

One other option I'm considering is to create a library that re-defines all of these ~335 functions as nullable, under a namespace; e.g.

namespace allow_null_coercion;

function strlen(?string $string): int {
    return \strlen(\strval($string));
}

Then developers would include that library, and use the namespace themselves:

namespace allow_null_coercion;

$search = $request->input('q'); // Could return NULL

// ...

echo strlen($search);
Magazine answered 14/10, 2022 at 13:15 Comment(2)
Your second RFC makes lots of sense to me. All the problems you described on NULL coercion are happening to the OpenMage project. We started work on PHP 8 .1 compatibility since last year but the NULL coercion issues keep popping out in unexpected places. For old projects that was based on loose typing, it's not easy to upgrade PHP. So it's unfortunate that your RFC is not moving forward.Abdias
maybe keep the existing behavior and have a namespace for devs who dont want null coercion.. so namespace will be dont_allow_null_coercion. The rest of us can live happily. Even better maybe call lthe new namespace This_is_not_php_This_is_java_namespaceAdam
P
6

If you're explicitly trying to handle the case of null, then a slightly cleaner fix would be strlen($row ?? '') using the "null coalescing operator".

In most cases, the two are probably equivalent but with strict_types=1 in effect, they behave differently if the value is some other type that can be cast to string:

declare(strict_types=1);
$row = 42;
echo strlen($row); // TypeError: must be of type string, int given
echo strlen((string) $row); // Succeeds, outputting '2'
echo strlen($row ?? ''); // TypeError: must be of type string, int given

On the other hand, note that the ?? operator is based on isset, not === null, so an undefined variable will behave differently:

declare(strict_types=1);
$row = [];
echo strlen($row['no_such_key']); // Warning: Undefined array key; TypeError: must be of type string, null given
echo strlen((string) $row['no_such_key']); // Warning: Undefined array key; outputs '0'
echo strlen($row['no_such_key'] ?? ''); // No warning, just outputs '0'

If you care about that case, the most directly equivalent code to the old behaviour is rather more verbose:

echo strlen($row === null ? '' : $row);
Plaid answered 5/1, 2022 at 10:6 Comment(7)
Thanks for the detailed use case scenario. I noticed that without declare(strict_types=1); passing an int did not throw a warning with 8.1. Do you know why? It seems that php 8 is not enforcing strict typing in all cases.Fogarty
@Fogarty That's exactly what the strict_types declaration is for - if the value can be safely cast to a string, it's allowed. A better name would be "scalar_types=cast" for the default mode, and "scalar_types=error" for the "strict" mode. That hasn't changed in 8.1, only the handling of null, which was never affected by that setting.Plaid
However, with strict_types declared, I get Fatal error: Uncaught TypeError: strlen(): Argument #1 ($str) must be of type string, int given. Does this mean 42 is not able to be safely cast to a string?Fogarty
It seems, with strlen anyway, I only need to worry about NULL values as it works without warnings or errors with integers. Is this accurate?Fogarty
@Fogarty Again, that's what the strict_types setting is for - in the default mode, it casts int 42 to string '42' and carries on; in strict_types=1 mode, it throws an error. That's why it's called "strict", because it's stricter about what you are allowed to do.Plaid
Thanks. My confusion is my understanding over php 8 enforcing strict types by default. It seems it is not always so. phpWatch says: In PHP 8, internal function parameters have types and value validations enforced, and will throw \TypeError or \ValueError exceptions if the expected type or value is not allowed. Fogarty
@Fogarty No idea what that quote is referring to, but AFAIK, the fundamental behaviour of scalar types hasn't changed, and still depends on the strict_types setting like it has since 7.0. The recent change, and the subject of this question, is the handling of null values, not integers to strings etc. This isn't the right place to discuss anything else, so if you have a different question, ask it on a new page.Plaid
E
0

Rector is probably your best bet for a quick resolution. It has the rule NullToStrictStringFuncCallArgRector to fix this, and it fixes it by casting it to a string as well:

-     mb_strtolower($value);
+     mb_strtolower((string) $value);
Echevarria answered 8/12, 2022 at 13:2 Comment(0)
A
0

A quick way to upgrade your code would be to replace the calls to the core functions with calls to your own functions that in turn would do the necessary checking.

example:

Replace all calls to 'strlen' with 'mystrlen' and code your 'mystrlen' function as follows:

function mystrlen ($str){
    return strlen ($str ?? "");
}
Adam answered 14/8 at 8:31 Comment(0)

© 2022 - 2024 — McMap. All rights reserved.