Jeroen De Dauw has uploaded a new change for review. https://gerrit.wikimedia.org/r/57347
Change subject: Get rid of GenericArrayObject usage ...................................................................... Get rid of GenericArrayObject usage This was being a rather weird semi-dependency on MW offsering little advantage Change-Id: I79769fb9d2da442bbaad1f7307af8b465b010790 --- M Diff.classes.php M Diff.php M RELEASE-NOTES D compat/GenericArrayObject.php M includes/diffop/diff/Diff.php M tests/diffop/DiffTest.php 6 files changed, 422 insertions(+), 269 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Diff refs/changes/47/57347/1 diff --git a/Diff.classes.php b/Diff.classes.php index 683773d..5c07180 100644 --- a/Diff.classes.php +++ b/Diff.classes.php @@ -12,8 +12,6 @@ * @author Jeroen De Dauw < [email protected] > */ return array( - 'GenericArrayObject' => 'compat/GenericArrayObject.php', - 'Diff\Appendable' => 'includes/Appendable.php', 'Diff\DiffOpFactory' => 'includes/DiffOpFactory.php', diff --git a/Diff.php b/Diff.php index eaefbe3..0e53e8e 100644 --- a/Diff.php +++ b/Diff.php @@ -28,7 +28,7 @@ * @ingroup Test */ -define( 'Diff_VERSION', '0.5' ); +define( 'Diff_VERSION', '0.6 alpha' ); // @codeCoverageIgnoreStart call_user_func( function() { diff --git a/RELEASE-NOTES b/RELEASE-NOTES index a0f2da7..746f1ef 100644 --- a/RELEASE-NOTES +++ b/RELEASE-NOTES @@ -4,6 +4,19 @@ Latest version of the release notes: https://gerrit.wikimedia.org/r/gitweb?p=mediawiki/extensions/Diff.git;a=blob;f=RELEASE-NOTES +=== Version 0.6 === +dev + +; Removals + +* GenericArrayObject has been removed from this package. + Diff derives from ArrayObject rather than GenericArrayObject. + Its interface has not changed expect for the points below. +* The getObjectType method in Diff (previously defined in GenericArrayObject) + is now private rather than public. +* Adding a non-DiffOp element to a Diff will now result in an InvalidArgumentException + rather than a MWException. + === Version 0.5 === 2013-02-26 diff --git a/compat/GenericArrayObject.php b/compat/GenericArrayObject.php deleted file mode 100644 index 2f36042..0000000 --- a/compat/GenericArrayObject.php +++ /dev/null @@ -1,232 +0,0 @@ -<?php - -/** - * Extends ArrayObject and does two things: - * - * Allows for deriving classes to easily intercept additions - * and deletions for purposes such as additional indexing. - * - * Enforces the objects to be of a certain type, so this - * can be replied upon, much like if this had true support - * for generics, which sadly enough is not possible in PHP. - * - * @since 0.1 - * - * @file - * @ingroup Diff - * - * @licence GNU GPL v2+ - * @author Jeroen De Dauw < [email protected] > - * - * @codeCoverageIgnoreStart - */ -abstract class GenericArrayObject extends \ArrayObject { - - /** - * Returns the name of an interface/class that the element should implement/extend. - * - * @since 0.1 - * - * @return string - */ - public abstract function getObjectType(); - - /** - * @see SiteList::getNewOffset() - * @since 0.1 - * @var integer - */ - protected $indexOffset = 0; - - /** - * 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... - * - * @since 0.1 - * - * @return integer - */ - protected function getNewOffset() { - while ( $this->offsetExists( $this->indexOffset ) ) { - $this->indexOffset++; - } - - return $this->indexOffset; - } - - /** - * Constructor. - * @see ArrayObject::__construct - * - * @since 0.1 - * - * @param null|array $input - * @param int $flags - * @param string $iterator_class - */ - public function __construct( $input = null, $flags = 0, $iterator_class = 'ArrayIterator' ) { - parent::__construct( array(), $flags, $iterator_class ); - - if ( !is_null( $input ) ) { - foreach ( $input as $offset => $value ) { - $this->offsetSet( $offset, $value ); - } - } - } - - /** - * @see ArrayObject::append - * - * @since 0.1 - * - * @param mixed $value - */ - public function append( $value ) { - $this->setElement( null, $value ); - } - - /** - * @see ArrayObject::offsetSet() - * - * @since 0.1 - * - * @param mixed $index - * @param mixed $value - * - * @throws \MWException - */ - public function offsetSet( $index, $value ) { - $this->setElement( $index, $value ); - } - - /** - * Returns if the provided value has the same type as the elements - * that can be added to this ArrayObject. - * - * @since 0.1 - * - * @param mixed $value - * - * @return boolean - */ - protected function hasValidType( $value ) { - $class = $this->getObjectType(); - return $value instanceof $class; - } - - /** - * Method that actually sets the element and holds - * all common code needed for set operations, including - * type checking and offset resolving. - * - * If you want to do additional indexing or have code that - * otherwise needs to be executed whenever an element is added, - * you can overload @see preSetElement. - * - * @since 0.1 - * - * @param mixed $index - * @param mixed $value - * - * @throws \MWException - */ - protected function setElement( $index, $value ) { - if ( !$this->hasValidType( $value ) ) { - throw new \MWException( - 'Can only add ' . $this->getObjectType() . ' implementing objects to ' . get_called_class() . '.' - ); - } - - if ( is_null( $index ) ) { - $index = $this->getNewOffset(); - } - - if ( $this->preSetElement( $index, $value ) ) { - parent::offsetSet( $index, $value ); - } - } - - /** - * Gets called before a new element is added to the ArrayObject. - * - * At this point the index is always set (ie not null) and the - * value is always of the type returned by @see getObjectType. - * - * Should return a boolean. When false is returned the element - * does not get added to the ArrayObject. - * - * @since 0.1 - * - * @param integer|string $index - * @param mixed $value - * - * @return boolean - */ - protected function preSetElement( $index, $value ) { - return true; - } - - /** - * @see Serializable::serialize - * - * @since 0.1 - * - * @return string - */ - public function serialize() { - return serialize( $this->getSerializationData() ); - } - - /** - * Returns an array holding all the data that should go into serialization calls. - * This is intended to allow overloading without having to reimplement the - * behavior of this base class. - * - * @since 0.1 - * - * @return array - */ - protected function getSerializationData() { - return array( - 'data' => $this->getArrayCopy(), - 'index' => $this->indexOffset, - ); - } - - /** - * @see Serializable::unserialize - * - * @since 0.1 - * - * @param string $serialization - * - * @return array - */ - public function unserialize( $serialization ) { - $serializationData = unserialize( $serialization ); - - 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->indexOffset = $serializationData['index']; - - return $serializationData; - } - - /** - * Returns if the ArrayObject has no elements. - * - * @since 0.1 - * - * @return boolean - */ - public function isEmpty() { - return $this->count() === 0; - } - -} -// @codeCoverageIgnoreEnd diff --git a/includes/diffop/diff/Diff.php b/includes/diffop/diff/Diff.php index 45d3280..1ee4cbc 100644 --- a/includes/diffop/diff/Diff.php +++ b/includes/diffop/diff/Diff.php @@ -1,6 +1,7 @@ <?php namespace Diff; + use InvalidArgumentException; /** @@ -16,7 +17,7 @@ * @author Jeroen De Dauw < [email protected] > * @author Daniel Kinzler */ -class Diff extends \GenericArrayObject implements IDiff { +class Diff extends \ArrayObject implements IDiff { /** * @since 0.4 @@ -42,6 +43,13 @@ ); /** + * @since 0.1 + * + * @var integer + */ + protected $indexOffset = 0; + + /** * @see Diff::__construct * * @since 0.1 @@ -52,29 +60,31 @@ * @throws InvalidArgumentException */ public function __construct( array $operations = array(), $isAssociative = null ) { - foreach ( $operations as $operation ) { - if ( !( $operation instanceof DiffOp ) ) { - throw new InvalidArgumentException( 'All elements fed to the Diff constructor should be of type DiffOp' ); - } - } - if ( $isAssociative !== null && !is_bool( $isAssociative ) ) { throw new InvalidArgumentException( '$isAssociative should be a boolean or null' ); } - $this->isAssociative = $isAssociative; + parent::__construct( array() ); - parent::__construct( $operations ); + foreach ( $operations as $offset => $operation ) { + if ( !( $operation instanceof DiffOp ) ) { + throw new InvalidArgumentException( 'All elements fed to the Diff constructor should be of type DiffOp' ); + } + + $this->offsetSet( $offset, $operation ); + } + + $this->isAssociative = $isAssociative; } /** - * @see GenericArrayObject::getObjectType + * Returns the name of an interface/class that the element should implement/extend. * * @since 0.1 * * @return string */ - public function getObjectType() { + private function getObjectType() { return '\Diff\DiffOp'; } @@ -117,7 +127,13 @@ } /** - * @see GenericArrayObject::preSetElement + * Gets called before a new element is added to the ArrayObject. + * + * At this point the index is always set (ie not null) and the + * value is always of the type returned by @see getObjectType. + * + * Should return a boolean. When false is returned the element + * does not get added to the ArrayObject. * * @since 0.1 * @@ -146,26 +162,7 @@ } /** - * @see GenericArrayObject::getSerializationData - * - * @since 0.1 - * - * @return array - */ - protected function getSerializationData() { - $assoc = $this->isAssociative === null ? 'n' : ( $this->isAssociative ? 't' : 'f' ); - - return array_merge( - parent::getSerializationData(), - array( - 'typePointers' => $this->typePointers, - 'assoc' => $assoc - ) - ); - } - - /** - * @see GenericArrayObject::unserialize + * @see Serializable::unserialize * * @since 0.1 * @@ -174,7 +171,15 @@ * @return array */ public function unserialize( $serialization ) { - $serializationData = parent::unserialize( $serialization ); + $serializationData = unserialize( $serialization ); + + 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->indexOffset = $serializationData['index']; $this->typePointers = $serializationData['typePointers']; @@ -383,4 +388,124 @@ ); } + /** + * 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... + * + * @since 0.1 + * + * @return integer + */ + protected function getNewOffset() { + while ( $this->offsetExists( $this->indexOffset ) ) { + $this->indexOffset++; + } + + return $this->indexOffset; + } + + /** + * @see ArrayObject::append + * + * @since 0.1 + * + * @param mixed $value + */ + public function append( $value ) { + $this->setElement( null, $value ); + } + + /** + * @see ArrayObject::offsetSet() + * + * @since 0.1 + * + * @param mixed $index + * @param mixed $value + * + * @throws \MWException + */ + public function offsetSet( $index, $value ) { + $this->setElement( $index, $value ); + } + + /** + * Returns if the provided value has the same type as the elements + * that can be added to this ArrayObject. + * + * @since 0.1 + * + * @param mixed $value + * + * @return boolean + */ + protected function hasValidType( $value ) { + $class = $this->getObjectType(); + return $value instanceof $class; + } + + /** + * Method that actually sets the element and holds + * all common code needed for set operations, including + * type checking and offset resolving. + * + * If you want to do additional indexing or have code that + * otherwise needs to be executed whenever an element is added, + * you can overload @see preSetElement. + * + * @since 0.1 + * + * @param mixed $index + * @param mixed $value + * + * @throws InvalidArgumentException + */ + protected function setElement( $index, $value ) { + if ( !$this->hasValidType( $value ) ) { + throw new InvalidArgumentException( + 'Can only add ' . $this->getObjectType() . ' implementing objects to ' . get_called_class() . '.' + ); + } + + if ( is_null( $index ) ) { + $index = $this->getNewOffset(); + } + + if ( $this->preSetElement( $index, $value ) ) { + parent::offsetSet( $index, $value ); + } + } + + /** + * @see Serializable::serialize + * + * @since 0.1 + * + * @return string + */ + public function serialize() { + $assoc = $this->isAssociative === null ? 'n' : ( $this->isAssociative ? 't' : 'f' ); + + $data = array( + 'data' => $this->getArrayCopy(), + 'index' => $this->indexOffset, + 'typePointers' => $this->typePointers, + 'assoc' => $assoc + ); + + return serialize( $data ); + } + + /** + * Returns if the ArrayObject has no elements. + * + * @since 0.1 + * + * @return boolean + */ + public function isEmpty() { + return $this->count() === 0; + } + } diff --git a/tests/diffop/DiffTest.php b/tests/diffop/DiffTest.php index 4cb1d54..901d5e9 100644 --- a/tests/diffop/DiffTest.php +++ b/tests/diffop/DiffTest.php @@ -1,12 +1,15 @@ <?php namespace Diff\Test; + use Diff\Diff; use Diff\DiffOp; use Diff\MapDiff; use Diff\DiffOpAdd; use Diff\DiffOpRemove; use Diff\DiffOpChange; +use InvalidArgumentException; +use stdClass; /** * Tests for the Diff\Diff class. @@ -37,7 +40,7 @@ * @licence GNU GPL v2+ * @author Jeroen De Dauw < [email protected] > */ -class DiffTest extends \GenericArrayObjectTest { +class DiffTest extends \PHPUnit_Framework_TestCase { public function elementInstancesProvider() { return array( @@ -150,6 +153,9 @@ $this->assertContainsOnlyInstancesOf( '\Diff\DiffOp', $diff ); + /** + * @var DiffOp $operation + */ foreach ( $diff as $operation ) { if ( !in_array( $operation->getType(), $types ) ) { $types[] = $operation->getType(); @@ -407,5 +413,248 @@ $this->assertEquals( 1, count( $diff->getChanges() ) ); } + /** + * @since 0.6 + * + * @param array $elements + * + * @return Diff + */ + protected function getNew( array $elements = array() ) { + $class = $this->getInstanceClass(); + return new $class( $elements ); + } + + /** + * @dataProvider elementInstancesProvider + * + * @since 0.6 + * + * @param array $elements + */ + public function testConstructor( array $elements ) { + $arrayObject = $this->getNew( $elements ); + + $this->assertEquals( count( $elements ), $arrayObject->count() ); + } + + /** + * @dataProvider elementInstancesProvider + * + * @since 0.6 + * + * @param array $elements + */ + public function testIsEmpty( array $elements ) { + $arrayObject = $this->getNew( $elements ); + + $this->assertEquals( $elements === array(), $arrayObject->isEmpty() ); + } + + /** + * @dataProvider instanceProvider + * + * @since 0.6 + * + * @param Diff $list + */ + public function testUnset( Diff $list ) { + if ( $list->isEmpty() ) { + $this->assertTrue( true ); // We cannot test unset if there are no elements + } else { + $offset = $list->getIterator()->key(); + $count = $list->count(); + $list->offsetUnset( $offset ); + $this->assertEquals( $count - 1, $list->count() ); + } + + if ( !$list->isEmpty() ) { + $offset = $list->getIterator()->key(); + $count = $list->count(); + unset( $list[$offset] ); + $this->assertEquals( $count - 1, $list->count() ); + } + } + + /** + * @dataProvider elementInstancesProvider + * + * @since 0.6 + * + * @param array $elements + */ + public function testAppend( array $elements ) { + $list = $this->getNew(); + + $listSize = count( $elements ); + + foreach ( $elements as $element ) { + $list->append( $element ); + } + + $this->assertEquals( $listSize, $list->count() ); + + $list = $this->getNew(); + + foreach ( $elements as $element ) { + $list[] = $element; + } + + $this->assertEquals( $listSize, $list->count() ); + + $this->checkTypeChecks( function ( Diff $list, $element ) { + $list->append( $element ); + } ); + } + + /** + * @since 0.6 + * + * @param callback $function + */ + protected function checkTypeChecks( $function ) { + $excption = null; + $list = $this->getNew(); + + + foreach ( array( 42, 'foo', array(), new stdClass(), 4.2 ) as $element ) { + $this->assertInvalidArgument( $function, $list, $element ); + } + } + + /** + * Asserts that an InvalidArgumentException gets thrown when calling the provided + * callable. Extra arguments specified to the method are also provided to the callable. + * + * @since 0.6 + * + * @param callable $function + */ + protected function assertInvalidArgument( $function ) { + $this->setExpectedException( 'InvalidArgumentException' ); + + $arguments = func_get_args(); + array_shift( $arguments ); + + call_user_func_array( $function, $arguments ); + } + + /** + * @dataProvider elementInstancesProvider + * + * @since 0.6 + * + * @param array $elements + */ + public function testOffsetSet( array $elements ) { + if ( $elements === array() ) { + $this->assertTrue( true ); + return; + } + + $list = $this->getNew(); + + $element = reset( $elements ); + $list->offsetSet( 42, $element ); + $this->assertEquals( $element, $list->offsetGet( 42 ) ); + + $list = $this->getNew(); + + $element = reset( $elements ); + $list['oHai'] = $element; + $this->assertEquals( $element, $list['oHai'] ); + + $list = $this->getNew(); + + $element = reset( $elements ); + $list->offsetSet( 9001, $element ); + $this->assertEquals( $element, $list[9001] ); + + $list = $this->getNew(); + + $element = reset( $elements ); + $list->offsetSet( null, $element ); + $this->assertEquals( $element, $list[0] ); + + $list = $this->getNew(); + $offset = 0; + + foreach ( $elements as $element ) { + $list->offsetSet( null, $element ); + $this->assertEquals( $element, $list[$offset++] ); + } + + $this->assertEquals( count( $elements ), $list->count() ); + + $this->checkTypeChecks( function ( Diff $list, $element ) { + $list->offsetSet( mt_rand(), $element ); + } ); + } + + /** + * @dataProvider instanceProvider + * + * @since 0.6 + * + * @param Diff $list + */ + public function testSerialization( Diff $list ) { + $serialization = serialize( $list ); + $copy = unserialize( $serialization ); + + $this->assertEquals( $serialization, serialize( $copy ) ); + $this->assertEquals( count( $list ), count( $copy ) ); + + $list = $list->getArrayCopy(); + $copy = $copy->getArrayCopy(); + + $this->assertArrayEquals( $list, $copy, true, true ); + } + + /** + * Assert that two arrays are equal. By default this means that both arrays need to hold + * the same set of values. Using additional arguments, order and associated key can also + * be set as relevant. + * + * @since 0.6 + * + * @param array $expected + * @param array $actual + * @param boolean $ordered If the order of the values should match + * @param boolean $named If the keys should match + */ + protected function assertArrayEquals( array $expected, array $actual, $ordered = false, $named = false ) { + if ( !$ordered ) { + $this->objectAssociativeSort( $expected ); + $this->objectAssociativeSort( $actual ); + } + + if ( !$named ) { + $expected = array_values( $expected ); + $actual = array_values( $actual ); + } + + call_user_func_array( + array( $this, 'assertEquals' ), + array_merge( array( $expected, $actual ), array_slice( func_get_args(), 4 ) ) + ); + } + + /** + * Does an associative sort that works for objects. + * + * @since 0.6 + * + * @param array $array + */ + protected function objectAssociativeSort( array &$array ) { + uasort( + $array, + function ( $a, $b ) { + return serialize( $a ) > serialize( $b ) ? 1 : -1; + } + ); + } + } -- To view, visit https://gerrit.wikimedia.org/r/57347 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I79769fb9d2da442bbaad1f7307af8b465b010790 Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/Diff Gerrit-Branch: master Gerrit-Owner: Jeroen De Dauw <[email protected]> _______________________________________________ MediaWiki-commits mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
