Merge pull request #654 from wmde/addAtIndexFix

I'm not 100% sure this is the "best" behavior for the edge case. But it's simple, consistent, and tested, and we don't rely on it anywhere. And it's better than what we had before. So in it goes.

Author: Daniel Kinzler

src/ReferenceList.php

@@ -71,9 +71,15 @@ public function addReference( Reference $reference, $index = null ) {
 			return;
 		}
 
+		$splHash = spl_object_hash( $reference );
+
+		if ( array_key_exists( $splHash, $this->references ) ) {
+			return;
+		}
+
 		if ( $index === null || $index >= count( $this->references ) ) {
 			// Append object to the end of the reference list.
-			$this->references[spl_object_hash( $reference )] = $reference;
+			$this->references[$splHash] = $reference;
 		} else {
 			$this->insertReferenceAtIndex( $reference, $index );
 		}
@@ -104,9 +110,11 @@ private function insertReferenceAtIndex( Reference $reference, $index ) {
 			throw new InvalidArgumentException( '$index must be an integer' );
 		}
 
+		$splHash = spl_object_hash( $reference );
+
 		$this->references = array_merge(
 			array_slice( $this->references, 0, $index ),
-			[ spl_object_hash( $reference ) => $reference ],
+			[ $splHash => $reference ],
 			array_slice( $this->references, $index )
 		);
 	}

tests/unit/ReferenceListTest.php

@@ -241,17 +241,31 @@ public function testAddReferenceAtIndexMovesIdenticalObjects() {
 		$list->addNewReference( new PropertyNoValueSnak( 1 ) );
 		$reference = new Reference( [ new PropertyNoValueSnak( 2 ) ] );
 		$list->addReference( $reference );
-		$this->assertSame( 1, $list->indexOf( $reference ), 'pre condition' );
+		$list->addNewReference( new PropertyNoValueSnak( 3 ) );
+
+		$this->assertSame(
+			1,
+			$list->indexOf( $reference ),
+			'pre-condition is that the element is at index 1'
+		);
 
 		$list->addReference( $reference, 0 );
 
-		$this->assertCount( 2, $list, 'not added' );
-		$this->assertSame( 0, $list->indexOf( $reference ), 'can decrease index' );
+		$this->assertCount( 3, $list, 'not added' );
+		$this->assertSame(
+			1,
+			$list->indexOf( $reference ),
+			'make sure calling addReference with a lower index did not changed it'
+		);
 
 		$list->addReference( $reference, 2 );
 
-		$this->assertCount( 2, $list, 'not added' );
-		$this->assertSame( 0, $list->indexOf( $reference ), 'can not increase index' );
+		$this->assertCount( 3, $list, 'not added' );
+		$this->assertSame(
+			1,
+			$list->indexOf( $reference ),
+			'make sure calling addReference with a higher index did not changed it'
+		);
 	}
 
 	public function testAddReferenceAtIndexZero() {