-
-
Notifications
You must be signed in to change notification settings - Fork 409
Add Rector to replace null keys in array_key_exists with empty string #7180
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
Add Rector to replace null keys in array_key_exists with empty string #7180
Conversation
if (! $node instanceof FuncCall) { | ||
return null; | ||
} | ||
|
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.
add skip early $node->isFirstClassCallable()
early here
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.
Ok.
return null; | ||
} | ||
|
||
if ($this->isName($keyExpr, 'null')) { |
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.
use valueResolver->isNull instead, ensure exactly null variable, not variable named null
return $node; | ||
} | ||
|
||
return $node; |
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.
return null ?
CODE_SAMPLE | ||
, | ||
<<<'CODE_SAMPLE' | ||
array_key_exists($key ?? '', $array); |
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 prefer check type, if not string, then use (string)
cast instead, see other rule
final class NullToStrictStringFuncCallArgRector extends AbstractRector implements MinPhpVersionInterface |
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.
Since the logic seems similar, I think you can create re-usable service to be used in both NullToStrictStringFuncCallArgRector
and this rule so it share similar behaviour, just different php version and function.
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.
Does it need Rector\Php85\Enum\NameNullToStrictNullFunctionMap
, and should only array_key_exists
be added?
Or without Rector\Php85\Enum\NameNullToStrictNullFunctionMap
?
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.
for just single function, no need to add to enum imo
* @param Arg[] $args | ||
* @param int|string $position | ||
*/ | ||
private function processNullToStrictStringOnNodePosition( |
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 method looks duplicated with NullToStrictStringFuncCallArgRector
, can be moved to shared service imo
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 think I should create a class like this, right?
final class NullToStrictStringConverter
{
public function convertIfNull(Node $expr): Node
{}
}
Where should I put it?”
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.
Yes, on Php81 namespace since it start there
https://github.com/rectorphp/rector-src/tree/main/rules/Php81/NodeManipulator
PHP 8.5+ deprecates passing null as the key to
array_key_exists()
.This Rector rule automatically replaces:
with:
https://wiki.php.net/rfc/deprecations_php_8_5