jsonmapper icon indicating copy to clipboard operation
jsonmapper copied to clipboard

Do not accept null in typed arrays

Open dktapps opened this issue 1 year ago • 5 comments

this behaviour is completely unexpected, and no static analyzer would spot it. null ought to be treated like all other simple types.

dktapps avatar Jul 14 '23 10:07 dktapps

Can you add a bit more context and explain the change you are proposing? Currently you are referring to "this behaviour" without explicitly stating which.

What do you do? What do you get? What do you want instead as the consumer of this library?

SvenRtbg avatar Jul 14 '23 12:07 SvenRtbg

I thought the unit test changes would adequately explain the problem, but here goes:

In my project, a model property of type string[] was expected to contain exclusively string. However, jsonmapper allows the property to contain null elements, which can lead to crashes in strict code, and is not detectable by any static analyser.

For example:

$mapper = new \JsonMapper;
$mapper->bExceptionOnMissingData = true;
$mapper->bExceptionOnUndefinedProperty = true;
$mapper->bEnforceMapType = false;
$chainData = $mapper->map(["chain" => [null,"hello"]], new JwtChain);

with the following model:

final class JwtChain{

	/**
	 * @var string[]
	 * @required
	 */
	public array $chain;
}

should either coerce the NULL to "", or bail out due to incompatible types (string[] should not accept null).

I opted for coercion since it's the same behaviour already applied to every scalar type.

dktapps avatar Jul 14 '23 12:07 dktapps

You modify testMapTypedSimpleArray(), which explicitly tested that an incoming array of strings may contain a null value, and will lead to a null value in the generated array of objects. Changing this behavior will break many existing applications that rely on it.

I do see that this behavior is ill-suited for some use cases, though. In hindsight the better way would have been to allow null in arrays only when the type said it, e.g. ?ClassName[] (a syntax that did not exist seven years ago).

I will not modify the default existing behavior, but adding a new switch (e.g. strict null checks in typed arrays) will be fine.

(bStrictNullTypes was originally meant for such things, but because of BC I won't change that.)

cweiske avatar Aug 01 '23 08:08 cweiske

In PHPDoc the more idiomatic way to describe the type would be @var (string|null)[] or @var (ClassName|null)[].

I will not modify the default existing behavior, but adding a new switch (e.g. strict null checks in typed arrays) will be fine.

I could live with that solution, although perhaps some factory method that sets some properties by default would be useful (I currently always use strictProperties and some other things)

dktapps avatar Aug 01 '23 09:08 dktapps

Will you provide a patch?

cweiske avatar Oct 24 '23 07:10 cweiske

Closing because of no answer in 3 months.

cweiske avatar Jan 27 '24 14:01 cweiske