Missing tests for ByPropertyIdArray

[JeroenDeDauw]

ByPropertyIdArray::moveObjectInPropertyGroup

if ( $toIndex > $lastIndex + 1 || $toIndex < $numericIndices[0] ) {

This predicate can be removed with false and the tests still pass

---

Full mutation testing report:

36) \Humbug\Mutator\ConditionalBoundary\GreaterThan
Diff on \Wikibase\DataModel\ByPropertyIdArray::moveObjectInPropertyGroup() in /home/j/workspace/WikibaseDataModel/src/ByPropertyIdArray.php:
--- Original
+++ New
@@ @@
 
-		if ( $toIndex > $lastIndex + 1 || $toIndex < $numericIndices[0] ) {
+		if ( $toIndex >= $lastIndex + 1 || $toIndex < $numericIndices[0] ) {
 			throw new OutOfBoundsException( 'Object cannot be moved to ' . $toIndex );
 		}
 
 		if ( $toIndex >= $lastIndex ) {
 			$this->moveObjectToEndOfPropertyGroup( $object );
 		} else {


37) \Humbug\Mutator\Number\IntegerValue
Diff on \Wikibase\DataModel\ByPropertyIdArray::moveObjectInPropertyGroup() in /home/j/workspace/WikibaseDataModel/src/ByPropertyIdArray.php:
--- Original
+++ New
@@ @@
 
-		if ( $toIndex > $lastIndex + 1 || $toIndex < $numericIndices[0] ) {
+		if ( $toIndex > $lastIndex + 0 || $toIndex < $numericIndices[0] ) {
 			throw new OutOfBoundsException( 'Object cannot be moved to ' . $toIndex );
 		}
 
 		if ( $toIndex >= $lastIndex ) {
 			$this->moveObjectToEndOfPropertyGroup( $object );
 		} else {


38) \Humbug\Mutator\Boolean\LogicalOr
Diff on \Wikibase\DataModel\ByPropertyIdArray::moveObjectInPropertyGroup() in /home/j/workspace/WikibaseDataModel/src/ByPropertyIdArray.php:
--- Original
+++ New
@@ @@
 
-		if ( $toIndex > $lastIndex + 1 || $toIndex < $numericIndices[0] ) {
+		if ( $toIndex > $lastIndex + 1 && $toIndex < $numericIndices[0] ) {
 			throw new OutOfBoundsException( 'Object cannot be moved to ' . $toIndex );
 		}
 
 		if ( $toIndex >= $lastIndex ) {
 			$this->moveObjectToEndOfPropertyGroup( $object );
 		} else {


39) \Humbug\Mutator\ConditionalBoundary\GreaterThan
Diff on \Wikibase\DataModel\ByPropertyIdArray::movePropertyGroup() in /home/j/workspace/WikibaseDataModel/src/ByPropertyIdArray.php:
--- Original
+++ New
@@ @@
 
-		if ( $toIndex > $oldIndex ) {
+		if ( $toIndex >= $oldIndex ) {
 			// If the group shall be moved towards the bottom, the number of objects within the
 			// group needs to be subtracted from the absolute toIndex:
 			$toIndex -= count( $byIdClone[$propertyId->getSerialization()] );
 		}
 
 		foreach ( $this->getPropertyIds() as $pId ) {


40) \Humbug\Mutator\ConditionalBoundary\LessThanOrEqualTo
Diff on \Wikibase\DataModel\ByPropertyIdArray::moveObjectToIndex() in /home/j/workspace/WikibaseDataModel/src/ByPropertyIdArray.php:
--- Original
+++ New
@@ @@
 		} else {
-			$edgeIndex = ( $toIndex <= $propertyIndices[0] )
+			$edgeIndex = ( $toIndex < $propertyIndices[0] )
 				? $propertyIndices[0]
 				: $propertyIndices[count( $propertyIndices ) - 1];
 
 			$this->moveObjectInPropertyGroup( $object, $edgeIndex );
 			$this->movePropertyGroup( $object->getPropertyId(), $toIndex );
 		}


41) \Humbug\Mutator\ConditionalBoundary\LessThan
Diff on \Wikibase\DataModel\ByPropertyIdArray::addObjectToPropertyGroup() in /home/j/workspace/WikibaseDataModel/src/ByPropertyIdArray.php:
--- Original
+++ New
@@ @@
 			// index:
-			if ( $index < $validIndices[0] ) {
+			if ( $index <= $validIndices[0] ) {
 				array_unshift( $this->byId[$propertyId->getSerialization()], $object );
 			} else {
 				$this->byId[$propertyId->getSerialization()][] = $object;
 			}
 		}
 


42) \Humbug\Mutator\Number\IntegerValue
Diff on \Wikibase\DataModel\ByPropertyIdArray::addObjectToPropertyGroup() in /home/j/workspace/WikibaseDataModel/src/ByPropertyIdArray.php:
--- Original
+++ New
@@ @@
 			// index:
-			if ( $index < $validIndices[0] ) {
+			if ( $index < $validIndices[1] ) {
 				array_unshift( $this->byId[$propertyId->getSerialization()], $object );
 			} else {
 				$this->byId[$propertyId->getSerialization()][] = $object;
 			}
 		}

[thiemowmde]

I created wmde/WikibaseDataModelServices#157 as a replacement for the ByPropertyIdArray class a while ago. My replacement does not implement the full feature set of ByPropertyIdArray, on purpose. we are currently exposing this feature via an index parameter in the API, but nobody is using it (proof, that single spike was me). I vote for not fiddling with this class any more, but just killing it. We can still expose that index feature, but it will behave a little different. Basically: Users can not move entire statement groups any more.

If we can agree on this, this issue is obsolete.

[JeroenDeDauw]

I agree this class is badly designed as is. The indexing stuff got merged over my objections and this class is now one of the few I removed my name as author from since it got corrupted so much. Having something better designed would be great. Till then it is nice to keep track of the specific issues this class has now.

[JeroenDeDauw]

Actually I don't think this is worthwhile fixing and focus is better spend on replacing the class.