Merge "Prefetch terms of entities mentioned in edit summaries"

wikimedia · Dec 12, 2024 · e9c1e0d · e9c1e0d
2 parents 759cd48 + 3e11a69
commit e9c1e0d
Show file tree

Hide file tree

Showing 11 changed files with 99 additions and 137 deletions.
diff --git a/extension-repo.json b/extension-repo.json
@@ -1183,7 +1183,6 @@
 				"WikibaseRepo.EntityIdLookup",
 				"WikibaseRepo.LanguageFallbackChainFactory",
 				"WikibaseRepo.PrefetchingTermLookup",
-				"WikibaseRepo.Settings",
 				"WikibaseRepo.TermBuffer"
 			]
 		},
@@ -1226,8 +1225,7 @@
 			"services": [
 				"WikibaseRepo.LanguageFallbackChainFactory",
 				"WikibaseRepo.LinkTargetEntityIdLookup",
-				"WikibaseRepo.PrefetchingTermLookup",
-				"WikibaseRepo.Settings"
+				"WikibaseRepo.PrefetchingTermLookup"
 			]
 		},
 		"RecentChangeSave": {

diff --git a/repo/includes/Hooks/DifferenceEngineViewHeaderHookHandler.php b/repo/includes/Hooks/DifferenceEngineViewHeaderHookHandler.php
@@ -9,10 +9,9 @@
 use Wikibase\Lib\LanguageFallbackChainFactory;
 use Wikibase\Lib\SettingsArray;
 use Wikibase\Lib\Store\LinkTargetEntityIdLookup;
-use Wikibase\Repo\FederatedProperties\SummaryParsingPrefetchHelper;
 
 /**
- * Hook for prefetching and handling federated properties before links are rendered.
+ * Hook for prefetching the terms of entities mentioned in edit summaries on diff pages.
  *
  * @license GPL-2.0-or-later
  */
@@ -85,7 +84,7 @@ public function onDifferenceEngineViewHeader( $differenceEngine ) {
 			return;
 		}
 
-		$this->summaryParsingPrefetcher->prefetchFederatedProperties(
+		$this->summaryParsingPrefetcher->prefetchTermsForMentionedEntities(
 			[ $differenceEngine->getOldRevision(), $differenceEngine->getNewRevision() ],
 			$this->languageFallbackChainFactory->newFromContext( $differenceEngine->getContext() )->getFetchLanguageCodes(),
 			[ TermTypes::TYPE_LABEL ]

diff --git a/repo/includes/Hooks/LabelPrefetchHookHandler.php b/repo/includes/Hooks/LabelPrefetchHookHandler.php
@@ -9,11 +9,9 @@
 use Wikibase\DataAccess\PrefetchingTermLookup;
 use Wikibase\DataModel\Services\Term\TermBuffer;
 use Wikibase\Lib\LanguageFallbackChainFactory;
-use Wikibase\Lib\SettingsArray;
 use Wikibase\Lib\Store\EntityIdLookup;
 use Wikibase\Lib\Store\StorageException;
 use Wikibase\Lib\TermIndexEntry;
-use Wikibase\Repo\FederatedProperties\SummaryParsingPrefetchHelper;
 use Wikimedia\Rdbms\IResultWrapper;
 
 /**
@@ -53,15 +51,10 @@ class LabelPrefetchHookHandler implements ChangesListInitRowsHook {
 	 */
 	private $languageFallbackChainFactory;
 
-	/**
-	 * @var bool
-	 */
-	private $federatedPropertiesEnabled;
-
 	/**
 	 * @var SummaryParsingPrefetchHelper
 	 */
-	private $federatedPropertiesPrefetchingHelper;
+	private $summaryParsingPrefetchHelper;
 
 	/**
 	 * @return self
@@ -71,7 +64,6 @@ public static function factory(
 		EntityIdLookup $entityIdLookup,
 		LanguageFallbackChainFactory $languageFallbackChainFactory,
 		PrefetchingTermLookup $prefetchingTermLookup,
-		SettingsArray $repoSettings,
 		TermBuffer $termBuffer
 	): self {
 		$termTypes = [ TermIndexEntry::TYPE_LABEL, TermIndexEntry::TYPE_DESCRIPTION ];
@@ -82,7 +74,6 @@ public static function factory(
 			$titleFactory,
 			$termTypes,
 			$languageFallbackChainFactory,
-			$repoSettings->getSetting( 'federatedPropertiesEnabled' ),
 			new SummaryParsingPrefetchHelper( $prefetchingTermLookup )
 		);
 	}
@@ -93,7 +84,6 @@ public static function factory(
 	 * @param TitleFactory $titleFactory
 	 * @param string[] $termTypes
 	 * @param LanguageFallbackChainFactory $languageFallbackChainFactory
-	 * @param bool $federatedPropertiesEnabled
 	 * @param SummaryParsingPrefetchHelper $summaryParsingPrefetchHelper
 	 */
 	public function __construct(
@@ -102,16 +92,14 @@ public function __construct(
 		TitleFactory $titleFactory,
 		array $termTypes,
 		LanguageFallbackChainFactory $languageFallbackChainFactory,
-		bool $federatedPropertiesEnabled,
 		SummaryParsingPrefetchHelper $summaryParsingPrefetchHelper
 	) {
 		$this->buffer = $buffer;
 		$this->idLookup = $idLookup;
 		$this->titleFactory = $titleFactory;
 		$this->termTypes = $termTypes;
 		$this->languageFallbackChainFactory = $languageFallbackChainFactory;
-		$this->federatedPropertiesEnabled = $federatedPropertiesEnabled;
-		$this->federatedPropertiesPrefetchingHelper = $summaryParsingPrefetchHelper;
+		$this->summaryParsingPrefetchHelper = $summaryParsingPrefetchHelper;
 	}
 
 	/**
@@ -126,13 +114,11 @@ public function onChangesListInitRows( $list, $rows ): void {
 				->getFetchLanguageCodes();
 			$this->buffer->prefetchTerms( $entityIds, $this->termTypes, $languageCodes );
 
-			if ( $this->federatedPropertiesEnabled ) {
-				$this->federatedPropertiesPrefetchingHelper->prefetchFederatedProperties(
-					$rows,
-					$languageCodes,
-					$this->termTypes
-				);
-			}
+			$this->summaryParsingPrefetchHelper->prefetchTermsForMentionedEntities(
+				$rows,
+				$languageCodes,
+				$this->termTypes
+			);
 
 		} catch ( StorageException $ex ) {
 			wfLogWarning( __METHOD__ . ': ' . $ex->getMessage() );

diff --git a/repo/includes/Hooks/PageHistoryPagerHookHandler.php b/repo/includes/Hooks/PageHistoryPagerHookHandler.php
@@ -9,62 +9,40 @@
 use Wikibase\DataAccess\PrefetchingTermLookup;
 use Wikibase\DataModel\Term\TermTypes;
 use Wikibase\Lib\LanguageFallbackChainFactory;
-use Wikibase\Lib\SettingsArray;
 use Wikibase\Lib\Store\LinkTargetEntityIdLookup;
-use Wikibase\Repo\FederatedProperties\SummaryParsingPrefetchHelper;
 use Wikimedia\Rdbms\IResultWrapper;
 
 //phpcs:disable MediaWiki.NamingConventions.LowerCamelFunctionsName.FunctionName
 /**
  * Hook handler for prefetching on history pages.
  *
- * Currently only used when federated properties are enabled.
- *
  * @license GPL-2.0-or-later
  * @author Tobias Andersson
  */
 class PageHistoryPagerHookHandler implements PageHistoryPager__doBatchLookupsHook {
 
-	/** @var bool */
-	private $federatedPropertiesEnabled;
-
-	/** @var LinkTargetEntityIdLookup */
-	private $linkTargetEntityIdLookup;
+	private LinkTargetEntityIdLookup $linkTargetEntityIdLookup;
 
-	/** @var LanguageFallbackChainFactory */
-	private $languageFallbackChainFactory;
+	private LanguageFallbackChainFactory $languageFallbackChainFactory;
 
-	/** @var SummaryParsingPrefetchHelper|null */
-	private $federatedPropertiesPrefetchHelper;
+	private SummaryParsingPrefetchHelper $summaryParsingPrefetchHelper;
 
-	/**
-	 * @param bool $federatedPropertiesEnabled
-	 * @param PrefetchingTermLookup $prefetchingLookup
-	 * @param LinkTargetEntityIdLookup $linkTargetEntityIdLookup
-	 * @param LanguageFallbackChainFactory $languageFallbackChainFactory
-	 */
 	public function __construct(
-		bool $federatedPropertiesEnabled,
 		PrefetchingTermLookup $prefetchingLookup,
 		LinkTargetEntityIdLookup $linkTargetEntityIdLookup,
 		LanguageFallbackChainFactory $languageFallbackChainFactory
 	) {
-		$this->federatedPropertiesEnabled = $federatedPropertiesEnabled;
 		$this->linkTargetEntityIdLookup = $linkTargetEntityIdLookup;
 		$this->languageFallbackChainFactory = $languageFallbackChainFactory;
-		if ( $federatedPropertiesEnabled ) {
-			$this->federatedPropertiesPrefetchHelper = new SummaryParsingPrefetchHelper( $prefetchingLookup );
-		}
+		$this->summaryParsingPrefetchHelper = new SummaryParsingPrefetchHelper( $prefetchingLookup );
 	}
 
 	public static function factory(
 		LanguageFallbackChainFactory $languageFallbackChainFactory,
 		LinkTargetEntityIdLookup $linkTargetEntityIdLookup,
-		PrefetchingTermLookup $prefetchingTermLookup,
-		SettingsArray $repoSettings
+		PrefetchingTermLookup $prefetchingTermLookup
 	): self {
 		return new self(
-			$repoSettings->getSetting( 'federatedPropertiesEnabled' ),
 			$prefetchingTermLookup,
 			$linkTargetEntityIdLookup,
 			$languageFallbackChainFactory,
@@ -76,21 +54,18 @@ public static function factory(
 	 * @param IResultWrapper $result
 	 */
 	public function onPageHistoryPager__doBatchLookups( $pager, $result ) {
-		if ( !$this->federatedPropertiesEnabled ) {
-			return;
-		}
-
 		$entityId = $this->linkTargetEntityIdLookup->getEntityId( $pager->getTitle() );
 		if ( $entityId === null ) {
+			// XXX: This means we only prefetch when showing the edit history of an entity.
 			return;
 		}
 
 		$languageFallbackChain = $this->languageFallbackChainFactory->newFromContext( $pager->getContext() );
 
-		$this->federatedPropertiesPrefetchHelper->prefetchFederatedProperties(
+		$this->summaryParsingPrefetchHelper->prefetchTermsForMentionedEntities(
 			$result,
 			$languageFallbackChain->getFetchLanguageCodes(),
-			[ TermTypes::TYPE_LABEL ]
+			[ TermTypes::TYPE_LABEL, TermTypes::TYPE_DESCRIPTION ]
 		);
 	}
 }
diff --git a/...operties/SummaryParsingPrefetchHelper.php → ...es/Hooks/SummaryParsingPrefetchHelper.php b/...operties/SummaryParsingPrefetchHelper.php → ...es/Hooks/SummaryParsingPrefetchHelper.php
@@ -1,75 +1,81 @@
 <?php
 declare( strict_types = 1 );
 
-namespace Wikibase\Repo\FederatedProperties;
+namespace Wikibase\Repo\Hooks;
 
 use LogicException;
 use MediaWiki\Revision\RevisionRecord;
-use Wikibase\DataAccess\PrefetchingTermLookup;
-use Wikibase\DataModel\Entity\NumericPropertyId;
-use Wikibase\DataModel\Entity\PropertyId;
+use Wikibase\DataModel\Entity\BasicEntityIdParser;
+use Wikibase\DataModel\Entity\EntityId;
+use Wikibase\DataModel\Entity\EntityIdParsingException;
+use Wikibase\DataModel\Services\Term\TermBuffer;
 use Wikimedia\Rdbms\IResultWrapper;
 
 /**
- * A helper class for parsing and prefetching properties from summaries for federated properties
+ * A helper class for parsing and prefetching terms of entities mentioned in edit summaries.
  *
  * @license GPL-2.0-or-later
  */
 class SummaryParsingPrefetchHelper {
 
-	/**
-	 * @var PrefetchingTermLookup
-	 */
-	private $prefetchingLookup;
+	private TermBuffer $termBuffer;
+	private BasicEntityIdParser $entityIdParser;
 
-	public function __construct( PrefetchingTermLookup $prefetchingLookup ) {
-		$this->prefetchingLookup = $prefetchingLookup;
+	public function __construct(
+		TermBuffer $termBuffer
+	) {
+		$this->termBuffer = $termBuffer;
+		$this->entityIdParser = new BasicEntityIdParser();
 	}
 
 	/**
-	 * Matching links to properties in in edit summaries, such as "[[Property:P123]]" or "[[wdbeta:Special:EntityPage/P123]]"
+	 * Matching links to properties in in edit summaries, such as "[[Q23]]", "[[Property:P123]]"
+	 * or "[[wdbeta:Special:EntityPage/P123]]".
 	 */
-	public const PROPERTY_SUMMARY_REGEXP = '/\[\[(\S+)(P[1-9]\d*)\]\]/';
+	public const ENTITY_ID_SUMMARY_REGEXP = '/\[\[(\S*)([PQ][1-9]\d*)\]\]/';
 
 	/**
 	 * @param IResultWrapper|\stdClass[]|RevisionRecord[] $rows
 	 * @param array $languageCodes
 	 * @param array $termTypes
 	 */
-	public function prefetchFederatedProperties( $rows, array $languageCodes, array $termTypes ): void {
-		$resultProperties = $this->extractSummaryProperties( $rows );
-		if ( !$resultProperties ) {
+	public function prefetchTermsForMentionedEntities( $rows, array $languageCodes, array $termTypes ): void {
+		$mentionedEntityIds = $this->extractSummaryMentions( $rows );
+		if ( !$mentionedEntityIds ) {
 			return;
 		}
 
-		$this->prefetchingLookup->prefetchTerms(
-			$resultProperties,
+		$this->termBuffer->prefetchTerms(
+			$mentionedEntityIds,
 			$termTypes,
 			$languageCodes
 		);
 	}
 
 	/**
 	 * @param IResultWrapper|\stdClass[]|RevisionRecord[] $result
-	 * @return PropertyId[]
+	 * @return EntityId[]
 	 */
-	public function extractSummaryProperties( $result ): array {
-		$propertyIds = [];
+	public function extractSummaryMentions( $result ): array {
+		$entityIds = [];
 		foreach ( $result as $revisionRow ) {
 			$comment = $this->getCommentText( $revisionRow );
 			if ( $comment === null ) {
 				continue;
 			}
 
 			$matches = [];
-			preg_match( self::PROPERTY_SUMMARY_REGEXP, $comment, $matches );
-			if ( count( $matches ) === 3 ) {
-				$propertyId = $matches[2];
-				// TODO: Change to FederatedPropertyId when this functionality is supported in Feddy Props v2
-				$propertyIds[] = new NumericPropertyId( $propertyId );
+			if ( !preg_match_all( self::ENTITY_ID_SUMMARY_REGEXP, $comment, $matches, PREG_PATTERN_ORDER ) ) {
+				continue;
+			}
+			foreach ( $matches[2] as $match ) {
+				try {
+					$entityIds[] = $this->entityIdParser->parse( $match );
+				} catch ( EntityIdParsingException $ex ) {
+				}
 			}
 		}
-		return $propertyIds;
+		return $entityIds;
 	}
 
 	/**

diff --git a/.../phpunit/includes/FederatedProperties/Hooks/DifferenceEngineViewHeaderHookHandlerTest.php b/.../phpunit/includes/FederatedProperties/Hooks/DifferenceEngineViewHeaderHookHandlerTest.php
@@ -21,8 +21,8 @@
 use Wikibase\Lib\LanguageWithConversion;
 use Wikibase\Lib\Store\LinkTargetEntityIdLookup;
 use Wikibase\Lib\TermLanguageFallbackChain;
-use Wikibase\Repo\FederatedProperties\SummaryParsingPrefetchHelper;
 use Wikibase\Repo\Hooks\DifferenceEngineViewHeaderHookHandler;
+use Wikibase\Repo\Hooks\SummaryParsingPrefetchHelper;
 
 /**
  * @covers \Wikibase\Repo\Hooks\DifferenceEngineViewHeaderHookHandler

diff --git a/repo/tests/phpunit/includes/FederatedProperties/Hooks/LabelPrefetchHookHandlerTest.php b/repo/tests/phpunit/includes/FederatedProperties/Hooks/LabelPrefetchHookHandlerTest.php
@@ -58,7 +58,6 @@ public function testDoChangesListInitRows() {
 		$itemTwo->getStatements()->addStatement( new Statement( new PropertyNoValueSnak( 3 ) ) );
 
 		$itemThreeNotFoundId = new ItemId( 'Q3' );
-		$itemThreeNotFound = null;
 
 		$expectedItemIds = [
 			$itemOneId,
@@ -85,8 +84,7 @@ public function testDoChangesListInitRows() {
 			$this->getPrefetchTermsCallback( $expectedItemIds, $expectedTermTypes, $expectedLanguageCodes ),
 			$expectedTermTypes,
 			$expectedLanguageCodes,
-			$prefetchingTermLookup,
-			true
+			$prefetchingTermLookup
 		);
 
 		/** @var ChangesList $changesList */

diff --git a/repo/tests/phpunit/includes/Hooks/LabelPrefetchHookHandlerTest.php b/repo/tests/phpunit/includes/Hooks/LabelPrefetchHookHandlerTest.php
@@ -21,9 +21,9 @@ class LabelPrefetchHookHandlerTest extends LabelPrefetchHookHandlerTestBase {
 
 	public function testDoChangesListInitRows() {
 		$rows = [
-			(object)[ 'rc_namespace' => NS_MAIN, 'rc_title' => 'XYZ' ],
-			(object)[ 'rc_namespace' => NS_MAIN, 'rc_title' => 'Q23' ],
-			(object)[ 'rc_namespace' => NS_MAIN, 'rc_title' => 'P55' ],
+			(object)[ 'rc_namespace' => NS_MAIN, 'rc_title' => 'XYZ', 'rc_comment_text' => '' ],
+			(object)[ 'rc_namespace' => NS_MAIN, 'rc_title' => 'Q23', 'rc_comment_text' => 'foo' ],
+			(object)[ 'rc_namespace' => NS_MAIN, 'rc_title' => 'P55', 'rc_comment_text' => '' ],
 		];
 
 		$expectedTermTypes = [ 'label', 'description' ];