diff --git a/composer.json b/composer.json index 311514e2..edfb0c43 100644 --- a/composer.json +++ b/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" ] } } diff --git a/src/Statement/StatementListChanger.php b/src/Statement/StatementListChanger.php index 963f868c..3b1ed6e9 100644 --- a/src/Statement/StatementListChanger.php +++ b/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() ); } } diff --git a/tests/unit/Lookup/DispatchingEntityLookupTest.php b/tests/unit/Lookup/DispatchingEntityLookupTest.php index 86f9906b..574465cd 100644 --- a/tests/unit/Lookup/DispatchingEntityLookupTest.php +++ b/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; } diff --git a/tests/unit/Statement/StatementListChangerTest.php b/tests/unit/Statement/StatementListChangerTest.php index 2457c603..d555d9a6 100644 --- a/tests/unit/Statement/StatementListChangerTest.php +++ b/tests/unit/Statement/StatementListChangerTest.php @@ -24,7 +24,7 @@ public function testConstructor() { } public function testClear() { - $statementList = $this->newStatementList( [ 'P1$A' ] ); + $statementList = $this->newStatementList( [ 'P1$a' ] ); $instance = new StatementListChanger(); $instance->clear( $statementList ); @@ -32,6 +32,27 @@ public function testClear() { $this->assertTrue( $statementList->isEmpty() ); } + public function groupByPropertyIdProvider() { + return [ + [ + [], + [] + ], + [ + [ 'P1$a' ], + [ 'P1$a' ] + ], + [ + [ 'P1$a', 'P2$b', 'P1$c', 'P2$d' ], + [ 'P1$a', 'P1$c', 'P2$b', 'P2$d' ] + ], + [ + [ 'P1$a', 'P1$b', 'P2$c', 'P3$d', 'P1$e', 'P2$f' ], + [ 'P1$a', 'P1$b', 'P1$e', 'P2$c', 'P2$f', 'P3$d' ] + ], + ]; + } + /** * @dataProvider groupByPropertyIdProvider */ @@ -44,23 +65,27 @@ public function testGroupByPropertyId( array $guids, array $expectedGuids ) { $this->assertGuids( $expectedGuids, $statementList ); } - public function groupByPropertyIdProvider() { + public function addToGroupProvider() { return [ - [ + 'add to empty list' => [ [], - [] + 'P1$new', + [ 'P1$new' ] ], - [ - [ 'P1$A' ], - [ 'P1$A' ] + 'append' => [ + [ 'P1$a' ], + 'P2$new', + [ 'P1$a', 'P2$new' ] ], - [ - [ 'P1$A', 'P2$B', 'P1$C', 'P2$D' ], - [ 'P1$A', 'P1$C', 'P2$B', 'P2$D' ] + 'insert' => [ + [ 'P1$a', 'P2$b' ], + 'P1$new', + [ 'P1$a', 'P1$new', 'P2$b' ] ], - [ - [ 'P1$A', 'P1$B', 'P2$C', 'P3$D', 'P1$E', 'P2$F' ], - [ 'P1$A', 'P1$B', 'P1$E', 'P2$C', 'P2$F', 'P3$D' ] + 'prefer last group when not ordered' => [ + [ 'P1$a', 'P2$b', 'P1$c', 'P2$d' ], + 'P1$new', + [ 'P1$a', 'P2$b', 'P1$c', 'P1$new', 'P2$d' ] ], ]; } @@ -68,63 +93,147 @@ public function groupByPropertyIdProvider() { /** * @dataProvider addToGroupProvider */ - public function testAddToGroup( array $guids, $newGuid, $index, array $expectedGuids ) { + public function testAddToGroup( array $guids, $newGuid, array $expectedGuids ) { $statementList = $this->newStatementList( $guids ); $statement = $this->newStatement( $newGuid ); $instance = new StatementListChanger(); - $instance->addToGroup( $statementList, $statement, $index ); + $instance->addToGroup( $statementList, $statement ); $this->assertGuids( $expectedGuids, $statementList ); } - public function addToGroupProvider() { + public function addToGroupByIndexProvider() { return [ - 'add to empty list' => [ + // Add to an empty list + 'add to empty list with exact index' => [ [], - 'P1$A', + 'P1$new', 0, - [ 'P1$A' ] + [ 'P1$new' ] ], - 'negative index' => [ + 'add to empty list with extreme index' => [ [], - 'P1$A', - -100, - [ 'P1$A' ] + 'P1$new', + 100, + [ 'P1$new' ] ], - 'extreme index' => [ + 'add to empty list with negative index' => [ [], - 'P1$A', - 100, - [ 'P1$A' ] + 'P1$new', + -100, + [ 'P1$new' ] ], - 'append one statement' => [ - [ 'P1$A' ], - 'P1$B', + + // Add the second statement with the same property + 'append with exact index' => [ + [ 'P1$a' ], + 'P1$new', 1, - [ 'P1$A', 'P1$B' ] + [ 'P1$a', 'P1$new' ] ], - 'prepend one statement' => [ - [ 'P1$A' ], - 'P1$B', + 'prepend with exact index' => [ + [ 'P1$a' ], + 'P1$new', 0, - [ 'P1$B', 'P1$A' ] + [ 'P1$new', 'P1$a' ] ], - 'insert one statement' => [ - [ 'P1$A', 'P2$B' ], - 'P1$C', + + // Add to a list with multiple properties + 'insert with exact index' => [ + [ 'P1$a', 'P2$b' ], + 'P1$new', 1, - [ 'P1$A', 'P1$C', 'P2$B' ] + [ 'P1$a', 'P1$new', 'P2$b' ] ], - 'move group to the end' => [ - [ 'P1$A', 'P2$B' ], - 'P1$C', + 'insert with extreme index' => [ + [ 'P1$a', 'P2$b' ], + 'P1$new', 100, - [ 'P2$B', 'P1$A', 'P1$C' ] + [ 'P1$a', 'P1$new', 'P2$b' ] + ], + 'prepend with negative index' => [ + [ 'P1$a', 'P2$b' ], + 'P1$new', + -100, + [ 'P1$new', 'P1$a', 'P2$b' ] + ], + + // Add to a list that has multiple groups with the same property + 'decrease index to closest match' => [ + [ 'P1$a', 'P2$b', 'P2$c', 'P2$d', 'P1$e' ], + 'P1$new', + 2, + [ 'P1$a', 'P1$new', 'P2$b', 'P2$c', 'P2$d', 'P1$e' ], + ], + 'increase index to closest match' => [ + [ 'P1$a', 'P2$b', 'P2$c', 'P2$d', 'P1$e' ], + 'P1$new', + 3, + [ 'P1$a', 'P2$b', 'P2$c', 'P2$d', 'P1$new', 'P1$e' ], + ], + 'prefer decreasing when no closer match' => [ + [ 'P1$a', 'P2$b', 'P2$c', 'P1$d' ], + 'P1$new', + 2, + [ 'P1$a', 'P1$new', 'P2$b', 'P2$c', 'P1$d' ], + ], + + // Add a new property to a list that has internal group borders + 'decrease index to closest group border' => [ + [ 'P1$a', 'P2$b', 'P2$c', 'P2$d', 'P1$e' ], + 'P3$new', + 2, + [ 'P1$a', 'P3$new', 'P2$b', 'P2$c', 'P2$d', 'P1$e' ], + ], + 'increase index to closest group border' => [ + [ 'P1$a', 'P2$b', 'P2$c', 'P2$d', 'P1$e' ], + 'P3$new', + 3, + [ 'P1$a', 'P2$b', 'P2$c', 'P2$d', 'P3$new', 'P1$e' ], + ], + 'prefer decreasing when no closer group border' => [ + [ 'P1$a', 'P2$b', 'P2$c', 'P1$d' ], + 'P3$new', + 2, + [ 'P1$a', 'P3$new', 'P2$b', 'P2$c', 'P1$d' ], + ], + + // Add a new property to a list that has no internal group borders + 'decrease index to closest list limit' => [ + [ 'P1$a', 'P1$b', 'P1$c' ], + 'P2$new', + 1, + [ 'P2$new', 'P1$a', 'P1$b', 'P1$c' ], + ], + 'increase index to closest list limit' => [ + [ 'P1$a', 'P1$b', 'P1$c' ], + 'P2$new', + 2, + [ 'P1$a', 'P1$b', 'P1$c', 'P2$new' ], + ], + 'prefer decreasing when no closer list limit' => [ + [ 'P1$a', 'P1$b' ], + 'P2$new', + 1, + [ 'P2$new', 'P1$a', 'P1$b' ], ], ]; } + /** + * @dataProvider addToGroupByIndexProvider + */ + public function testAddToGroupByIndex( array $guids, $newGuid, $index, array $expectedGuids ) { + $statementList = $this->newStatementList( $guids ); + $statement = $this->newStatement( $newGuid ); + + $instance = new StatementListChanger(); + $instance->addToGroup( $statementList, $statement, $index ); + + $this->assertGuids( $expectedGuids, $statementList ); + } + /** * @param string[] $guids * @@ -140,6 +249,11 @@ private function newStatementList( array $guids ) { return $statementList; } + /** + * @param string $guid + * + * @return Statement + */ private function newStatement( $guid ) { list( $propertyId, ) = explode( '$', $guid, 2 );