Skip to content

Conversation

@fritz-gerneth
Copy link
Contributor

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)

public function __construct(
public readonly string $name,
public readonly string $type,
public readonly bool $allowsNull,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would go with false as the default value as this was not supported before it is imho the right choice.

@fritz-gerneth fritz-gerneth force-pushed the issue-809 branch 2 times, most recently from b3de7d5 to 53b6e83 Compare February 10, 2026 09:45
@DanielBadura DanielBadura added the enhancement New feature or request label Feb 10, 2026
@DanielBadura DanielBadura added this to the 3.16.0 milestone Feb 10, 2026
@DanielBadura DanielBadura merged commit 52f45dc into patchlevel:3.16.x Feb 10, 2026
40 of 41 checks passed
@DanielBadura
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants