Merge pull request #714 from wmde/moveGetSnakListHash

Move Hashable interface from HashArray to SnakList
wmde · Mar 14, 2017 · 5688aba · 5688aba
2 parents e306e75 + 6534583
commit 5688aba
Show file tree

Hide file tree

Showing 4 changed files with 54 additions and 40 deletions.
diff --git a/src/HashArray.php b/src/HashArray.php
@@ -7,7 +7,6 @@
 use Hashable;
 use InvalidArgumentException;
 use Traversable;
-use Wikibase\DataModel\Internal\MapValueHasher;
 
 /**
  * Generic array object with lookups based on hashes of the elements.
@@ -27,7 +26,7 @@
  * @license GPL-2.0+
  * @author Jeroen De Dauw < jeroendedauw@gmail.com >
  */
-abstract class HashArray extends ArrayObject implements Hashable, Comparable {
+abstract class HashArray extends ArrayObject implements Comparable {
 
 	/**
 	 * Maps element hashes to their offsets.
@@ -232,20 +231,6 @@ public function offsetUnset( $index ) {
 		}
 	}
 
-	/**
-	 * @see Hashable::getHash
-	 *
-	 * The hash is purely valuer based. Order of the elements in the array is not held into account.
-	 *
-	 * @since 0.1
-	 *
-	 * @return string
-	 */
-	public function getHash() {
-		$hasher = new MapValueHasher();
-		return $hasher->hash( $this );
-	}
-
 	/**
 	 * @see Comparable::equals
 	 *

diff --git a/src/Snak/SnakList.php b/src/Snak/SnakList.php
@@ -2,6 +2,7 @@
 
 namespace Wikibase\DataModel\Snak;
 
+use Hashable;
 use InvalidArgumentException;
 use Traversable;
 use Wikibase\DataModel\HashArray;
@@ -17,7 +18,7 @@
  * @author Jeroen De Dauw < jeroendedauw@gmail.com >
  * @author Addshore
  */
-class SnakList extends HashArray {
+class SnakList extends HashArray implements Hashable {
 
 	/**
 	 * @param Snak[]|Traversable $snaks
@@ -110,7 +111,9 @@ public function getSnak( $snakHash ) {
 	/**
 	 * @see HashArray::getHash
 	 *
-	 * @since 0.5
+	 * The hash is purely value based. Order of the elements in the array is not held into account.
+	 *
+	 * @since 0.1
 	 *
 	 * @return string
 	 */

diff --git a/tests/unit/HashArray/HashArrayWithoutDuplicatesTest.php b/tests/unit/HashArray/HashArrayWithoutDuplicatesTest.php
@@ -129,26 +129,4 @@ public function testRemoveElement( HashArray $array ) {
 		$this->assertTrue( true );
 	}
 
-	/**
-	 * @dataProvider instanceProvider
-	 * @param HashArray $array
-	 */
-	public function testGetHash( HashArray $array ) {
-		$hash = $array->getHash();
-
-		$this->assertSame( $hash, $array->getHash() );
-
-		$elements = $this->elementInstancesProvider();
-		$element = array_shift( $elements );
-		$element = $element[0][0];
-
-		$array->addElement( $element );
-
-		if ( $array->hasElement( $element ) ) {
-			$this->assertSame( $hash, $array->getHash() );
-		} else {
-			$this->assertNotSame( $hash, $array->getHash() );
-		}
-	}
-
 }
diff --git a/tests/unit/Snak/SnakListTest.php b/tests/unit/Snak/SnakListTest.php
@@ -3,6 +3,7 @@
 namespace Wikibase\DataModel\Tests\Snak;
 
 use DataValues\StringValue;
+use Hashable;
 use InvalidArgumentException;
 use Wikibase\DataModel\Entity\PropertyId;
 use Wikibase\DataModel\Snak\PropertyNoValueSnak;
@@ -293,4 +294,51 @@ public function testOrderByProperty( SnakList $snakList, SnakList $expected, arr
 		}
 	}
 
+	public function testHashableInterface() {
+		$this->assertInstanceOf( Hashable::class, new SnakList() );
+	}
+
+	public function testGetHash() {
+		$snakList = new SnakList( [ new PropertyNoValueSnak( 1 ) ] );
+		$hash = $snakList->getHash();
+
+		$this->assertInternalType( 'string', $hash, 'must be a string' );
+		$this->assertNotSame( '', $hash, 'must not be empty' );
+		$this->assertSame( $hash, $snakList->getHash(), 'second call must return the same hash' );
+
+		$otherList = new SnakList( [ new PropertyNoValueSnak( 2 ) ] );
+		$this->assertNotSame( $hash, $otherList->getHash() );
+	}
+
+	/**
+	 * This integration test (relies on SnakObject::getHash) is supposed to break whenever the hash
+	 * calculation changes.
+	 */
+	public function testHashStability() {
+		$snakList = new SnakList();
+		$this->assertSame( 'da39a3ee5e6b4b0d3255bfef95601890afd80709', $snakList->getHash() );
+
+		$snakList = new SnakList( [ new PropertyNoValueSnak( 1 ) ] );
+		$this->assertSame( '4327ac5109aaf437ccce05580c563a5857d96c82', $snakList->getHash() );
+	}
+
+	/**
+	 * @dataProvider provideEqualSnakLists
+	 */
+	public function testGivenEqualSnakLists_getHashIsTheSame( SnakList $self, SnakList $other ) {
+		$this->assertSame( $self->getHash(), $other->getHash() );
+	}
+
+	public function provideEqualSnakLists() {
+		$empty = new SnakList();
+		$oneSnak = new SnakList( [ new PropertyNoValueSnak( 1 ) ] );
+
+		return [
+			'same empty object' => [ $empty, $empty ],
+			'same non-empty object' => [ $oneSnak, $oneSnak ],
+			'equal empty objects' => [ $empty, new SnakList() ],
+			'equal non-empty objects' => [ $oneSnak, new SnakList( [ new PropertyNoValueSnak( 1 ) ] ) ],
+		];
+	}
+
 }