-
Couldn't load subscription status.
- Fork 544
Introduce allowFloatBoolNullAsArrayKey parameter #4012
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
f56eb92 to
011505b
Compare
011505b to
154bb87
Compare
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.
Also would be nice if it observed rule levels. Always wrong type like null to be always reported, but a partially wrong type like string|stdClass only on level 7+...
This is achieved with RuleLevelHelper::findTypeToCheck.
692ddc0 to
a5aea3c
Compare
Seems like the RuleLevelHelper::findTypeToCheck was not used at all (or wrongly used) on theses rules.
So I fixed it and added tests with and without the reportArrayKeyCast option.
a5aea3c to
ef97675
Compare
Current status, waiting #4166 first
627be15 to
4476b7a
Compare
This pull request has been marked as ready for review.
Friendly ping @ondrejmirtes ; I would be interested enabling this on phpstan-strict-rules :)
4476b7a to
02fd76b
Compare
18f2240 to
89f4394
Compare
So now this is only about the boolean. I don't think it's worth it adding a new config parameter just for that. Maybe it'd be better to advocate for deprecating that in PHP 8.6?
So now this is only about the boolean. I don't think it's worth it adding a new config parameter just for that.
It's for boolean or people which are using PHP < 8.5 (8.1) and still want the array key casted deprecated.
Since PHP supports 7.4 it might have some benefit.
Also I feel like waiting for 8.6 might be a little bit long for an important bug like the example of phpstan/phpstan#12589 ...
Maybe it'd be better to advocate for deprecating that in PHP 8.6?
It's would still be a good idea but I have no knowledge in C to implement this in PHP codebase
So deprecating bools was considered but rejected in the RFC process. Here's the comment and some discussion above it https://externals.io/message/127849#128184.
So deprecating bools was considered but rejected in the RFC process. Here's the comment and some discussion above it externals.io/message/127849#128184.
Then based on the fact it will be unlikely deprecated soon on PHP side, do you confirm that such options still make sens on PHPStan side ?
This pull request has been marked as ready for review.
I don't like the name of the config parameter. You're not reporting array key casts. For example if someone uses '1', you're not reporting that although it's still cast to 1.
This is more like allowFloatBoolNullAsArrayKey with default set to true.
This is more like
allowFloatBoolNullAsArrayKeywith default set totrue.
I renamed with allowFloatBoolNullAsArrayKey, but have some suggestions:
- allowBoolFloatNullAsArrayKey => Type are alphabetically ordered
- allowLooseArrayKeys
- strictArrayKeyTypes (default false)
Any preferences ?
ca192db to
a56ac23
Compare
Just found the issue phpstan/phpstan#7564 and I wonder if it should be included in this option or in a separate one.
If so, rather than introducing allowFloatBoolNull param in AllowedArrayKeysTypes it could be something like checking if
$type->toArrayKeyType()->isSuperTypeOf($type)->yes();
(to avoid reporting numeric-string array keys)
a56ac23 to
5aeddaa
Compare
This is more like
allowFloatBoolNullAsArrayKeywith default set totrue.I renamed with
allowFloatBoolNullAsArrayKey, but have some suggestions:
- allowBoolFloatNullAsArrayKey => Type are alphabetically ordered
- allowLooseArrayKeys
- strictArrayKeyTypes (default false)
Any preferences ?
Maybe you'll have some suggestion about the naming @staabm ?
I would go with one of the allowFloatBoolNullAsArrayKey variants, as otherwise we need to document somewhere what "loose" or "strict" means in this context
Uh oh!
There was an error while loading. Please reload this page.
Closes phpstan/phpstan#12589
Closes phpstan/phpstan#7884
Closes phpstan/phpstan#7864
Naming is opened to discussion.
I think we could enable this in phpstan-strict-rules.