Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

site: Simplify SiteList by removing GenericArrayObject indirection

Browse source 
== Background ==

In 2012, commit afe46f1403 (Id7e9b59c7e) added libs/GenericArrayObject
along with an (unused) abstract GenericArrayObjectTest case.

The same code was also added to the Wikibase extension with
change 6347b35a55cd (Ifa7f1dc702).

The code was then factored out from Wikibase into the wmde/Diff
library.

In 2013, GenericArrayObject was removed from wmde/Diff in the commit
at https://github.com/wmde/Diff/commit/d9c2bd5c140e2a783fd42298db6c.

== This change ==

Remove the GenericArrayObject indirection from SiteList as there exist
nothing outside SiteList refering to it in Codesearch Everywhere, and
even in SiteList much of the code in GenericArrayObject is overridden,
unused, or otherwise needlessly indirect.

Change-Id: Ifea09c5de50af1616058d8baa9037db273dfb0e5
This commit is contained in:
Timo Tijhof 2023-06-02 20:18:54 +01:00 committed by Aaron Schulz
parent e2747e78e0
commit 90e2ed6ccb
7 changed files with 123 additions and 573 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
RELEASE-NOTES-1.41
View file

@ -118,6 +118,8 @@ because of Phabricator reports.
* The following unused IDatabase methods were removed without deprecation:
- ::wasLockTimeout()
- ::wasConnectionLoss()
* The GenericArrayObject class, deprecated in 1.40,
has been removed.
* PrevNextNavigationRenderer class, deprecated in 1.39,
has been removed.
* class alias MediaWiki\User\WatchlistNotificationManager, deprecated in 1.36,

1
autoload.php
View file

@ -548,7 +548,6 @@ $wgAutoloadLocalClasses = [
'GenerateSitemap' => __DIR__ . '/maintenance/generateSitemap.php',
'GenerateUcfirstOverrides' => __DIR__ . '/maintenance/language/generateUcfirstOverrides.php',
'GenerateUpperCharTable' => __DIR__ . '/maintenance/language/generateUpperCharTable.php',
'GenericArrayObject' => __DIR__ . '/includes/libs/GenericArrayObject.php',
'GenericParameterJob' => __DIR__ . '/includes/jobqueue/GenericParameterJob.php',
'GetConfiguration' => __DIR__ . '/maintenance/getConfiguration.php',
'GetLagTimes' => __DIR__ . '/maintenance/getLagTimes.php',

236
includes/libs/GenericArrayObject.php
View file

@ -1,236 +0,0 @@
<?php
/**
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License along
* with this program; if not, write to the Free Software Foundation, Inc.,
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
* http://www.gnu.org/copyleft/gpl.html
*
* @file
*/
/**
* 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.
*
* @deprecated since 1.40 Use built-in ArrayObject instead.
* @since 1.20
* @license GPL-2.0-or-later
* @author Jeroen De Dauw < jeroendedauw@gmail.com >
*/
abstract class GenericArrayObject extends ArrayObject {
/**
* Returns the name of an interface/class that the element should implement/extend.
*
* @since 1.20
*
* @return string
*/
abstract public function getObjectType();
/**
* @see SiteList::getNewOffset()
* @since 1.20
* @var int
*/
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 1.20
*
* @return int
*/
protected function getNewOffset() {
while ( $this->offsetExists( $this->indexOffset ) ) {
$this->indexOffset++;
}
return $this->indexOffset;
}
/**
* @see ArrayObject::__construct
*
* @since 1.20
*
* @param null|array $input
* @param int $flags
* @param string $iterator_class
*/
public function __construct( $input = null, $flags = 0, $iterator_class = 'ArrayIterator' ) {
parent::__construct( [], $flags, $iterator_class );
if ( $input !== null ) {
foreach ( $input as $offset => $value ) {
$this->offsetSet( $offset, $value );
}
}
if ( static::class !== 'SiteList' ) {
wfDeprecated( __CLASS__, '1.40' );
}
}
/**
* @see ArrayObject::append
*
* @since 1.20
*
* @param mixed $value
*/
public function append( $value ): void {
$this->setElement( null, $value );
}
/**
* @see ArrayObject::offsetSet()
*
* @since 1.20
*
* @param mixed $index
* @param mixed $value
*/
public function offsetSet( $index, $value ): void {
$this->setElement( $index, $value );
}
/**
* Returns if the provided value has the same type as the elements
* that can be added to this ArrayObject.
*
* @since 1.20
*
* @param mixed $value
*
* @return bool
*/
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 1.20
*
* @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 '
. static::class . '.'
);
}
$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 1.20
*
* @param int|string $index
* @param mixed $value
*
* @return bool
*/
protected function preSetElement( $index, $value ) {
return true;
}
/**
* @see Serializable::serialize
*
* @since 1.38
*
* @return array
*/
public function __serialize(): array {
return $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 1.20
*
* @return array
*/
protected function getSerializationData() {
return [
'data' => $this->getArrayCopy(),
'index' => $this->indexOffset,
];
}
/**
* @see Serializable::unserialize
*
* @since 1.38
*
* @param array $serializationData
*/
public function __unserialize( $serializationData ): void {
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'];
}
/**
* Returns if the ArrayObject has no elements.
*
* @since 1.20
*
* @return bool
*/
public function isEmpty() {
return $this->count() === 0;
}
}

