Skip to content

Conversation

@LordSimal
Copy link
Contributor

No description provided.

@LordSimal LordSimal added this to the 3.x milestone Jan 21, 2024

$name = $arguments->getArgument('name');
if (empty($name)) {
if ($name === null || $name === '') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is better but psalm thinks it is 🤷🏻

Copy link
Member

Choose a reason for hiding this comment

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

if (!$name) was what I had in mind :)

Copy link
Contributor Author

@LordSimal LordSimal Jan 21, 2024

Choose a reason for hiding this comment

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

but psalm is not happy about that one as I described in core chat.

Because then null and '' would be handled in the same if which "could lead to unexpected behavior" according to psalm.

Copy link
Member

Choose a reason for hiding this comment

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

Thus the silencing on top of it :)

Copy link
Member

@dereuromark dereuromark Jan 21, 2024

Choose a reason for hiding this comment

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

Otherwise you would have to use

/** @var non-empty-string|null $name */
$name = ...
if ($name === null) 

if you wanted to make this clean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well If I just look at https://www.php.net/manual/en/types.comparisons.php then we didn't really do anything here other than just suppress the warning...

@dereuromark dereuromark merged commit b0b9b12 into 3.x Jan 21, 2024
@dereuromark dereuromark deleted the 3.x-stan-update branch January 21, 2024 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants