Feature/duplicated array keys #484

eeree · 2017-06-05T00:28:26Z

As #350 stuck for some time and also implementation was a bit buggy (nested arrays recursion issue and boolean/null array keys checks), I've decided to fix what's left.

stickler-ci · 2017-06-05T00:28:42Z

...Key/testRuleAppliesMultipleTimesToMethodWithAssotiativeArrayDefinitionWithDuplicatedKeys.php

+ * @link http://phpmd.org/
+ */
+
+class testRuleAppliesMultipleTimesToMethodWithAssotiativeArrayDefinitionWithDuplicatedKeys


Each class must be in a namespace of at least one level (a top-level vendor name)
Class name "testRuleAppliesMultipleTimesToMethodWithAssotiativeArrayDefinitionWithDuplicatedKeys" is not in camel caps format

stickler-ci · 2017-06-05T00:28:42Z

...plicatedArrayKey/testRuleAppliesToMethodWithAssotiativeArrayDefinitionWithDuplicatedKeys.php

+ * @link http://phpmd.org/
+ */
+
+class testRuleAppliesToMethodWithAssotiativeArrayDefinitionWithDuplicatedKeys


Each class must be in a namespace of at least one level (a top-level vendor name)
Class name "testRuleAppliesToMethodWithAssotiativeArrayDefinitionWithDuplicatedKeys" is not in camel caps format

stickler-ci · 2017-06-05T00:28:42Z

...ayKey/testRuleAppliesToMethodWithAssotiativeArrayDefinitionWithDuplicatedMixedQuotedKeys.php

+ * @link http://phpmd.org/
+ */
+
+class testRuleAppliesToMethodWithAssotiativeArrayDefinitionWithDuplicatedMixedQuotedKeys


Each class must be in a namespace of at least one level (a top-level vendor name)
Class name "testRuleAppliesToMethodWithAssotiativeArrayDefinitionWithDuplicatedMixedQuotedKeys" is not in camel caps format

stickler-ci · 2017-06-05T00:28:43Z

...rrayKey/testRuleAppliesToMethodWithAssotiativeArrayDefinitionWithDuplicatedMixedTypeKeys.php

+ * @link http://phpmd.org/
+ */
+
+class testRuleAppliesToMethodWithAssotiativeArrayDefinitionWithDuplicatedMixedTypeKeys


Each class must be in a namespace of at least one level (a top-level vendor name)
Class name "testRuleAppliesToMethodWithAssotiativeArrayDefinitionWithDuplicatedMixedTypeKeys" is not in camel caps format

stickler-ci · 2017-06-05T00:28:43Z

...files/Rule/CleanCode/DuplicatedArrayKey/testRuleAppliesWhenKeyIsDeclaredInNonStandardWay.php

+    return 'foo';
+}
+
+class testRuleAppliesWhenKeyIsDeclaredInNonStandardWay extends testRuleAppliesWhenKeyIsDeclaredInNonStandardWayParent


Class name "testRuleAppliesWhenKeyIsDeclaredInNonStandardWay" is not in camel caps format

stickler-ci · 2017-06-05T00:28:43Z

...files/Rule/CleanCode/DuplicatedArrayKey/testRuleAppliesWhenKeyIsDeclaredInNonStandardWay.php

+{
+    const DUPLICATED_KEY = 'foo';
+
+    static $classStaticPropertyDuplicate = 'foo';


Visibility must be declared on property "$classStaticPropertyDuplicate"

stickler-ci · 2017-06-05T00:28:43Z

...files/Rule/CleanCode/DuplicatedArrayKey/testRuleAppliesWhenKeyIsDeclaredInNonStandardWay.php

+    }
+}
+
+interface testRuleAppliesWhenKeyIsDeclaredInNonStandardWayInterface


Each class must be in a file by itself
Interface name "testRuleAppliesWhenKeyIsDeclaredInNonStandardWayInterface" is not in camel caps format

stickler-ci · 2017-06-05T00:28:44Z

...files/Rule/CleanCode/DuplicatedArrayKey/testRuleAppliesWhenKeyIsDeclaredInNonStandardWay.php

+    const INTERFACE_DUPLICATED_KEY = 'foo';
+}
+
+abstract class testRuleAppliesWhenKeyIsDeclaredInNonStandardWayAbstract


Each interface must be in a file by itself
Class name "testRuleAppliesWhenKeyIsDeclaredInNonStandardWayAbstract" is not in camel caps format

