str_replace does not allow a null replacement

[greg0ire]

Working php code Psalm is not happy with: https://psalm.dev/r/89bd6bab25
I would fix it but I could not find where Psalm gets its info from.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/89bd6bab25

<?php

echo str_replace('a', null, 'abc');

Psalm output (using commit e1c6fcc):

ERROR: NullArgument - 3:23 - Argument 2 of str_replace cannot be null, null value provided to parameter with type array<array-key, mixed>|string

[weirdan]

Looks like a sufficiently bad code that should be fixed rather than allowed.

[greg0ire]

I agree that it's bad in that it would not have crossed my mind to write it that way, but it feels like Psalm is overstepping here... this looks like something that phpcs would forbid, but not Psalm. Psalm is here to find bugs, and this works perfectly, doesn't it?

[weirdan]

That's debatable. On one hand 'Psalm is not PhpCS' is established position, I had the issues reported by me closed with that justification. On the other hand Psalm rejects a lot of perfectly working code (see all those Mixed* issues), so it's not here to only prevent bugs. It's here to make you write better code, and does this mainly by enforcing type correctness. And you're passing null (which is distinct type in PHP) where string[]|string is expected.

Besides, it's easy to fix: https://psalm.dev/r/1d16bdfe2a

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/1d16bdfe2a

<?php

echo str_replace('a', (string)null, 'abc');

Psalm output (using commit e1c6fcc):

No issues!

[greg0ire]

And you're passing null (which is distinct type in PHP) where string[]|string is expected.

Again, I did not write this :P , but if I had to fix it, I would use echo str_replace('a', '', 'abc');

I'm afraid that if I change this, people are going to go pedantic and tell me that nothing in the docs says that replacement as to be string[]|string

On the other hand Psalm rejects a lot of perfectly working code (see all those Mixed* issues), so it's not here to only prevent bugs.

I didn't know that was the case, closing, then, thank you.

[weirdan]

I'm afraid that if I change this, people are going to go pedantic and tell me that nothing in the docs says that replacement as to be string[]|string

Apart from the function description which reads 'Replace all occurrences of the search string with the replacement string' (https://www.php.net/str_replace).

If that doesn't work to convince them, invoke @Ocramius 🤣

[greg0ire]

Well the repo at hand is doctrine/dbal, so … should work I guess :P

[Ocramius]

should be fixed: use a (string) cast on that null, and pretend to never have seen that null also ever worked.

[greg0ire]

It's fixed, see the linked commit above ;)