jenkins-bot has submitted this change and it was merged. Change subject: Rework and clean up TermIndex interfaces ......................................................................
Rework and clean up TermIndex interfaces This is a direct follow up to all my comments in Ie0d92ae. Main reason for this patch is the removal of the redundant $entityType parameter. DEPLOYMENT: Needs Version 2.0.5 of PropertySuggester, see https://github.com/Wikidata-lib/PropertySuggester/pull/115 Change-Id: Ibec48cf6b33017336f73463e608b88c09e718993 --- M client/includes/api/PageTerms.php M client/tests/phpunit/includes/api/PageTermsTest.php M lib/includes/store/BufferingTermLookup.php M lib/includes/store/TermIndex.php M lib/includes/store/sql/TermSqlIndex.php M lib/tests/phpunit/store/MockTermIndex.php M lib/tests/phpunit/store/TermIndexTest.php M repo/includes/api/SearchEntities.php 8 files changed, 98 insertions(+), 98 deletions(-) Approvals: Daniel Kinzler: Looks good to me, approved jenkins-bot: Verified diff --git a/client/includes/api/PageTerms.php b/client/includes/api/PageTerms.php index 39329d4..da7d169 100644 --- a/client/includes/api/PageTerms.php +++ b/client/includes/api/PageTerms.php @@ -8,8 +8,8 @@ use ApiResult; use InvalidArgumentException; use Title; -use Wikibase\Store\EntityIdLookup; use Wikibase\DataModel\Entity\EntityId; +use Wikibase\Store\EntityIdLookup; use Wikibase\Term; use Wikibase\TermIndex; @@ -108,10 +108,10 @@ $entityIdGroups = $this->splitPageEntityMapByType( $entityIds ); $terms = array(); - foreach ( $entityIdGroups as $entityType => $entityIds ) { + foreach ( $entityIdGroups as $entityIds ) { $terms = array_merge( $terms, - $this->termIndex->getTermsOfEntities( $entityIds, $entityType, $termTypes, $languageCodes ) + $this->termIndex->getTermsOfEntities( $entityIds, $termTypes, $languageCodes ) ); } diff --git a/client/tests/phpunit/includes/api/PageTermsTest.php b/client/tests/phpunit/includes/api/PageTermsTest.php index 3b8d3c0..fe6124f 100644 --- a/client/tests/phpunit/includes/api/PageTermsTest.php +++ b/client/tests/phpunit/includes/api/PageTermsTest.php @@ -108,7 +108,7 @@ } /** - * @param array $terms + * @param array[] $terms * * @return TermIndex */ @@ -121,14 +121,14 @@ $termObjectsByEntityId[$key] = $this->makeTermsFromGroups( $entityId, $termGroups ); } - $this_ = $this; + $self = $this; $termIndex = $this->getMock( 'Wikibase\TermIndex' ); $termIndex->expects( $this->any() ) ->method( 'getTermsOfEntities' ) ->will( $this->returnCallback( - function ( array $entityIds, $entityType, $termTypes = null, $languages = null ) use( $termObjectsByEntityId, $this_ ) { - return $this_->getTermsOfEntities( $termObjectsByEntityId, $entityIds, $entityType, $termTypes, $languages ); + function( array $entityIds, array $termTypes = null, array $languagesCodes = null ) use ( $termObjectsByEntityId, $self ) { + return $self->getTermsOfEntities( $termObjectsByEntityId, $entityIds, $termTypes, $languagesCodes ); } ) ); @@ -139,14 +139,14 @@ * @note Public only because older PHP versions don't allow it to be called * from a closure otherwise. * - * @param array $termObjectsByEntityId + * @param array[] $termObjectsByEntityId * @param EntityId[] $entityIds - * @param string $entityType - * @param string|null $language + * @param string[]|null $termTypes + * @param string[]|null $languageCodes * * @return Term[] */ - public function getTermsOfEntities( $termObjectsByEntityId, $entityIds, $entityType, $termTypes, $languages ) { + public function getTermsOfEntities( array $termObjectsByEntityId, array $entityIds, array $termTypes = null, array $languageCodes = null ) { $result = array(); foreach ( $entityIds as $id ) { @@ -158,11 +158,9 @@ /** @var Term $term */ foreach ( $termObjectsByEntityId[$key] as $term ) { - if ( $languages !== null && !in_array( $term->getLanguage(), $languages ) ) { - continue; - } - - if ( $termTypes !== null && !in_array( $term->getType(), $termTypes ) ) { + if ( ( is_array( $termTypes ) && !in_array( $term->getType(), $termTypes ) ) + || ( is_array( $languageCodes ) && !in_array( $term->getLanguage(), $languageCodes ) ) + ) { continue; } @@ -175,9 +173,9 @@ /** * @param EntityId $entityId - * @param array $termGroups + * @param array[] $termGroups * - * @return array + * @return Term[] */ private function makeTermsFromGroups( EntityId $entityId, $termGroups ) { $terms = array(); @@ -221,9 +219,9 @@ /** * @param array $params - * @param array $terms + * @param array[] $terms * - * @return array + * @return array[] */ private function callApiModule( array $params, array $terms ) { $titles = $this->makeTitles( explode( '|', $params['titles'] ) ); diff --git a/lib/includes/store/BufferingTermLookup.php b/lib/includes/store/BufferingTermLookup.php index d88165e..82cb50d 100644 --- a/lib/includes/store/BufferingTermLookup.php +++ b/lib/includes/store/BufferingTermLookup.php @@ -6,7 +6,6 @@ use Wikibase\DataModel\Entity\EntityId; use Wikibase\Lib\Store\EntityTermLookupBase; use Wikibase\Lib\Store\StorageException; -use Wikibase\Store\TermBuffer; use Wikibase\Term; use Wikibase\TermIndex; use Wikibase\Utils; @@ -129,10 +128,10 @@ $entityIdsByType = $this->groupEntityIds( $entityIds ); $terms = array(); - foreach ( $entityIdsByType as $entityType => $entityIdGroup ) { + foreach ( $entityIdsByType as $entityIdGroup ) { $terms = array_merge( $terms, - $this->termIndex->getTermsOfEntities( $entityIdGroup, $entityType, $termTypes, $languageCodes ) + $this->termIndex->getTermsOfEntities( $entityIdGroup, $termTypes, $languageCodes ) ); } $bufferedKeys = $this->setBufferedTermObjects( $terms ); @@ -249,4 +248,5 @@ return $entityIdsByType; } + } diff --git a/lib/includes/store/TermIndex.php b/lib/includes/store/TermIndex.php index 52d6451..10b9dfb 100644 --- a/lib/includes/store/TermIndex.php +++ b/lib/includes/store/TermIndex.php @@ -52,12 +52,11 @@ /** * Returns the terms stored for the given entities. Can be filtered by language. - * Note that the entities must all be of the given type. + * Note that all entities queried in one call must be of the same type. * * @since 0.4 * - * @param EntityId[] $entityIds - * @param string|null $entityType (deprecated, use null) + * @param EntityId[] $entityIds Entity ids of one type only. * @param string[]|null $termTypes The types of terms to return, e.g. "label", "description", * or "alias". Compare the Term::TYPE_XXX constants. If null, all types are returned. * @param string[]|null $languageCodes The desired languages, given as language codes. @@ -65,7 +64,7 @@ * * @return Term[] */ - public function getTermsOfEntities( array $entityIds, $entityType = null, array $termTypes = null, array $languageCodes = null ); + public function getTermsOfEntities( array $entityIds, array $termTypes = null, array $languageCodes = null ); /** * Returns the terms that match the provided conditions. diff --git a/lib/includes/store/sql/TermSqlIndex.php b/lib/includes/store/sql/TermSqlIndex.php index 32236aa..5ba0980 100644 --- a/lib/includes/store/sql/TermSqlIndex.php +++ b/lib/includes/store/sql/TermSqlIndex.php @@ -399,7 +399,6 @@ ) { return $this->getTermsOfEntities( array( $entityId ), - $entityId->getEntityType(), $termTypes, $languageCodes ); @@ -411,46 +410,59 @@ * @see TermIndex::getTermsOfEntities * * @param EntityId[] $entityIds - * @param string|null $entityType * @param string[]|null $termTypes - * @param string[]|null $languageCodes Language codes + * @param string[]|null $languageCodes * - * @throws \MWException + * @throws MWException * @return Term[] */ - public function getTermsOfEntities( array $entityIds, $entityType = null, array $termTypes = null, + public function getTermsOfEntities( + array $entityIds, + array $termTypes = null, array $languageCodes = null ) { - return $this->fetchTerms( $entityIds, $entityType, $termTypes, $languageCodes ); + return $this->fetchTerms( $entityIds, $termTypes, $languageCodes ); } /** * @param EntityId[] $entityIds - * @param string|null $entityType * @param string[]|null $termTypes * @param string[]|null $languageCodes * - * @throws \MWException + * @throws MWException * @return array */ - private function fetchTerms( array $entityIds, $entityType, array $termTypes = null, + private function fetchTerms( + array $entityIds, + array $termTypes = null, array $languageCodes = null ) { - if ( is_array( $entityIds ) && empty( $entityIds ) ) { - return array(); - } - - if ( is_array( $languageCodes ) && empty( $languageCodes ) ) { - return array(); - } - - if ( is_array( $termTypes ) && empty( $termTypes ) ) { + if ( empty( $entityIds ) + || ( is_array( $termTypes ) && empty( $termTypes ) ) + || ( is_array( $languageCodes ) && empty( $languageCodes ) ) + ) { return array(); } wfProfileIn( __METHOD__ ); - $conditions = array(); + $entityType = null; + $numericIds = array(); + + foreach ( $entityIds as $id ) { + if ( $entityType === null ) { + $entityType = $id->getEntityType(); + } elseif ( $id->getEntityType() !== $entityType ) { + throw new MWException( "ID $id does not refer to an entity of type $entityType" ); + } + + $numericIds[] = $id->getNumericId(); + } + + $conditions = array( + 'term_entity_type' => $entityType, + 'term_entity_id' => $numericIds, + ); if ( $languageCodes !== null ) { $conditions['term_language'] = $languageCodes; @@ -459,21 +471,6 @@ if ( $termTypes !== null ) { $conditions['term_type'] = $termTypes; } - - $numericIds = array(); - foreach ( $entityIds as $entityId ) { - if( $entityType === null ) { - $entityType = $entityId->getEntityType(); - } elseif ( $entityId->getEntityType() !== $entityType ) { - throw new MWException( 'ID ' . $entityId->getSerialization() - . " does not refer to an entity of type $entityType." ); - } - - $numericIds[] = $entityId->getNumericId(); - } - - $conditions['term_entity_type'] = $entityType; - $conditions['term_entity_id'] = $numericIds; $dbr = $this->getReadDb(); @@ -939,7 +936,7 @@ * @param string[] $textsByLanguage A list of texts, or a list of lists of texts (keyed by language on the top level) * @param string $type * - * @throws \InvalidArgumentException + * @throws InvalidArgumentException * @return Term[] */ private function makeQueryTerms( $textsByLanguage, $type ) { diff --git a/lib/tests/phpunit/store/MockTermIndex.php b/lib/tests/phpunit/store/MockTermIndex.php index f025455..39bf096 100644 --- a/lib/tests/phpunit/store/MockTermIndex.php +++ b/lib/tests/phpunit/store/MockTermIndex.php @@ -181,33 +181,28 @@ } /** - * @param EntityId $id + * @param EntityId $entityId * @param string[]|null $termTypes - * @param string[]|null $languages + * @param string[]|null $languageCodes * * @return Term[] */ - public function getTermsOfEntity( EntityId $id, array $termTypes = null, array $languages = null ) { + public function getTermsOfEntity( EntityId $entityId, array $termTypes = null, array $languageCodes = null ) { $matchingTerms = array(); - if ( $termTypes ) { + if ( is_array( $termTypes ) ) { $termTypes = array_flip( $termTypes ); } - if ( $languages ) { - $languages = array_flip( $languages ); + if ( is_array( $languageCodes ) ) { + $languageCodes = array_flip( $languageCodes ); } - foreach( $this->terms as $term ) { - if ( $termTypes !== null && !isset( $termTypes[$term->getType()] ) ) { - continue; - } - - if ( $languages !== null && !isset( $languages[$term->getLanguage()] ) ) { - continue; - } - - if ( !$id->equals( $term->getEntityId() ) ) { + foreach ( $this->terms as $term ) { + if ( ( is_array( $termTypes ) && !isset( $termTypes[$term->getType()] ) ) + || ( is_array( $languageCodes ) && !isset( $languageCodes[$term->getLanguage()] ) ) + || !$entityId->equals( $term->getEntityId() ) + ) { continue; } @@ -219,11 +214,21 @@ /** * @see TermIndex::getTermsOfEntities + * + * @param EntityId[] $entityIds + * @param string[]|null $termTypes + * @param string[]|null $languageCodes + * + * @return Term[] */ - public function getTermsOfEntities( array $entityIds, $entityType = null, array $termTypes = null, array $languageCodes = null ) { + public function getTermsOfEntities( array $entityIds, array $termTypes = null, array $languageCodes = null ) { $terms = array(); + foreach ( $entityIds as $id ) { - $terms = array_merge( $terms, $this->getTermsOfEntity( $id, $termTypes, $languageCodes ) ); + $terms = array_merge( + $terms, + $this->getTermsOfEntity( $id, $termTypes, $languageCodes ) + ); } return $terms; @@ -371,4 +376,5 @@ return false; } + } diff --git a/lib/tests/phpunit/store/TermIndexTest.php b/lib/tests/phpunit/store/TermIndexTest.php index 95e5266..8e42140 100644 --- a/lib/tests/phpunit/store/TermIndexTest.php +++ b/lib/tests/phpunit/store/TermIndexTest.php @@ -568,27 +568,27 @@ $this->assertTrue( $lookup->saveTermsOfEntity( $item1 ) ); $this->assertTrue( $lookup->saveTermsOfEntity( $item2 ) ); - $ids = array( $item1->getId(), $item2->getId() ); + $itemIds = array( $item1->getId(), $item2->getId() ); - $labelTerms = $lookup->getTermsOfEntities( $ids, Item::ENTITY_TYPE, array( Term::TYPE_LABEL ) ); + $labelTerms = $lookup->getTermsOfEntities( $itemIds, array( Term::TYPE_LABEL ) ); $this->assertEquals( 6, count( $labelTerms ), "expected 3 labels" ); - $englishTerms = $lookup->getTermsOfEntities( $ids, Item::ENTITY_TYPE, null, array( 'en' ) ); + $englishTerms = $lookup->getTermsOfEntities( $itemIds, null, array( 'en' ) ); $this->assertEquals( 4, count( $englishTerms ), "expected 2 English terms" ); - $englishTerms = $lookup->getTermsOfEntities( array( $item1->getId() ), Item::ENTITY_TYPE, null, array( 'en' ) ); + $englishTerms = $lookup->getTermsOfEntities( array( $item1->getId() ), null, array( 'en' ) ); $this->assertEquals( 2, count( $englishTerms ), "expected 2 English terms" ); - $germanLabelTerms = $lookup->getTermsOfEntities( $ids, Item::ENTITY_TYPE, array( Term::TYPE_LABEL ), array( 'de' ) ); + $germanLabelTerms = $lookup->getTermsOfEntities( $itemIds, array( Term::TYPE_LABEL ), array( 'de' ) ); $this->assertEquals( 2, count( $germanLabelTerms ), "expected 1 German label" ); - $noTerms = $lookup->getTermsOfEntities( $ids, Item::ENTITY_TYPE, array( Term::TYPE_LABEL ), array() ); + $noTerms = $lookup->getTermsOfEntities( $itemIds, array( Term::TYPE_LABEL ), array() ); $this->assertEmpty( $noTerms, "expected no labels" ); - $noTerms = $lookup->getTermsOfEntities( $ids, Item::ENTITY_TYPE, array(), array( 'de' ) ); + $noTerms = $lookup->getTermsOfEntities( $itemIds, array(), array( 'de' ) ); $this->assertEmpty( $noTerms, "expected no labels" ); - $terms = $lookup->getTermsOfEntities( $ids, Item::ENTITY_TYPE ); + $terms = $lookup->getTermsOfEntities( $itemIds ); $this->assertEquals( 14, count( $terms ), "expected 7 terms for item" ); // make list of strings for easy checking diff --git a/repo/includes/api/SearchEntities.php b/repo/includes/api/SearchEntities.php index 6f89ae1..4564922 100644 --- a/repo/includes/api/SearchEntities.php +++ b/repo/includes/api/SearchEntities.php @@ -124,12 +124,13 @@ } $missing = $required - count( $ids ); - $ids = array_merge( $ids, $this->getRankedMatches( $params['search'], $params['type'], - $params['language'], $missing ) ); + $ids = array_merge( + $ids, + $this->getRankedMatches( $params['search'], $params['type'], $params['language'], $missing ) + ); $ids = array_unique( $ids ); - $entries = $this->getEntries( $ids, $params['search'], $params['type'], - $params['language'] ); + $entries = $this->getEntries( $ids, $params['search'], $params['language'] ); wfProfileOut( __METHOD__ ); return $entries; @@ -197,20 +198,19 @@ } /** - * @param EntityId[] $ids + * @param EntityId[] $entityIds * @param string $search - * @param string $entityType - * @param string|null $language language code + * @param string $languageCode * * @return array[] */ - private function getEntries( array $ids, $search, $entityType, $language ) { + private function getEntries( array $entityIds, $search, $languageCode ) { /** * @var array[] $entries */ $entries = array(); - foreach ( $ids as $id ) { + foreach ( $entityIds as $id ) { $key = $id->getSerialization(); $title = $this->titleLookup->getTitleForId( $id ); $entries[ $key ] = array( @@ -221,7 +221,7 @@ // Find all the remaining terms for the given entities $terms = WikibaseRepo::getDefaultInstance()->getStore()->getTermIndex()->getTermsOfEntities( - $ids, $entityType, null, array( $language ) ); + $entityIds, null, array( $languageCode ) ); // TODO: This needs to be rethought when a different search engine is used $aliasPattern = '/^' . preg_quote( $search, '/' ) . '/i'; -- To view, visit https://gerrit.wikimedia.org/r/179931 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ibec48cf6b33017336f73463e608b88c09e718993 Gerrit-PatchSet: 7 Gerrit-Project: mediawiki/extensions/Wikibase Gerrit-Branch: master Gerrit-Owner: Thiemo Mättig (WMDE) <thiemo.maet...@wikimedia.de> Gerrit-Reviewer: Addshore <addshorew...@gmail.com> Gerrit-Reviewer: Aude <aude.w...@gmail.com> Gerrit-Reviewer: Daniel Kinzler <daniel.kinz...@wikimedia.de> Gerrit-Reviewer: Thiemo Mättig (WMDE) <thiemo.maet...@wikimedia.de> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