149
includes/site/SiteList.php
View file

@ -19,13 +19,24 @@
*/
/**
* Collection of Site objects.
* Array-like collection of Site objects.
*
* This uses ArrayObject to intercept additions and deletions for purposes
* such as additional indexing, and to enforce that values are restricted
* to Site objects only.
*
* @since 1.21
* @ingroup Site
* @author Jeroen De Dauw < jeroendedauw@gmail.com >
*/
class SiteList extends GenericArrayObject {
class SiteList extends ArrayObject {
/**
* @see SiteList::getNewOffset()
* @since 1.20
* @var int
*/
protected $indexOffset = 0;
/**
* Internal site identifiers pointing to their sites offset value.
*
@ -55,10 +66,58 @@ class SiteList extends GenericArrayObject {
protected $byNavigationId = [];
/**
* @see GenericArrayObject::getObjectType
* @see Overrides ArrayObject::__construct https://www.php.net/manual/en/arrayobject.construct.php
* @since 1.20
* @param null|array $input
* @param int $flags
* @param string $iterator_class
*/
public function __construct( $input = null, $flags = 0, $iterator_class = 'ArrayIterator' ) {
parent::__construct( [], $flags, $iterator_class );
if ( $input !== null ) {
foreach ( $input as $offset => $value ) {
$this->offsetSet( $offset, $value );
}
}
}
/**
* @see Overrides ArrayObject::append
* @since 1.20
* @param mixed $value
*/
public function append( $value ): void {
$this->setElement( null, $value );
}
/**
* @since 1.20
* @see Overrides ArrayObject::offsetSet()
* @param mixed $index
* @param mixed $value
*/
public function offsetSet( $index, $value ): void {
$this->setElement( $index, $value );
}
/**
* Returns if the provided value has the same type as the elements
* that can be added to this ArrayObject.
*
* @since 1.20
* @param mixed $value
* @return bool
*/
protected function hasValidType( $value ) {
$class = $this->getObjectType();
return $value instanceof $class;
}
/**
* The class or interface type that array elements must match.
*
* @since 1.21
*
* @return string
*/
public function getObjectType() {
@ -66,16 +125,36 @@ class SiteList extends GenericArrayObject {
}
/**
* @see GenericArrayObject::preSetElement
* Find a new offset for when appending an element.
*
* @since 1.21
*
* @param int|string $index
* @param Site $site
*
* @return bool
* @since 1.20
* @return int
*/
protected function preSetElement( $index, $site ) {
protected function getNewOffset() {
while ( $this->offsetExists( $this->indexOffset ) ) {
$this->indexOffset++;
}
return $this->indexOffset;
}
/**
* Actually set the element and enforce type checking and offset resolving.
*
* @since 1.20
* @param int|string|null $index
* @param Site $site
*/
protected function setElement( $index, $site ) {
if ( !$this->hasValidType( $site ) ) {
throw new InvalidArgumentException(
'Can only add ' . $this->getObjectType() . ' implementing objects to '
. static::class . '.'
);
}
$index ??= $this->getNewOffset();
if ( $this->hasSite( $site->getGlobalId() ) ) {
$this->removeSite( $site->getGlobalId() );
}
@ -88,7 +167,7 @@ class SiteList extends GenericArrayObject {
$this->byNavigationId[$navId] = $index;
}
return true;
parent::offsetSet( $index, $site );
}
/**
@ -167,10 +246,9 @@ class SiteList extends GenericArrayObject {
}
/**
* Returns if the list contains no sites.
* Whether the list contains no sites.
*
* @since 1.21
*
* @return bool
*/
public function isEmpty() {
@ -288,7 +366,7 @@ class SiteList extends GenericArrayObject {
}
/**
* A version ID that identifies the serialization structure used by getSerializationData()
* A version ID that identifies the serialization structure used by __serialize()
* and unserialize(). This is useful for constructing cache keys in cases where the cache relies
* on serialization for storing the SiteList.
*
@ -299,7 +377,7 @@ class SiteList extends GenericArrayObject {
/**
* Returns the version ID that identifies the serialization structure used by
* getSerializationData() and unserialize(), including the structure of any nested structures.
* __serialize() and unserialize(), including the structure of any nested structures.
* This is useful for constructing cache keys in cases where the cache relies
* on serialization for storing the SiteList.
*
@ -311,34 +389,37 @@ class SiteList extends GenericArrayObject {
}
/**
* @see GenericArrayObject::getSerializationData
*
* @since 1.21
*
* @see Overrides Serializable::serialize
* @since 1.38
* @return array
*/
protected function getSerializationData() {
public function __serialize(): array {
// Data that should go into serialization calls.
//
// NOTE: When changing the structure, either implement unserialize() to handle the
// old structure too, or update SERIAL_VERSION_ID to kill any caches.
return array_merge(
parent::getSerializationData(),
[
'internalIds' => $this->byInternalId,
'globalIds' => $this->byGlobalId,
'navigationIds' => $this->byNavigationId
]
);
return [
'data' => $this->getArrayCopy(),
'index' => $this->indexOffset,
'internalIds' => $this->byInternalId,
'globalIds' => $this->byGlobalId,
'navigationIds' => $this->byNavigationId,
];
}
/**
* @see GenericArrayObject::__unserialize
*
* @see Overrides Serializable::unserialize
* @since 1.38
*
* @param array $serializationData
*/
public function __unserialize( $serializationData ): void {
parent::__unserialize( $serializationData );
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->byInternalId = $serializationData['internalIds'];
$this->byGlobalId = $serializationData['globalIds'];

1
tests/common/TestsAutoLoader.php
View file

@ -178,7 +178,6 @@ $wgAutoloadClasses += [
# tests/phpunit/includes/libs
'BagOStuffTestBase' => "$testDir/phpunit/includes/libs/objectcache/BagOStuffTestBase.php",
'GenericArrayObjectTest' => "$testDir/phpunit/includes/libs/GenericArrayObjectTest.php",
'Wikimedia\ParamValidator\TypeDef\TypeDefTestCase' => "$testDir/phpunit/unit/includes/libs/ParamValidator/TypeDef/TypeDefTestCase.php",
'Wikimedia\ParamValidator\TypeDef\TypeDefTestCaseTrait' => "$testDir/phpunit/unit/includes/libs/ParamValidator/TypeDef/TypeDefTestCaseTrait.php",

278
tests/phpunit/includes/libs/GenericArrayObjectTest.php
View file

@ -1,278 +0,0 @@
<?php
/**
* Tests for the GenericArrayObject and deriving classes.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License along
* with this program; if not, write to the Free Software Foundation, Inc.,
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
* http://www.gnu.org/copyleft/gpl.html
*
* @file
* @since 1.20
*
* @ingroup Test
* @group GenericArrayObject
*
* @author Jeroen De Dauw < jeroendedauw@gmail.com >
*/
abstract class GenericArrayObjectTest extends PHPUnit\Framework\TestCase {
use MediaWikiCoversValidator;
/**
* Returns objects that can serve as elements in the concrete
* GenericArrayObject deriving class being tested.
*
* @since 1.20
*
* @return array
*/
abstract public function elementInstancesProvider();
/**
* Returns the name of the concrete class being tested.
*
* @since 1.20
*
* @return string
*/
abstract public function getInstanceClass();
/**
* Provides instances of the concrete class being tested.
*
* @since 1.20
*
* @return array
*/
public function instanceProvider() {
$instances = [];
foreach ( $this->elementInstancesProvider() as $elementInstances ) {
$instances[] = $this->getNew( $elementInstances[0] );
}
return $this->arrayWrap( $instances );
}
/**
* @since 1.20
*
* @param array $elements
*
* @return GenericArrayObject
*/
protected function getNew( array $elements = [] ) {
$class = $this->getInstanceClass();
return new $class( $elements );
}
/**
* @dataProvider elementInstancesProvider
*
* @since 1.20
*
* @param array $elements
*
* @covers GenericArrayObject::__construct
*/
public function testConstructor( array $elements ) {
$arrayObject = $this->getNew( $elements );
$this->assertEquals( count( $elements ), $arrayObject->count() );
}
/**
* @dataProvider elementInstancesProvider
*
* @since 1.20
*
* @param array $elements
*
* @covers GenericArrayObject::isEmpty
*/
public function testIsEmpty( array $elements ) {
$arrayObject = $this->getNew( $elements );
$this->assertEquals( $elements === [], $arrayObject->isEmpty() );
}
/**
* @dataProvider instanceProvider
*
* @since 1.20
*
* @param GenericArrayObject $list
*
* @covers GenericArrayObject::offsetUnset
*/
public function testUnset( GenericArrayObject $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 1.20
*
* @param array $elements
*
* @covers GenericArrayObject::append
*/
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( static function ( GenericArrayObject $list, $element ) {
$list->append( $element );
} );
}
/**
* @since 1.20
*
* @param callable $function
*/
protected function checkTypeChecks( $function ) {
$list = $this->getNew();
$elementClass = $list->getObjectType();
foreach ( [ 42, 'foo', [], (object)[], 4.2 ] as $element ) {
$validValid = $element instanceof $elementClass;
try {
$function( $list, $element );
$valid = true;
} catch ( InvalidArgumentException $exception ) {
$valid = false;
}
$this->assertEquals(
$validValid,
$valid,
'Object of invalid type got successfully added to a GenericArrayObject'
);
}
}
/**
* @dataProvider elementInstancesProvider
*
* @since 1.20
*
* @param array $elements
* @covers GenericArrayObject::getObjectType
* @covers GenericArrayObject::offsetSet
*/
public function testOffsetSet( array $elements ) {
if ( $elements === [] ) {
$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( static function ( GenericArrayObject $list, $element ) {
$list->offsetSet( mt_rand(), $element );
} );
}
/**
* @dataProvider instanceProvider
*
* @since 1.21
*
* @param GenericArrayObject $list
*
* @covers GenericArrayObject::getSerializationData
* @covers GenericArrayObject::serialize
* @covers GenericArrayObject::unserialize
*/
public function testSerialization( GenericArrayObject $list ) {
$serialization = serialize( $list );
$copy = unserialize( $serialization );
$this->assertEquals( $serialization, serialize( $copy ) );
$this->assertSame( count( $list ), count( $copy ) );
$list = $list->getArrayCopy();
$copy = $copy->getArrayCopy();
$this->assertArrayEquals( $list, $copy, true, true );
}
}

29
tests/phpunit/includes/site/SiteListTest.php
View file

@ -1,5 +1,4 @@
<?php
/**
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@ -17,15 +16,13 @@
* http://www.gnu.org/copyleft/gpl.html
*
* @file
* @since 1.21
*
* @ingroup Site
* @ingroup Test
*
* @group Site
*
* @author Jeroen De Dauw < jeroendedauw@gmail.com >
*/
/**
* @covers SiteList
* @group Site
* @author Jeroen De Dauw < jeroendedauw@gmail.com >
*/
class SiteListTest extends MediaWikiIntegrationTestCase {
/**
@ -65,7 +62,6 @@ class SiteListTest extends MediaWikiIntegrationTestCase {
/**
* @dataProvider siteListProvider
* @param SiteList $sites
* @covers SiteList::isEmpty
*/
public function testIsEmpty( SiteList $sites ) {
$this->assertEquals( count( $sites ) === 0, $sites->isEmpty() );
@ -74,7 +70,6 @@ class SiteListTest extends MediaWikiIntegrationTestCase {
/**
* @dataProvider siteListProvider
* @param SiteList $sites
* @covers SiteList::getSite
*/
public function testGetSiteByGlobalId( SiteList $sites ) {
/**
@ -90,7 +85,6 @@ class SiteListTest extends MediaWikiIntegrationTestCase {
/**
* @dataProvider siteListProvider
* @param SiteList $sites
* @covers SiteList::getSiteByInternalId
*/
public function testGetSiteByInternalId( $sites ) {
/**
@ -108,7 +102,6 @@ class SiteListTest extends MediaWikiIntegrationTestCase {
/**
* @dataProvider siteListProvider
* @param SiteList $sites
* @covers SiteList::getSiteByNavigationId
*/
public function testGetSiteByNavigationId( $sites ) {
/**
@ -127,7 +120,6 @@ class SiteListTest extends MediaWikiIntegrationTestCase {
/**
* @dataProvider siteListProvider
* @param SiteList $sites
* @covers SiteList::hasSite
*/
public function testHasGlobalId( $sites ) {
$this->assertFalse( $sites->hasSite( 'non-existing-global-id' ) );
@ -146,7 +138,6 @@ class SiteListTest extends MediaWikiIntegrationTestCase {
/**
* @dataProvider siteListProvider
* @param SiteList $sites
* @covers SiteList::hasInternalId
*/
public function testHasInternallId( $sites ) {
/**
@ -164,7 +155,6 @@ class SiteListTest extends MediaWikiIntegrationTestCase {
/**
* @dataProvider siteListProvider
* @param SiteList $sites
* @covers SiteList::hasNavigationId
*/
public function testHasNavigationId( $sites ) {
/**
@ -183,7 +173,6 @@ class SiteListTest extends MediaWikiIntegrationTestCase {
/**
* @dataProvider siteListProvider
* @param SiteList $sites
* @covers SiteList::getGlobalIdentifiers
*/
public function testGetGlobalIdentifiers( SiteList $sites ) {
$identifiers = $sites->getGlobalIdentifiers();
@ -204,13 +193,7 @@ class SiteListTest extends MediaWikiIntegrationTestCase {
/**
* @dataProvider siteListProvider
*
* @since 1.21
*
* @param SiteList $list
* @covers SiteList::getSerializationData
* @covers SiteList::__serialize
* @covers SiteList::__unserialize
*/
public function testSerialization( SiteList $list ) {
$serialization = serialize( $list );