stickler-ci · 2017-06-05T00:28:44Z

...files/Rule/CleanCode/DuplicatedArrayKey/testRuleAppliesWhenKeyIsDeclaredInNonStandardWay.php

+}
+
+abstract class testRuleAppliesWhenKeyIsDeclaredInNonStandardWayAbstract
+    implements testRuleAppliesWhenKeyIsDeclaredInNonStandardWayInterface


The implements keyword must be on the same line as the class name

stickler-ci · 2017-06-05T00:28:44Z

...files/Rule/CleanCode/DuplicatedArrayKey/testRuleAppliesWhenKeyIsDeclaredInNonStandardWay.php

+    const DUPLICATED_KEY = 'foo';
+}
+
+class testRuleAppliesWhenKeyIsDeclaredInNonStandardWayParent


Each class must be in a file by itself
Class name "testRuleAppliesWhenKeyIsDeclaredInNonStandardWayParent" is not in camel caps format

stickler-ci · 2017-06-05T00:28:44Z

...files/Rule/CleanCode/DuplicatedArrayKey/testRuleAppliesWhenKeyIsDeclaredInNonStandardWay.php

+}
+
+class testRuleAppliesWhenKeyIsDeclaredInNonStandardWayParent
+    extends testRuleAppliesWhenKeyIsDeclaredInNonStandardWayAbstract


The extends keyword must be on the same line as the class name

stickler-ci · 2017-06-05T00:28:45Z

...edArrayKey/testRuleNotAppliesToMethodWithAssotiativeArrayDefinitionWithoutDuplicatedKeys.php

+ * @link http://phpmd.org/
+ */
+
+class testRuleNotAppliesToMethodWithAssotiativeArrayDefinitionWithoutDuplicatedKeys


Each class must be in a namespace of at least one level (a top-level vendor name)
Class name "testRuleNotAppliesToMethodWithAssotiativeArrayDefinitionWithoutDuplicatedKeys" is not in camel caps format

stickler-ci · 2017-06-05T00:28:45Z

...CleanCode/DuplicatedArrayKey/testRuleNotAppliesToMethodWithNonAssotiativeArrayDefinition.php

+ * @link http://phpmd.org/
+ */
+
+class testRuleNotAppliesToMethodWithNonAssotiativeArrayDefinition


Each class must be in a namespace of at least one level (a top-level vendor name)
Class name "testRuleNotAppliesToMethodWithNonAssotiativeArrayDefinition" is not in camel caps format

stickler-ci · 2017-06-05T00:28:45Z

...files/Rule/CleanCode/DuplicatedArrayKey/testRuleNotAppliesToMethodWithoutArrayDefinition.php

+ * @link http://phpmd.org/
+ */
+
+class testRuleNotAppliesToMethodWithoutArrayDefinition


Each class must be in a namespace of at least one level (a top-level vendor name)
Class name "testRuleNotAppliesToMethodWithoutArrayDefinition" is not in camel caps format

stickler-ci · 2017-06-05T00:44:01Z

...Key/testRuleAppliesMultipleTimesToMethodWithAssotiativeArrayDefinitionWithDuplicatedKeys.php

+
+namespace PHPMDTest;
+
+class testRuleAppliesMultipleTimesToMethodWithAssotiativeArrayDefinitionWithDuplicatedKeys


Class name "testRuleAppliesMultipleTimesToMethodWithAssotiativeArrayDefinitionWithDuplicatedKeys" is not in camel caps format

stickler-ci · 2017-06-05T00:44:02Z

...plicatedArrayKey/testRuleAppliesToMethodWithAssotiativeArrayDefinitionWithDuplicatedKeys.php

+
+namespace PHPMDTest;
+
+class testRuleAppliesToMethodWithAssotiativeArrayDefinitionWithDuplicatedKeys


Class name "testRuleAppliesToMethodWithAssotiativeArrayDefinitionWithDuplicatedKeys" is not in camel caps format

stickler-ci · 2017-06-05T00:44:02Z

...ayKey/testRuleAppliesToMethodWithAssotiativeArrayDefinitionWithDuplicatedMixedQuotedKeys.php

+
+namespace PHPMDTest;
+
+class testRuleAppliesToMethodWithAssotiativeArrayDefinitionWithDuplicatedMixedQuotedKeys


Class name "testRuleAppliesToMethodWithAssotiativeArrayDefinitionWithDuplicatedMixedQuotedKeys" is not in camel caps format

