Do not use ByPropertyIdArray

Author: thiemowmde

composer.json

@@ -57,7 +57,7 @@
 			"vendor/bin/phpcs src/* tests/* --standard=phpcs.xml --extensions=php -sp"
 		],
 		"phpmd": [
-			"vendor/bin/phpmd src/,tests/unit/ text phpmd.xml"
+			"vendor/bin/phpmd src/ text phpmd.xml"
 		]
 	}
 }

src/Statement/StatementListChanger.php

@@ -2,7 +2,7 @@
 
 namespace Wikibase\DataModel\Services\Statement;
 
-use Wikibase\DataModel\ByPropertyIdArray;
+use Wikibase\DataModel\Entity\PropertyId;
 use Wikibase\DataModel\Statement\Statement;
 use Wikibase\DataModel\Statement\StatementList;
 
@@ -20,18 +20,6 @@ public function clear( StatementList $statementList ) {
 		}
 	}
 
-	/**
-	 * @param StatementList $statementList
-	 * @param Statement[] $statements
-	 */
-	private function replaceStatements( StatementList $statementList, array $statements ) {
-		$this->clear( $statementList );
-
-		foreach ( $statements as $statement ) {
-			$statementList->addStatement( $statement );
-		}
-	}
-
 	public function groupByProperty( StatementList $statementList ) {
 		$byId = [];
 
@@ -51,19 +39,116 @@ public function groupByProperty( StatementList $statementList ) {
 
 	/**
 	 * @param StatementList $statementList
-	 * @param Statement $statement
+	 * @param Statement $newStatement
+	 * @param int|null $index An absolute index in the list. If the index is not next to a statement
+	 *  with the same property ID, the closest possibly position is used instead. Default is null,
+	 *  which adds the new statement after the last statement with the same property ID.
+	 */
+	public function addToGroup(
+		StatementList $statementList,
+		Statement $newStatement,
+		$index = null
+	) {
+		$statements = $statementList->toArray();
+		$id = $newStatement->getPropertyId();
+
+		if ( $index === null ) {
+			$index = $this->getLastIndexWithinGroup( $statements, $id );
+		} else {
+			// Limit search range to avoid looping non-existing positions
+			$indexInRange = min( max( 0, $index ), count( $statements ) );
+			$index = $this->getClosestIndexWithinGroup( $statements, $id, $indexInRange );
+			if ( $index === null ) {
+				$index = $this->getClosestIndexAtGroupBorder( $statements, $indexInRange );
+			}
+		}
+
+		$statementList->addStatement( $newStatement, $index );
+	}
+
+	/**
+	 * @param Statement[] $statements
+	 * @param PropertyId $id
+	 *
+	 * @return int|null
+	 */
+	private function getLastIndexWithinGroup( array $statements, PropertyId $id ) {
+		for ( $i = count( $statements ); $i > 0; $i-- ) {
+			if ( $statements[$i - 1]->getPropertyId()->equals( $id ) ) {
+				return $i;
+			}
+		}
+
+		return null;
+	}
+
+	/**
+	 * @param Statement[] $statements
+	 * @param PropertyId $id
 	 * @param int $index
+	 *
+	 * @return int|null
 	 */
-	public function addToGroup( StatementList $statementList, Statement $statement, $index ) {
-		if ( $statementList->isEmpty() ) {
-			$statementList->addStatement( $statement );
-			return;
+	private function getClosestIndexWithinGroup( array $statements, PropertyId $id, $index ) {
+		$longestDistance = max( $index, count( $statements ) - $index );
+
+		for ( $i = 0; $i <= $longestDistance; $i++ ) {
+			if ( $this->isWithinGroup( $statements, $id, $index - $i ) ) {
+				return $index - $i;
+			} elseif ( $i && $this->isWithinGroup( $statements, $id, $index + $i ) ) {
+				return $index + $i;
+			}
 		}
 
-		$byPropertyIdArray = new ByPropertyIdArray( $statementList->toArray() );
-		$byPropertyIdArray->buildIndex();
-		$byPropertyIdArray->addObjectAtIndex( $statement, $index );
-		$this->replaceStatements( $statementList, $byPropertyIdArray->toFlatArray() );
+		return null;
+	}
+
+	/**
+	 * @param Statement[] $statements
+	 * @param int $index
+	 *
+	 * @return int|null
+	 */
+	private function getClosestIndexAtGroupBorder( array $statements, $index ) {
+		$longestDistance = max( $index, count( $statements ) - $index );
+
+		for ( $i = 0; $i <= $longestDistance; $i++ ) {
+			if ( $this->isGroupBorder( $statements, $index - $i ) ) {
+				return $index - $i;
+			} elseif ( $i && $this->isGroupBorder( $statements, $index + $i ) ) {
+				return $index + $i;
+			}
+		}
+
+		return null;
+	}
+
+	/**
+	 * @param Statement[] $statements
+	 * @param PropertyId $id
+	 * @param int $index
+	 *
+	 * @return bool
+	 */
+	private function isWithinGroup( array $statements, PropertyId $id, $index ) {
+		$count = count( $statements );
+
+		// Valid if the index either prepends ot appends a statement with the same property ID
+		return $index > 0 && $index <= $count && $statements[$index - 1]->getPropertyId()->equals( $id )
+			|| $index >= 0 && $index < $count && $statements[$index]->getPropertyId()->equals( $id );
+	}
+
+	/**
+	 * @param Statement[] $statements
+	 * @param int $index
+	 *
+	 * @return bool
+	 */
+	private function isGroupBorder( array $statements, $index ) {
+		// First and last possible position is always a border
+		return $index <= 0
+			|| $index >= count( $statements )
+			|| !$statements[$index - 1]->getPropertyId()->equals( $statements[$index]->getPropertyId() );
 	}
 
 }

tests/unit/Lookup/DispatchingEntityLookupTest.php

@@ -72,13 +72,14 @@ public function testGivenNotExistingEntityIdFromKnownRepository_getEntityReturns
 
 	/**
 	 * @param Exception $exception
-	 * @return \PHPUnit_Framework_MockObject_MockObject|EntityLookup
+	 *
+	 * @return EntityLookup
 	 */
 	private function getExceptionThrowingLookup( Exception $exception ) {
 		$lookup = $this->getMock( EntityLookup::class );
 		$lookup->expects( $this->any() )
 			->method( $this->anything() )
-			->willThrowException( $exception );
+			->will( $this->throwException( $exception ) );
 		return $lookup;
 	}