-
-
Notifications
You must be signed in to change notification settings - Fork 344
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
Detecting duplicated array keys #350
Detecting duplicated array keys #350
Conversation
3184b8f
to
5570e3e
Compare
@rafalwrzeszcz thanks for the PR. About the Ruleset, I think CleanCode or UnusedCode would be a better fit, as duplicated array keys have nothing to do with software design. https://phpmd.org/rules/index.html https://phpmd.org/rules/index.html#clean-code-rules |
AppVeyor fails:
|
@ravage84 Moved to clean code. Any clue why AppVeyor is failing? The same test suite runs fine on Scrutinizer and Travis (and also locally). It may be PHP7-related, but Travis build for PHP7 succeeds. |
0c69edd
to
48e6db8
Compare
Nope, no idea. Can you try to execute the tests on a Windows machine with PHP7? |
@@ -0,0 +1,8 @@ | |||
<?php | |||
|
|||
class testRuleNotAppliesToMethodWithoutArrayDefinition |
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.
Not sure, if that's really necessary.
Would your code be able to detect duplicated array keys, if one would use a constant or class constant as the array key(s)? For example:
or mixed:
|
@ravage84 No and I don't think it's feasible in a reasonable time/PR size - constant can come from outside of the file or even a library. But it should detect keys defined with same constant twice (like |
That's fine for me. Just wanted to check. |
@ravage84 Then it has to wait few days, not sure if I will find some time to set up env for Win before the weekend. |
Take your time. I would like @manuelpichler to have an eye on this anyway. But I know he is very busy lately. |
@rafalwrzeszcz how is your status on this? |
@rafalwrzeszcz can you give me an update on this? |
foreach ($node->findChildrenOfType('ArrayElement') as $arrayElement) { | ||
// member of nested array - will be handled when `apply()` moves to that array | ||
// could be nice if PDepend provides a method to fetch just direct children | ||
if ($arrayElement->getParent() != $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.
This one will end up with:
PHP Fatal error: Nesting level too deep - recursive dependency? in src\main\php\PHPMD\Rule\CleanCode\DuplicatedArrayKey.php on line 87
Comparing objects in PHP is recursive (deep-comparison).
|
||
$children = $arrayElement->getChildren(); | ||
// non-associative array | ||
if (count($children) == 1) { |
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 one could be improved, as given array:
$arr = [
'foo',
'bar',
'' => 'baz',
0 => '20',
'0' => '30',
false => '40',
1 => '50',
'1' => '60',
true => '70',
null => '80',
];
Will result in:
Array
(
[0] => 40
[1] => 70
[] => 80
)
All due to casting.
// numbers don't need any processing, they have plain value | ||
|
||
// this is string literal, not number | ||
if (in_array($key[0], 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.
It can be simplified to trim($key, '\'"');
Closing as superseded by #484 |
This is the implementation fo #322 request.
I added it to
Design
section, any better idea?