Fix "Crash: str_increment(): Argument #1 ($string) must be composed only of alphanumeric ASCII characters"

[staabm]

I thought about different variants on how this could be solved.
most important point IMO is we don't crash PHPStan on unexpected input.

I think it makes sense to report a ERROR type for PHP 8.3+ (so when function_exists('str_increment'); and PHP started to emit deprecations) when invalid input is provided.

closes phpstan/phpstan#13481

[phpstan-bot]

This pull request has been marked as ready for review.

[samsonasik]

str_increment() call exists in StrIncrementDecrementFunctionReturnTypeExtension as well

phpstan-src/src/Type/Php/StrIncrementDecrementFunctionReturnTypeExtension.php

Line 114 in e9556fb

return str_increment($s);

probably worth check there as well?

[staabm]

The added test covers this line

[VincentLanglet]

This feels kinda weird because

• On PHP 8.3 & 8.4 we return ErrorType while it's still valid code (even if deprecated)
• On PHP 8.5+ we're still returning '1' for ''++ while it's invalid code

Should we introduce a method inside PHPVersion class ?

Then you could keep @++$varValue for PHPVersion <= 8.5

[staabm]

this was one of the variants I considered. the ++$varValue is consistent across all versions (it differs only by emitting a deprecation warning).

if we are fine with swalling the deprecation warning the impl could be simplified.

I think we wouldn't even need per version expectations

[ondrejmirtes]

I wonder: how come we only use str_increment in our code but not str_decrement at all? I feel like places doing -- possibly on strings should also have similar conditions around that code.

[ondrejmirtes]

Forgot to link it - I tested it and -- is deprecated too https://3v4l.org/q5Fp3/rfc#vgit.master

[staabm]

what do you think about swallowing the deprecations as there is currently no need to use the str_increment/derecment functions..?
(and these are no good replacement)

[ondrejmirtes]

What's wrong with calling them and catching ValueError?

[staabm]

its "a lot of work" without a gain

it will also lead to different results depending on the used php runtime version

[VincentLanglet]

What's wrong with calling them and catching ValueError?

The only wrong thing I see is the fact that we get a ValueError on PHP 8.3/8.4 (and considered the result as ErrorType) while ++ and -- are supposed to be still valid (and working, even if deprecated).

[ondrejmirtes]

Do you think this will work or am I missing something? e4c5865

[staabm]

Do you think this will work or am I missing something?

as is I think it misses some cases, e.g. ++'ab c1' (this PRs test)

[ondrejmirtes]

I'll cherry pick the tests from here and we'll see. Right now I'm fixing the tests which I forgot to run before pushing the commit 🤦

[ondrejmirtes]

See 392e58d - I know it's not perfect and it's a weird quirk that for some strings ++/-- keeps them the same but str_increment throws a ValueError, but I think I can live with this now.