From c5b80275ac307606fab0aa670764b3c84dc8f73d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thiemo=20M=C3=A4ttig?= Date: Thu, 5 Oct 2017 14:11:40 +0200 Subject: [PATCH] [WIP] Do not build SnakList on top of ArrayObject --- src/Snak/SnakList.php | 190 ++++++++++--------------------- tests/unit/Snak/SnakListTest.php | 7 +- 2 files changed, 63 insertions(+), 134 deletions(-) diff --git a/src/Snak/SnakList.php b/src/Snak/SnakList.php index e63995b3..dc10221c 100644 --- a/src/Snak/SnakList.php +++ b/src/Snak/SnakList.php @@ -2,16 +2,18 @@ namespace Wikibase\DataModel\Snak; -use ArrayObject; use Comparable; +use Countable; use Hashable; use InvalidArgumentException; +use Iterator; +use Serializable; use Traversable; use Wikibase\DataModel\Internal\MapValueHasher; /** * List of Snak objects. - * Indexes the snaks by hash and ensures no more the one snak with the same hash are in the list. + * Indexes the snaks by hash and ensures no more than one snak with the same hash is in the list. * * @since 0.1 * @@ -19,19 +21,12 @@ * @author Jeroen De Dauw < jeroendedauw@gmail.com > * @author Addshore */ -class SnakList extends ArrayObject implements Comparable, Hashable { +class SnakList implements Comparable, Countable, Hashable, Iterator, Serializable { /** - * Maps snak hashes to their offsets. - * - * @var array [ snak hash (string) => snak offset (string|int) ] + * @var Snak[] */ - private $offsetHashes = []; - - /** - * @var int - */ - private $indexOffset = 0; + private $snaks = []; /** * @param Snak[]|Traversable $snaks @@ -43,8 +38,12 @@ public function __construct( $snaks = [] ) { throw new InvalidArgumentException( '$snaks must be an array or an instance of Traversable' ); } - foreach ( $snaks as $index => $snak ) { - $this->setElement( $index, $snak ); + foreach ( $snaks as $value ) { + if ( !( $value instanceof Snak ) ) { + throw new InvalidArgumentException( '$value must be a Snak' ); + } + + $this->addSnak( $value ); } } @@ -56,7 +55,7 @@ public function __construct( $snaks = [] ) { * @return boolean */ public function hasSnakHash( $snakHash ) { - return array_key_exists( $snakHash, $this->offsetHashes ); + return isset( $this->snaks[$snakHash] ); } /** @@ -65,10 +64,7 @@ public function hasSnakHash( $snakHash ) { * @param string $snakHash */ public function removeSnakHash( $snakHash ) { - if ( $this->hasSnakHash( $snakHash ) ) { - $offset = $this->offsetHashes[$snakHash]; - $this->offsetUnset( $offset ); - } + unset( $this->snaks[$snakHash] ); } /** @@ -79,11 +75,13 @@ public function removeSnakHash( $snakHash ) { * @return boolean Indicates if the snak was added or not. */ public function addSnak( Snak $snak ) { - if ( $this->hasSnak( $snak ) ) { + $hash = $snak->getHash(); + + if ( $this->hasSnakHash( $hash ) ) { return false; } - $this->append( $snak ); + $this->snaks[$hash] = $snak; return true; } @@ -115,12 +113,7 @@ public function removeSnak( Snak $snak ) { * @return Snak|bool */ public function getSnak( $snakHash ) { - if ( !$this->hasSnakHash( $snakHash ) ) { - return false; - } - - $offset = $this->offsetHashes[$snakHash]; - return $this->offsetGet( $offset ); + return isset( $this->snaks[$snakHash] ) ? $this->snaks[$snakHash] : false; } /** @@ -143,6 +136,13 @@ public function equals( $target ) { && $this->getHash() === $target->getHash(); } + /** + * @return int + */ + public function count() { + return count( $this->snaks ); + } + /** * @see Hashable::getHash * @@ -154,7 +154,7 @@ public function equals( $target ) { */ public function getHash() { $hasher = new MapValueHasher(); - return $hasher->hash( $this ); + return $hasher->hash( $this->snaks ); } /** @@ -169,106 +169,16 @@ public function getHash() { public function orderByProperty( array $order = [] ) { $byProperty = array_fill_keys( $order, [] ); - /** @var Snak $snak */ - foreach ( $this as $snak ) { - $byProperty[$snak->getPropertyId()->getSerialization()][] = $snak; + foreach ( $this->snaks as $snak ) { + $byProperty[$snak->getPropertyId()->getSerialization()][$snak->getHash()] = $snak; } - $ordered = []; + $this->snaks = []; foreach ( $byProperty as $snaks ) { - $ordered = array_merge( $ordered, $snaks ); - } - - $this->exchangeArray( $ordered ); - - $index = 0; - foreach ( $ordered as $snak ) { - $this->offsetHashes[$snak->getHash()] = $index++; - } - } - - /** - * Finds a new offset for when appending an element. - * The base class does this, so it would be better to integrate, - * but there does not appear to be any way to do this... - * - * @return int - */ - private function getNewOffset() { - while ( $this->offsetExists( $this->indexOffset ) ) { - $this->indexOffset++; - } - - return $this->indexOffset; - } - - /** - * @see ArrayObject::offsetUnset - * - * @since 0.1 - * - * @param int|string $index - */ - public function offsetUnset( $index ) { - if ( $this->offsetExists( $index ) ) { - /** - * @var Hashable $element - */ - $element = $this->offsetGet( $index ); - $hash = $element->getHash(); - unset( $this->offsetHashes[$hash] ); - - parent::offsetUnset( $index ); + $this->snaks = array_merge( $this->snaks, $snaks ); } } - /** - * @see ArrayObject::append - * - * @param Snak $value - */ - public function append( $value ) { - $this->setElement( null, $value ); - } - - /** - * @see ArrayObject::offsetSet() - * - * @param int|string $index - * @param Snak $value - */ - public function offsetSet( $index, $value ) { - $this->setElement( $index, $value ); - } - - /** - * Method that actually sets the element and holds - * all common code needed for set operations, including - * type checking and offset resolving. - * - * @param int|string $index - * @param Snak $value - * - * @throws InvalidArgumentException - */ - private function setElement( $index, $value ) { - if ( !( $value instanceof Snak ) ) { - throw new InvalidArgumentException( '$value must be a Snak' ); - } - - if ( $this->hasSnak( $value ) ) { - return; - } - - if ( $index === null ) { - $index = $this->getNewOffset(); - } - - $hash = $value->getHash(); - $this->offsetHashes[$hash] = $index; - parent::offsetSet( $index, $value ); - } - /** * @see Serializable::serialize * @@ -276,8 +186,8 @@ private function setElement( $index, $value ) { */ public function serialize() { return serialize( [ - 'data' => $this->getArrayCopy(), - 'index' => $this->indexOffset, + 'data' => array_values( $this->snaks ), + 'index' => count( $this->snaks ) - 1, ] ); } @@ -288,14 +198,10 @@ public function serialize() { */ public function unserialize( $serialized ) { $serializationData = unserialize( $serialized ); - - foreach ( $serializationData['data'] as $offset => $value ) { - // Just set the element, bypassing checks and offset resolving, - // as these elements have already gone through this. - parent::offsetSet( $offset, $value ); + $this->snaks = []; + foreach ( $serializationData['data'] as $snak ) { + $this->addSnak( $snak ); } - - $this->indexOffset = $serializationData['index']; } /** @@ -304,7 +210,27 @@ public function unserialize( $serialized ) { * @return bool */ public function isEmpty() { - return !$this->getIterator()->valid(); + return $this->snaks === []; + } + + public function current() { + return current( $this->snaks ); + } + + public function next() { + return next( $this->snaks ); + } + + public function key() { + return key( $this->snaks ); + } + + public function valid() { + return current( $this->snaks ) !== false; + } + + public function rewind() { + return reset( $this->snaks ); } } diff --git a/tests/unit/Snak/SnakListTest.php b/tests/unit/Snak/SnakListTest.php index 93fec118..83bb1b07 100644 --- a/tests/unit/Snak/SnakListTest.php +++ b/tests/unit/Snak/SnakListTest.php @@ -82,9 +82,12 @@ public function invalidConstructorArgumentsProvider() { ]; } - public function testGivenAssociativeArray_constructorPreservesArrayKeys() { + public function testGivenAssociativeArray_constructorDoesNotPreserveArrayKeys() { $snakList = new SnakList( [ 'key' => new PropertyNoValueSnak( 1 ) ] ); - $this->assertSame( [ 'key' ], array_keys( iterator_to_array( $snakList ) ) ); + $this->assertSame( + [ 'c77761897897f63f151c4a1deb8bd3ad23ac51c6' ], + array_keys( iterator_to_array( $snakList ) ) + ); } /**