Skip to content

[WIP] Do not build SnakList on top of ArrayObject #763

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
thiemowmde wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[WIP] Do not build SnakList on top of ArrayObject #763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
190 changes: 58 additions & 132 deletions src/Snak/SnakList.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,36 +2,31 @@

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
*
* @license GPL-2.0+
* @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
Expand All @@ -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 );
}
}

Expand All @@ -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] );
}

/**
Expand All @@ -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] );
}

/**
Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
}

/**
Expand All @@ -143,6 +136,13 @@ public function equals( $target ) {
&& $this->getHash() === $target->getHash();
}

/**
* @return int
*/
public function count() {
return count( $this->snaks );
}

/**
* @see Hashable::getHash
*
Expand All @@ -154,7 +154,7 @@ public function equals( $target ) {
*/
public function getHash() {
$hasher = new MapValueHasher();
return $hasher->hash( $this );
return $hasher->hash( $this->snaks );
}

/**
Expand All @@ -169,115 +169,25 @@ 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
*
* @return string
*/
public function serialize() {
return serialize( [
'data' => $this->getArrayCopy(),
'index' => $this->indexOffset,
'data' => array_values( $this->snaks ),
'index' => count( $this->snaks ) - 1,
] );
}

Expand All @@ -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'];
}

/**
Expand All @@ -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 );
}

}
7 changes: 5 additions & 2 deletions tests/unit/Snak/SnakListTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) )
);
}

/**
Expand Down