stickler-ci · 2017-06-05T00:44:03Z

...rrayKey/testRuleAppliesToMethodWithAssotiativeArrayDefinitionWithDuplicatedMixedTypeKeys.php

+
+namespace PHPMDTest;
+
+class testRuleAppliesToMethodWithAssotiativeArrayDefinitionWithDuplicatedMixedTypeKeys


Class name "testRuleAppliesToMethodWithAssotiativeArrayDefinitionWithDuplicatedMixedTypeKeys" is not in camel caps format

stickler-ci · 2017-06-05T00:44:03Z

...files/Rule/CleanCode/DuplicatedArrayKey/testRuleAppliesWhenKeyIsDeclaredInNonStandardWay.php

+    const INTERFACE_DUPLICATED_KEY = 'foo';
+}
+
+abstract class testRuleAppliesWhenKeyIsDeclaredInNonStandardWayAbstract implements


Each interface must be in a file by itself
Class name "testRuleAppliesWhenKeyIsDeclaredInNonStandardWayAbstract" is not in camel caps format

stickler-ci · 2017-06-05T00:44:03Z

...files/Rule/CleanCode/DuplicatedArrayKey/testRuleAppliesWhenKeyIsDeclaredInNonStandardWay.php

+    const DUPLICATED_KEY = 'foo';
+}
+
+class testRuleAppliesWhenKeyIsDeclaredInNonStandardWayParent extends testRuleAppliesWhenKeyIsDeclaredInNonStandardWayAbstract


Each class must be in a file by itself
Class name "testRuleAppliesWhenKeyIsDeclaredInNonStandardWayParent" is not in camel caps format
Line exceeds 120 characters; contains 125 characters

stickler-ci · 2017-06-05T00:44:04Z

...edArrayKey/testRuleNotAppliesToMethodWithAssotiativeArrayDefinitionWithoutDuplicatedKeys.php

+
+namespace PHPMDTest;
+
+class testRuleNotAppliesToMethodWithAssotiativeArrayDefinitionWithoutDuplicatedKeys


Class name "testRuleNotAppliesToMethodWithAssotiativeArrayDefinitionWithoutDuplicatedKeys" is not in camel caps format

stickler-ci · 2017-06-05T00:44:04Z

...CleanCode/DuplicatedArrayKey/testRuleNotAppliesToMethodWithNonAssotiativeArrayDefinition.php

+
+namespace PHPMDTest;
+
+class testRuleNotAppliesToMethodWithNonAssotiativeArrayDefinition


Class name "testRuleNotAppliesToMethodWithNonAssotiativeArrayDefinition" is not in camel caps format

stickler-ci · 2017-06-05T00:44:05Z

...files/Rule/CleanCode/DuplicatedArrayKey/testRuleNotAppliesToMethodWithoutArrayDefinition.php

+
+namespace PHPMDTest;
+
+class testRuleNotAppliesToMethodWithoutArrayDefinition


Class name "testRuleNotAppliesToMethodWithoutArrayDefinition" is not in camel caps format

ravage84

Please add a test with two or more arrays with the same duplicated array keys
And change some of the tests that not all return, at least one should assign the content of the array to a variable or whatever.

ravage84 · 2017-06-06T00:13:54Z

...Key/testRuleAppliesMultipleTimesToMethodWithAssotiativeArrayDefinitionWithDuplicatedKeys.php

