Addshore has uploaded a new change for review. https://gerrit.wikimedia.org/r/94930
Change subject: Cleanup api GetEntities and surrounding clasess ...................................................................... Cleanup api GetEntities and surrounding clasess Change-Id: I7314f865075ebd9d2957892078b3267fa0ab3ddf --- M repo/includes/api/GetEntities.php M repo/includes/api/ItemByTitleHelper.php M repo/includes/api/ResultBuilder.php M repo/tests/phpunit/includes/api/ItemByTitleHelperTest.php M repo/tests/phpunit/includes/api/ResultBuilderTest.php 5 files changed, 153 insertions(+), 191 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase refs/changes/30/94930/1 diff --git a/repo/includes/api/GetEntities.php b/repo/includes/api/GetEntities.php index 6575a7d..3f955a3 100644 --- a/repo/includes/api/GetEntities.php +++ b/repo/includes/api/GetEntities.php @@ -7,16 +7,14 @@ use MWException; use ValueParsers\ParseException; use Wikibase\DataModel\Entity\EntityId; -use Wikibase\EntityContent; +use Wikibase\LanguageFallbackChainFactory; use Wikibase\Lib\Serializers\SerializationOptions; use Wikibase\Lib\Serializers\EntitySerializer; use Wikibase\Lib\Serializers\SerializerFactory; use Wikibase\Repo\WikibaseRepo; +use Wikibase\StringNormalizer; use Wikibase\Utils; use Wikibase\StoreFactory; -use Wikibase\Item; -use Wikibase\EntityContentFactory; -use Wikibase\LanguageFallbackChainFactory; /** * API module to get the data for one or more Wikibase entities. @@ -27,21 +25,17 @@ * @author Jeroen De Dauw < [email protected] > * @author Marius Hoch < [email protected] > * @author Michał Łazowik + * @author Adam Shorland */ class GetEntities extends ApiWikibase { /** - * @var \Wikibase\StringNormalizer + * @var StringNormalizer */ protected $stringNormalizer; /** - * @var \Wikibase\Lib\EntityIdParser - */ - protected $entityIdParser; - - /** - * @var \Wikibase\LanguageFallbackChainFactory + * @var LanguageFallbackChainFactory */ protected $languageFallbackChainFactory; @@ -49,7 +43,6 @@ parent::__construct( $main, $name, $prefix ); $this->stringNormalizer = WikibaseRepo::getDefaultInstance()->getStringNormalizer(); - $this->entityIdParser = WikibaseRepo::getDefaultInstance()->getEntityIdParser(); $this->languageFallbackChainFactory = WikibaseRepo::getDefaultInstance()->getLanguageFallbackChainFactory(); } @@ -58,7 +51,6 @@ */ public function execute() { wfProfileIn( __METHOD__ ); - $params = $this->extractRequestParams(); if ( !isset( $params['ids'] ) && ( empty( $params['sites'] ) || empty( $params['titles'] ) ) ) { @@ -66,12 +58,11 @@ $this->dieUsage( 'Either provide the item "ids" or pairs of "sites" and "titles" for corresponding pages', 'param-missing' ); } - $params['ids'] = $this->getEntityIdsFromParams( $params ); - - foreach ( $params['ids'] as $entityId ) { + foreach ( $this->getEntityIdsFromParams( $params ) as $entityId ) { $this->handleEntity( $entityId, $params ); } + //todo remove once result builder is used... if ( $this->getResult()->getIsRawMode() ) { $this->getResult()->setIndexedTagName_internal( array( 'entities' ), 'entity' ); } @@ -82,37 +73,62 @@ } /** - * Get array of entities from api request params + * Get a unique array of EntityIds from api request params * * @param array $params * - * @return array + * @return EntityId[] */ protected function getEntityIdsFromParams( array $params ) { - $entityIds = isset( $params['ids'] ) ? $params['ids'] : array(); + $fromIds = $this->getEntityIdsFromIdParam( $params ); + $fromSiteTitleCombinations = $this->getItemIdsFromSiteTitleParams( $params ); + $ids = array_merge( $fromIds, $fromSiteTitleCombinations ); + return array_unique( $ids ); + } + /** + * @param array $params + * @return EntityId[] + */ + private function getEntityIdsFromIdParam( $params ) { + $ids = array(); + if( isset( $params['ids'] ) ) { + $entityIdParser = WikibaseRepo::getDefaultInstance()->getEntityIdParser(); + foreach( $params['ids'] as $id ) { + try { + $ids[] = $entityIdParser->parse( $id ); + } catch( ParseException $e ) { + wfProfileOut( __METHOD__ ); + $this->dieUsage( "Invalid id: $id", 'no-such-entity' ); + } + } + } + return $ids; + } + + /** + * @param array $params + * @return EntityId[] + */ + private function getItemIdsFromSiteTitleParams( $params ) { + $ids = array(); if ( !empty( $params['sites'] ) && !empty( $params['titles'] ) ) { $siteLinkCache = StoreFactory::getStore()->newSiteLinkCache(); $siteStore = SiteSQLStore::newInstance(); $itemByTitleHelper = new ItemByTitleHelper( $this->resultBuilder, $siteLinkCache, $siteStore, $this->stringNormalizer ); - $otherIDs = $itemByTitleHelper->getEntityIds( $params['sites'], $params['titles'], $params['normalize'] ); - $entityIds = array_merge( $entityIds, $otherIDs ); + list( $ids, $missingItems ) = $itemByTitleHelper->getItemIds( $params['sites'], $params['titles'], $params['normalize'] ); + $this->addMissingItemstoResult( $missingItems ); } - - return $this->uniqueEntities( $entityIds ); + return $ids; } /** - * Checks whether props contain sitelinks indirectly - * - * @since 0.5 - * - * @param array $props - * - * @return bool + * @param array $missingItems Array of arrays, Each internal array has a key 'site' and 'title' */ - protected function hasImpliedSiteLinksProp( $props ) { - return in_array( 'sitelinks/urls', $props ); + private function addMissingItemstoResult( $missingItems ){ + foreach( $missingItems as $missingItem ) { + $this->resultBuilder->addMissingEntity( $missingItem ); + } } /** @@ -125,7 +141,7 @@ * @return array */ protected function getPropsFromParams( $params ) { - if ( $this->hasImpliedSiteLinksProp( $params['props'] ) ) { + if ( in_array( 'sitelinks/urls', $params['props'] ) ) { $params['props'][] = 'sitelinks'; } @@ -133,136 +149,85 @@ } /** - * Makes an arry of entity ids unique after applaying normalization. - * - * @param array $entityIds - * - * @return array - */ - protected function uniqueEntities( $entityIds ) { - $ids = array(); - foreach ( $entityIds as $entityId ) { - try { - $id = $this->entityIdParser->parse( $entityId ); - $ids[] = $id->getPrefixedId(); - } - catch ( ParseException $parseException ) { - // This will error below - $ids[] = null; - } - } - return array_unique( $ids ); - } - - /** * Fetches the entity with provided id and adds its serialization to the output. * * @since 0.2 * - * @param string $id + * @param EntityId $entityId * @param array $params * * @throws MWException */ - protected function handleEntity( $id, array $params ) { + protected function handleEntity( $entityId, array $params ) { wfProfileIn( __METHOD__ ); - - $entityContentFactory = WikibaseRepo::getDefaultInstance()->getEntityContentFactory(); - $entityIdFormatter = WikibaseRepo::getDefaultInstance()->getEntityIdFormatter(); - $entityIdParser = WikibaseRepo::getDefaultInstance()->getEntityIdParser(); - - $res = $this->getResult(); - + $result = $this->getResult(); $props = $this->getPropsFromParams( $params ); + $entityPath = array( 'entities', $entityId->getSerialization() ); + $entityContentFactory = WikibaseRepo::getDefaultInstance()->getEntityContentFactory(); - try { - $entityId = $entityIdParser->parse( $id ); - } catch( ParseException $e ) { - wfProfileOut( __METHOD__ ); - $this->dieUsage( "Invalid id: $id", 'no-such-entity' ); + $entityContent = $entityContentFactory->getFromId( $entityId ); + if ( is_null( $entityContent ) ) { + $this->resultBuilder->addMissingEntity( array( 'id' => $entityId->getSerialization() ) ); + return; } - // key should be numeric to get the correct behavior - // note that this setting depends upon "setIndexedTagName_internal" - //FIXME: if we get different kinds of entities at once, $entityId->getNumericId() may not be unique. - $entityPath = array( - 'entities', - !$this->getResult()->getIsRawMode() ? $entityIdFormatter->format( $entityId ) : $entityId->getNumericId() - ); - - // later we do a getContent but only if props are defined - if ( $params['props'] !== array() ) { - $page = $entityContentFactory->getWikiPageForId( $entityId ); - - if ( $page->exists() ) { - - // as long as getWikiPageForId only returns ids for legal items this holds - /** - * @var $entityContent EntityContent - */ - $entityContent = $page->getContent(); - - // this should not happen unless a page is not what we assume it to be - // that is, we want this to be a little more solid if something ges wrong - if ( is_null( $entityContent ) ) { - $res->addValue( $entityPath, 'id', $entityIdFormatter->format( $entityId ) ); - $res->addValue( $entityPath, 'illegal', "" ); - return; - } - - // default stuff to add that comes from the title/page/revision - if ( in_array( 'info', $props ) ) { - $res->addValue( $entityPath, 'pageid', intval( $page->getId() ) ); - $title = $page->getTitle(); - $res->addValue( $entityPath, 'ns', intval( $title->getNamespace() ) ); - $res->addValue( $entityPath, 'title', $title->getPrefixedText() ); - $revision = $page->getRevision(); - - if ( $revision !== null ) { - $res->addValue( $entityPath, 'lastrevid', intval( $revision->getId() ) ); - $res->addValue( $entityPath, 'modified', wfTimestamp( TS_ISO_8601, $revision->getTimestamp() ) ); - } - } - - $entity = $entityContent->getEntity(); - - // TODO: inject id formatter - $options = new SerializationOptions(); - if ( $params['languagefallback'] ) { - $languages = array(); - foreach ( $params['languages'] as $languageCode ) { - // $languageCode is already filtered as valid ones - $languages[$languageCode] = $this->languageFallbackChainFactory - ->newFromContextAndLanguageCode( $this->getContext(), $languageCode ); - } - } else { - $languages = $params['languages']; - } - $options->setLanguages( $languages ); - $options->setOption( EntitySerializer::OPT_SORT_ORDER, $params['dir'] ); - $options->setOption( EntitySerializer::OPT_PARTS, $props ); - $options->setIndexTags( $this->getResult()->getIsRawMode() ); - - $serializerFactory = new SerializerFactory(); - $entitySerializer = $serializerFactory->newSerializerForObject( $entity, $options ); - - $entitySerialization = $entitySerializer->getSerialized( $entity ); - - foreach ( $entitySerialization as $key => $value ) { - $res->addValue( $entityPath, $key, $value ); - } - } - else { - $res->addValue( $entityPath, 'missing', "" ); - } + //if there are no props defined only return type and id.. + if ( $params['props'] === array() ) { + $result->addValue( $entityPath, 'id', $entityId->getSerialization() ); + $result->addValue( $entityPath, 'type', $entityId->getEntityType() ); } else { - $res->addValue( $entityPath, 'id', $entityIdFormatter->format( $entityId ) ); - $res->addValue( $entityPath, 'type', $entityId->getEntityType() ); + if ( in_array( 'info', $props ) ) { + $title = $entityContent->getTitle(); + $result->addValue( $entityPath, 'pageid', $title->getArticleID() ); + $result->addValue( $entityPath, 'ns', intval( $title->getNamespace() ) ); + $result->addValue( $entityPath, 'title', $title->getPrefixedText() ); + + $revision = $entityContent->getWikiPage()->getRevision(); + if ( $revision !== null ) { + $result->addValue( $entityPath, 'lastrevid', intval( $revision->getId() ) ); + $result->addValue( $entityPath, 'modified', wfTimestamp( TS_ISO_8601, $revision->getTimestamp() ) ); + } + } + + $serializerFactory = new SerializerFactory(); + $entity = $entityContent->getEntity(); + $options = $this->getSerializationOptions( $params, $props ); + $entitySerializer = $serializerFactory->newSerializerForObject( $entity, $options ); + $entitySerialization = $entitySerializer->getSerialized( $entity ); + + foreach ( $entitySerialization as $key => $value ) { + $result->addValue( $entityPath, $key, $value ); + } } wfProfileOut( __METHOD__ ); } /** + * @param array $params + * @param array $props + * + * @return SerializationOptions + */ + private function getSerializationOptions( $params, $props ){ + $options = new SerializationOptions(); + if ( $params['languagefallback'] ) { + $languages = array(); + foreach ( $params['languages'] as $languageCode ) { + // $languageCode is already filtered as valid ones + $languages[$languageCode] = $this->languageFallbackChainFactory + ->newFromContextAndLanguageCode( $this->getContext(), $languageCode ); + } + } else { + $languages = $params['languages']; + } + $options->setLanguages( $languages ); + $options->setOption( EntitySerializer::OPT_SORT_ORDER, $params['dir'] ); + $options->setOption( EntitySerializer::OPT_PARTS, $props ); + $options->setIndexTags( $this->getResult()->getIsRawMode() ); + return $options; + } + + /** * @see \ApiBase::getAllowedParams() */ public function getAllowedParams() { diff --git a/repo/includes/api/ItemByTitleHelper.php b/repo/includes/api/ItemByTitleHelper.php index cb77740..d71b110 100644 --- a/repo/includes/api/ItemByTitleHelper.php +++ b/repo/includes/api/ItemByTitleHelper.php @@ -44,7 +44,12 @@ * @param SiteStore $siteStore * @param StringNormalizer $stringNormalizer */ - public function __construct( ResultBuilder $resultBuilder, SiteLinkCache $siteLinkCache, SiteStore $siteStore, StringNormalizer $stringNormalizer ) { + public function __construct( + ResultBuilder $resultBuilder, + SiteLinkCache $siteLinkCache, + SiteStore $siteStore, + StringNormalizer $stringNormalizer + ) { $this->resultBuilder = $resultBuilder; $this->siteLinkCache = $siteLinkCache; $this->siteStore = $siteStore; @@ -52,16 +57,17 @@ } /** - * Tries to find entity ids for given client pages. + * Tries to find item ids for given client pages. * * @param array $sites * @param array $titles * @param bool $normalize * * @throws UsageException - * @return array + * @return array( ItemId[], array() ) + * List containing valid ItemIds and MissingItem site title combinations */ - public function getEntityIds( array $sites, array $titles, $normalize ) { + public function getItemIds( array $sites, array $titles, $normalize ) { $ids = array(); $numSites = count( $sites ); $numTitles = count( $titles ); @@ -82,44 +88,46 @@ ); } + $missingItems = array(); foreach( $sites as $siteId ) { foreach( $titles as $title ) { - $entityId = $this->getEntiyId( $siteId, $title, $normalize ); - if( !is_null( $entityId ) ) { - $ids[] = $entityId; + $itemId = $this->getItemId( $siteId, $title, $normalize ); + if( !is_null( $itemId ) ) { + $ids[] = $itemId; } else { - //todo move this out of the helper... - $this->resultBuilder->addMissingEntity( $siteId, $title ); + $missingItems[] = array( 'site' => $siteId, 'title' => $title ); } } } - return $ids; + return array( $ids, $missingItems ); } /** - * Tries to find entity id for given siteId and title combination + * Tries to find item id for given siteId and title combination * * @param string $siteId * @param string $title * @param bool $normalize * - * @return string|null + * @return ItemId|null */ - private function getEntiyId( $siteId, $title, $normalize ) { + private function getItemId( $siteId, $title, $normalize ) { $title = $this->stringNormalizer->trimToNFC( $title ); $id = $this->siteLinkCache->getItemIdForLink( $siteId, $title ); // Try harder by requesting normalization on the external site. if ( $id === false && $normalize === true ) { $siteObj = $this->siteStore->getSite( $siteId ); - $id = $this->normalizeTitle( $title, $siteObj ); + //XXX: this passes the normalized title back into $title by reference... + $this->normalizeTitle( $title, $siteObj ); + $id = $this->siteLinkCache->getItemIdForLink( $siteObj->getGlobalId(), $title ); } if ( $id === false ) { return null; } else { - return ItemId::newFromNumber( $id )->getPrefixedId(); + return ItemId::newFromNumber( $id ); } } @@ -129,8 +137,6 @@ * * @param string &$title * @param Site $site - * - * @return integer|boolean */ public function normalizeTitle( &$title, Site $site ) { $normalizedTitle = $site->normalizePageName( $title ); @@ -139,9 +145,6 @@ $this->resultBuilder->addNormalizedTitle( $title, $normalizedTitle ); //XXX: the below is an ugly hack as we pass title by reference... $title = $normalizedTitle; - return $this->siteLinkCache->getItemIdForLink( $site->getGlobalId(), $normalizedTitle ); } - - return false; } } diff --git a/repo/includes/api/ResultBuilder.php b/repo/includes/api/ResultBuilder.php index 02bb2f0..e0cdb73 100644 --- a/repo/includes/api/ResultBuilder.php +++ b/repo/includes/api/ResultBuilder.php @@ -217,15 +217,14 @@ /** * Add an entry for a missing entity... - * @param string $siteId - * @param string $title + * @param array $missingDetails array containing key value pair missing details */ - public function addMissingEntity( $siteId, $title ){ + public function addMissingEntity( $missingDetails ){ //@todo fix Bug 45509 (useless missing attribute in xml...) $this->getResult()->addValue( 'entities', - (string)($this->missingEntityCounter), - array( 'site' => $siteId, 'title' => $title, 'missing' => "" ) + (string)( $this->missingEntityCounter ), + array_merge( $missingDetails, array( 'missing' => "" ) ) ); $this->missingEntityCounter--; } diff --git a/repo/tests/phpunit/includes/api/ItemByTitleHelperTest.php b/repo/tests/phpunit/includes/api/ItemByTitleHelperTest.php index 2f58d02..df3a1a3 100644 --- a/repo/tests/phpunit/includes/api/ItemByTitleHelperTest.php +++ b/repo/tests/phpunit/includes/api/ItemByTitleHelperTest.php @@ -2,6 +2,7 @@ namespace Wikibase\Test; +use MediaWikiSite; use Title; use Wikibase\Api\ItemByTitleHelper; use Wikibase\Api\ResultBuilder; @@ -25,7 +26,7 @@ class ItemByTitleHelperTest extends \MediaWikiTestCase { public function getSiteStoreMock() { - $dummySite = new \MediaWikiSite(); + $dummySite = new MediaWikiSite(); $siteStoreMock = $this->getMockBuilder( '\SiteStore' ) ->disableOriginalConstructor() @@ -41,17 +42,13 @@ /** * Gets a mock ResultBuilder object which excepts a certain number of calls to certain methods * - * @param int $expectedMissingEntities number of expected call to this method * @param int $expectedNormalizedTitle number of expected call to this method - * * @return ResultBuilder */ - public function getResultBuilderMock( $expectedMissingEntities = 0, $expectedNormalizedTitle = 0) { + public function getResultBuilderMock( $expectedNormalizedTitle = 0 ) { $apiResultBuilderMock = $this->getMockBuilder( 'Wikibase\Api\ResultBuilder' ) ->disableOriginalConstructor() ->getMock(); - $apiResultBuilderMock->expects( $this->exactly( $expectedMissingEntities ) ) - ->method( 'addMissingEntity' ); $apiResultBuilderMock->expects( $this->exactly( $expectedNormalizedTitle ) ) ->method( 'addNormalizedTitle' ); @@ -92,7 +89,7 @@ $sites = array( 'FooSite' ); $titles = array( 'Berlin', 'London' ); - $entityIds = $itemByTitleHelper->getEntityIds( $sites, $titles, false ); + list( $entityIds, ) = $itemByTitleHelper->getItemIds( $sites, $titles, false ); foreach( $entityIds as $entityId ) { $this->assertEquals( $expectedEntityId, $entityId ); @@ -105,7 +102,7 @@ public function testGetEntityIdNormalized() { $itemByTitleHelper = new ItemByTitleHelper( // Two values should be added: The normalization and the failure to find an entity - $this->getResultBuilderMock( 1, 1 ), + $this->getResultBuilderMock( 1 ), $this->getSiteLinkCacheMock( false ), $this->getSiteStoreMock(), new StringNormalizer() @@ -114,7 +111,7 @@ $sites = array( 'FooSite' ); $titles = array( 'berlin_germany' ); - $entityIds = $itemByTitleHelper->getEntityIds( $sites, $titles, true ); + list( $entityIds, ) = $itemByTitleHelper->getItemIds( $sites, $titles, true ); // Still nothing could be found $this->assertEquals( array(), $entityIds ); @@ -127,7 +124,7 @@ public function testGetEntityIdsNotFound() { $itemByTitleHelper = new ItemByTitleHelper( // Two result values should be added (for both titles which wont be found) - $this->getResultBuilderMock( 2 ), + $this->getResultBuilderMock(), $this->getSiteLinkCacheMock( false ), $this->getSiteStoreMock(), new StringNormalizer() @@ -136,7 +133,7 @@ $sites = array( 'FooSite' ); $titles = array( 'Berlin', 'London' ); - $itemByTitleHelper->getEntityIds( $sites, $titles, false ); + $itemByTitleHelper->getItemIds( $sites, $titles, false ); } /** @@ -155,7 +152,7 @@ $sites = array( 'FooSite' ); $titles = array( 'Berlin', 'London' ); - $itemByTitleHelper->getEntityIds( $sites, $titles, true ); + $itemByTitleHelper->getItemIds( $sites, $titles, true ); } static public function normalizeTitleProvider() { @@ -179,18 +176,16 @@ * @dataProvider normalizeTitleProvider */ public function testNormalizeTitle( $title, $expectedEntityId, $expectedAddNormalizedCalls ) { - $dummySite = new \MediaWikiSite(); + $dummySite = new MediaWikiSite(); $itemByTitleHelper = new ItemByTitleHelper( - $this->getResultBuilderMock( 0, $expectedAddNormalizedCalls ), + $this->getResultBuilderMock( $expectedAddNormalizedCalls ), $this->getSiteLinkCacheMock( $expectedEntityId ), $this->getSiteStoreMock(), new StringNormalizer() ); - $entityId = $itemByTitleHelper->normalizeTitle( $title, $dummySite ); - - $this->assertEquals( $expectedEntityId, $entityId ); + $itemByTitleHelper->normalizeTitle( $title, $dummySite ); // Normalization in unit tests is actually using Title::getPrefixedText instead of a real API call // XXX: The Normalized title is passed by via reference to $title... @@ -207,7 +202,7 @@ new StringNormalizer() ); - $itemByTitleHelper->getEntityIds( array( ), array( 'barfoo' ), false ); + $itemByTitleHelper->getItemIds( array( ), array( 'barfoo' ), false ); } } diff --git a/repo/tests/phpunit/includes/api/ResultBuilderTest.php b/repo/tests/phpunit/includes/api/ResultBuilderTest.php index 860cd1e..378a081 100644 --- a/repo/tests/phpunit/includes/api/ResultBuilderTest.php +++ b/repo/tests/phpunit/includes/api/ResultBuilderTest.php @@ -301,7 +301,7 @@ $resultBuilder = $this->getResultBuilder( $result ); foreach( $missingEntities as $missingDetails ){ - $resultBuilder->addMissingEntity( $missingDetails['siteId'], $missingDetails['title'] ); + $resultBuilder->addMissingEntity( $missingDetails ); } $this->assertEquals( $expected, $result->getData() ); @@ -311,7 +311,7 @@ return array( array( array( - array( 'siteId' => 'enwiki', 'title' => 'Berlin'), + array( 'site' => 'enwiki', 'title' => 'Berlin'), ), array( 'entities' => array( @@ -326,8 +326,8 @@ ), array( array( - array( 'siteId' => 'enwiki', 'title' => 'Berlin'), - array( 'siteId' => 'dewiki', 'title' => 'Foo'), + array( 'site' => 'enwiki', 'title' => 'Berlin'), + array( 'site' => 'dewiki', 'title' => 'Foo'), ), array( 'entities' => array( -- To view, visit https://gerrit.wikimedia.org/r/94930 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I7314f865075ebd9d2957892078b3267fa0ab3ddf Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/Wikibase Gerrit-Branch: master Gerrit-Owner: Addshore <[email protected]> _______________________________________________ MediaWiki-commits mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
