-
Notifications
You must be signed in to change notification settings - Fork 533
Fix "Crash: str_increment(): Argument #1 ($string) must be composed only of alphanumeric ASCII characters" #4316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
...nly of alphanumeric ASCII characters"
c957182 to
466bed0
Compare
This pull request has been marked as ready for review.
str_increment() call exists in StrIncrementDecrementFunctionReturnTypeExtension as well
probably worth check there as well?
The added test covers this line
src/Analyser/MutatingScope.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Forgot to link it - I tested it and -- is deprecated too https://3v4l.org/q5Fp3/rfc#vgit.master
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)
What's wrong with calling them and catching ValueError?
its "a lot of work" without a gain
it will also lead to different results depending on the used php runtime version
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).
Do you think this will work or am I missing something? e4c5865
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)
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 🤦
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.
Uh oh!
There was an error while loading. Please reload this page.
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