Merge pull request #634 from wmde/referencelistfix

Fix ReferenceList not removing all reference with same hash

Author: Benestar

src/ReferenceList.php

@@ -117,7 +117,7 @@ public function hasReference( Reference $reference ) {
 	}
 
 	/**
-	 * Returns the index of a reference or false if the reference could not be found.
+	 * Returns the index of the Reference object or false if the Reference could not be found.
 	 *
 	 * @since 0.5
 	 *
@@ -160,29 +160,31 @@ public function hasReferenceHash( $referenceHash ) {
 	}
 
 	/**
-	 * Removes the reference with the provided hash if it exists in the list.
+	 * Looks for the first Reference object in this list with the provided hash.
+	 * Removes all occurences of that object.
 	 *
 	 * @since 0.3
 	 *
 	 * @param string $referenceHash	`
 	 */
 	public function removeReferenceHash( $referenceHash ) {
-		foreach ( $this->references as $index => $reference ) {
-			if ( $reference->getHash() === $referenceHash ) {
-				$this->removeReferenceAtIndex( $index );
+		$reference = $this->getReference( $referenceHash );
+
+		if ( $reference === null ) {
+			return;
+		}
+
+		foreach ( $this->references as $index => $ref ) {
+			if ( $ref === $reference ) {
+				unset( $this->references[$index] );
 			}
 		}
-	}
 
-	/**
-	 * @param int $index
-	 */
-	private function removeReferenceAtIndex( $index ) {
-		array_splice( $this->references, $index, 1 );
+		$this->references = array_values( $this->references );
 	}
 
 	/**
-	 * Returns the reference with the provided hash, or
+	 * Returns the first Reference object with the provided hash, or
 	 * null if there is no such reference in the list.
 	 *
 	 * @since 0.3

tests/unit/ReferenceListTest.php

@@ -157,6 +157,26 @@ public function testRemoveReference( ReferenceList $array ) {
 		$this->assertTrue( true );
 	}
 
+	public function testRemoveReferenceRemovesIdenticalObjects() {
+		$reference = new Reference( array( new PropertyNoValueSnak( 1 ) ) );
+		$references = new ReferenceList( array( $reference, $reference ) );
+
+		$references->removeReference( $reference );
+
+		$this->assertTrue( $references->isEmpty() );
+	}
+
+	public function testRemoveReferenceDoesNotRemoveCopies() {
+		$reference = new Reference( array( new PropertyNoValueSnak( 1 ) ) );
+		$references = new ReferenceList( array( $reference, clone $reference ) );
+
+		$references->removeReference( $reference );
+
+		$this->assertFalse( $references->isEmpty() );
+		$this->assertTrue( $references->hasReference( $reference ) );
+		$this->assertNotSame( $reference, $references->getReference( $reference->getHash() ) );
+	}
+
 	public function testAddReferenceOnEmptyList() {
 		$reference = new Reference( array( new PropertyNoValueSnak( 1 ) ) );
 
@@ -330,6 +350,36 @@ public function testRemoveReferenceHash( ReferenceList $references ) {
 		$this->assertEquals( 0, count( $references ) );
 	}
 
+	public function testRemoveReferenceHashRemovesIdenticalObjects() {
+		$reference = new Reference( array( new PropertyNoValueSnak( 1 ) ) );
+		$references = new ReferenceList( array( $reference, $reference ) );
+
+		$references->removeReferenceHash( $reference->getHash() );
+
+		$this->assertTrue( $references->isEmpty() );
+	}
+
+	public function testRemoveReferenceHashDoesNotRemoveCopies() {
+		$reference = new Reference( array( new PropertyNoValueSnak( 1 ) ) );
+		$references = new ReferenceList( array( $reference, clone $reference ) );
+
+		$references->removeReferenceHash( $reference->getHash() );
+
+		$this->assertFalse( $references->isEmpty() );
+		$this->assertTrue( $references->hasReference( $reference ) );
+		$this->assertNotSame( $reference, $references->getReference( $reference->getHash() ) );
+	}
+
+	public function testRemoveReferenceHashUpdatesIndexes() {
+		$reference1 = new Reference( array( new PropertyNoValueSnak( 1 ) ) );
+		$reference2 = new Reference( array( new PropertyNoValueSnak( 2 ) ) );
+		$references = new ReferenceList( array( $reference1, $reference2 ) );
+
+		$references->removeReferenceHash( $reference1->getHash() );
+
+		$this->assertSame( 0, $references->indexOf( $reference2 ) );
+	}
+
 	public function testGivenOneSnak_addNewReferenceAddsSnak() {
 		$references = new ReferenceList();
 		$snak = new PropertyNoValueSnak( 1 );