@@ -2,7 +2,7 @@

 class testRuleAppliesMultipleTimesToMethodWithAssotiativeArrayDefinitionWithDuplicatedKeys
 {
-    function testRuleAppliesMultipleTimesToMethodWithAssotiativeArrayDefinitionWithDuplicatedKeys()
+    public function testRuleAppliesMultipleTimesToMethodWithAssotiativeArrayDefinitionWithDuplicatedKeys()


You shouldn't do this just to satisfy some CI, but this actually communicates intention explicitly.

ravage84 · 2017-06-06T00:18:20Z

src/main/php/PHPMD/Rule/CleanCode/DuplicatedArrayKey.php

+ * @license https://opensource.org/licenses/bsd-license.php BSD License
+ * @link http://phpmd.org/
+ */
+


Superfluous line

ravage84 · 2017-06-06T00:20:06Z

src/main/php/PHPMD/Rule/CleanCode/DuplicatedArrayKey.php

+/**
+ * Duplicated Array Key Rule
+ *
+ * This rule detects if array literal has duplicated entries for any key.


This rule detects duplicated array keys.

ravage84 · 2017-06-06T00:22:29Z

src/main/php/PHPMD/Rule/CleanCode/DuplicatedArrayKey.php

+class DuplicatedArrayKey extends AbstractRule implements MethodAware, FunctionAware
+{
+    /**
+     * This method checks if a given function or method contains an array literal


Overly complicated: "This method checks if a given function or method contains an array which has duplicated keys and emits a rule violation if so.

Actually, this method only loops over it...

ravage84 · 2017-06-06T00:24:20Z

src/main/php/PHPMD/Rule/CleanCode/DuplicatedArrayKey.php

+     * @param PDependASTNode $key
+     * @return string
+     */
+    private function stringFromLiteral(PDependASTNode $key)


castStringFromLiteral

ravage84 · 2017-06-06T00:57:29Z

...files/Rule/CleanCode/DuplicatedArrayKey/testRuleAppliesWhenKeyIsDeclaredInNonStandardWay.php

+            parent::INTERFACE_DUPLICATED_KEY => 'bar', // not applied (to be implemented?)
+            static::INTERFACE_DUPLICATED_KEY => 'bar', // not applied (to be implemented?)
+            $foo => 'bar', // not applied - resolving variables is impossible in this context
+            $baz => 'bar', // not applied - see above


copy comment to this line. It might not be clear where above.

ravage84 · 2017-06-06T00:58:27Z

...files/Rule/CleanCode/DuplicatedArrayKey/testRuleAppliesWhenKeyIsDeclaredInNonStandardWay.php

+            $baz => 'bar', // not applied - see above
+            $this->getDuplicatedName() => 'bar', // not applied - resolving variable depends on inheritance tree
+            self::$classStaticPropertyDuplicate => 'bar', // not applied - static property may be modified externally
+            self::$classPrivateStaticPropertyDuplicate, // not applied - see above


copy comment to this line. It might not be clear where above.

ravage84 · 2017-06-06T00:58:31Z

...files/Rule/CleanCode/DuplicatedArrayKey/testRuleAppliesWhenKeyIsDeclaredInNonStandardWay.php

+            self::$classStaticPropertyDuplicate => 'bar', // not applied - static property may be modified externally
+            self::$classPrivateStaticPropertyDuplicate, // not applied - see above
+            $this->classPrivatePropertyDuplicate => 'bar', // not applied - may be modified at any time
+            $this->classProtectedPropertyDuplicate => 'bar', // not applied - see above


copy comment to this line. It might not be clear where above.

ravage84 · 2017-06-06T00:58:35Z

...files/Rule/CleanCode/DuplicatedArrayKey/testRuleAppliesWhenKeyIsDeclaredInNonStandardWay.php

+            self::$classPrivateStaticPropertyDuplicate, // not applied - see above
+            $this->classPrivatePropertyDuplicate => 'bar', // not applied - may be modified at any time
+            $this->classProtectedPropertyDuplicate => 'bar', // not applied - see above
+            $this->classPublicPropertyDuplicate => 'bar', // not applied - see above


copy comment to this line. It might not be clear where above.

ravage84 · 2017-06-06T00:58:54Z

...files/Rule/CleanCode/DuplicatedArrayKey/testRuleAppliesWhenKeyIsDeclaredInNonStandardWay.php

+            $this->classPublicPropertyDuplicate => 'bar', // not applied - see above
+            global_function_duplicate() => 'bar', // not applied - may be different depending on namespace
+            GLOBAL_DUPLICATE => 'bar', // not applied - may be different depending on context
+            "foo" => 'bar', // applied - checkpoint


checkpoint? I don't get this.

I've added an additional duplicated key in the end, to check if after all the checks above, this one is not skipped.

The comment itself is not clear enough. Please, try to rephrase it.

ravage84 · 2017-06-06T01:07:46Z

src/main/php/PHPMD/Rule/CleanCode/DuplicatedArrayKey.php

+     * @param ASTNode $node Array node.
+     * @return void
+     */
+    private function analyzeArray(ASTNode $node)


Very general name. How about checkForDuplicatedArrayKeys?

ravage84 · 2017-06-06T01:09:52Z

@eeree if we ever meet, remember me to buy you a drink 😼

ravage84 · 2017-06-06T01:12:41Z

Ignore the Stickler CI nags, created #487 for that...

eeree · 2017-06-11T12:29:49Z

@ravage84
#484 (review) done

Refs: #484 (comment)

ravage84 · 2017-06-11T13:25:58Z

src/main/php/PHPMD/Rule/CleanCode/DuplicatedArrayKey.php

@@ -79,14 +79,19 @@ private function analyzeArray(ASTNode $node)
    }

    /**
-     * Sets normalized name as node's image.
+     * To compare keys, we have to cast them to string.


There is no short description. It should always have a short description and if necessary a long description.

https://www.phpdoc.org/docs/latest/getting-started/your-first-set-of-documentation.html#example

ravage84 · 2017-06-11T13:32:28Z

src/site/rst/rules/cleancode.rst

 Remark
 ======

  This document is based on a ruleset xml-file, that was taken from the original source of the `PMD`__ project. This means that most parts of the content on this page are the intellectual work of the PMD community and its contributors and not of the PHPMD project.

 __ http://pmd.sourceforge.net/
-


Done in d40a0f1

ravage84

Fix the last doc block and I think we are good 👍

…d-array-keys

ravage84 · 2017-06-11T20:47:26Z

🍸

cweiske · 2019-01-21T09:54:15Z

Since this feature has not been released 1.5 years later: phpstan is also able to detect duplicate array keys.

rafalwrzeszcz and others added 5 commits February 29, 2016 02:04

Detectign duplicated array keys.

44f20a9

Oh, we still support PHP 5.3… so 2009 ;).

5570e3e

Added public modifier, just so Scrutinizer don't complain.

dc87a6d

Moved rule to CleanCode section.

48e6db8

Closes phpmd#350

b1ea2ef

stickler-ci reviewed Jun 5, 2017

View reviewed changes

Make Stickler Bot happy

103930e

stickler-ci reviewed Jun 5, 2017

View reviewed changes

ravage84 self-assigned this Jun 6, 2017

ravage84 added the Feature label Jun 6, 2017

ravage84 added this to the 2.7.0 milestone Jun 6, 2017

ravage84 mentioned this pull request Jun 6, 2017

Detecting duplicated array keys #350

Closed

ravage84 requested changes Jun 6, 2017

View reviewed changes

ravage84 reviewed Jun 6, 2017

View reviewed changes

Resolves phpmd#484 (review)

ddb8c51

ravage84 added a commit that referenced this pull request Jun 11, 2017

Impove handling for rst files

d40a0f1

Refs: #484 (comment)

ravage84 reviewed Jun 11, 2017

View reviewed changes

ravage84 requested changes Jun 11, 2017

View reviewed changes

eeree added 2 commits June 11, 2017 16:57

Merge remote-tracking branch 'upstream/master' into feature/duplicate…

3c5a822

…d-array-keys

Resolves phpmd#484 (review)

a040a41

ravage84 merged commit a295850 into phpmd:master Jun 11, 2017

eeree mentioned this pull request Jun 14, 2017

[Proposal] duplicated array indexes rule #322

Closed

kylekatarnls mentioned this pull request Jul 30, 2019

Release 2.7.0 #642

Merged

8 tasks

violinist-bot mentioned this pull request Jul 30, 2019

Update phpmd/phpmd from 2.6.1 to 2.7.0 matthijsthoolen/Slacky#4

Closed


		namespace PHPMDTest;

		class testRuleAppliesMultipleTimesToMethodWithAssotiativeArrayDefinitionWithDuplicatedKeys


		namespace PHPMDTest;

		class testRuleAppliesToMethodWithAssotiativeArrayDefinitionWithDuplicatedKeys


		namespace PHPMDTest;

		class testRuleNotAppliesToMethodWithAssotiativeArrayDefinitionWithoutDuplicatedKeys


		namespace PHPMDTest;

		class testRuleNotAppliesToMethodWithNonAssotiativeArrayDefinition


		namespace PHPMDTest;

		class testRuleNotAppliesToMethodWithoutArrayDefinition

Feature/duplicated array keys #484

Feature/duplicated array keys #484

Conversation

eeree commented Jun 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravage84 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravage84 Jun 6, 2017 • edited

Choose a reason for hiding this comment

ravage84 commented Jun 6, 2017

ravage84 commented Jun 6, 2017

eeree commented Jun 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravage84 left a comment

Choose a reason for hiding this comment

ravage84 commented Jun 11, 2017

cweiske commented Jan 21, 2019

ravage84 left a comment •

edited

ravage84 Jun 6, 2017 •

edited