Add nullable information to Subscriber ArgumentMetadata#812
Add nullable information to Subscriber ArgumentMetadata#812DanielBadura merged 1 commit intopatchlevel:3.16.xfrom
Conversation
| public function __construct( | ||
| public readonly string $name, | ||
| public readonly string $type, | ||
| public readonly bool $allowsNull, |
There was a problem hiding this comment.
| public readonly bool $allowsNull, | |
| public readonly bool $allowsNull = false, |
The default should be false, as this is the current situation pre this patch. And yes, this is public api. We currently have only one class which is marked internal and thereby is not public api :D
There was a problem hiding this comment.
How about making it default null instead? The reason I opted against null is that this can be false information without means to detect this (not passed does not mean not nullable). Having it null would suggest 'unknown'. But that puts more burden on users to also check for null of course :)
There was a problem hiding this comment.
But what would the behaviour be, if $allowsNull is null? Either it supports nullable values (true) or not (false), i cannot thing of a third option here 🤔
There was a problem hiding this comment.
Behavior would be left to the recipient of the ArgumentMetadata. This class is only about the information we have, null meaning "we do not have it".
But since nullable arguments were not supported before:
- they probably were either not nullable before
- or if nullable, they were never called with null
So I do not really have a strong opinion about this either way :)
There was a problem hiding this comment.
I would go with false as the default value as this was not supported before it is imho the right choice.
b3de7d5 to
53b6e83
Compare
53b6e83 to
c2f249b
Compare
|
Thank you! |
Not sure if to make this argument mandatory, would be a breaking change if this is considered public API. Do not like default here either though (no reasonable default value)