Thiemo Mättig (WMDE) has uploaded a new change for review. https://gerrit.wikimedia.org/r/245895
Change subject: Add FIXMEs/refactor DataUpdates related interfaces and classes ...................................................................... Add FIXMEs/refactor DataUpdates related interfaces and classes This is the essence of all the comments written on the four basic patches * I1618217, * Ic0974d0, * I7b03cc0 and * If4ec903 that introduced this infrastructure and started using it. Warning, this does not include all the comments written on https://github.com/wmde/WikibaseDataModelServices/pull/75 Still to do: * Find a better namespace, possibly "ParserOutput" (suggested by Aude). * Find a better split for all the stuff that's currently going on in EntityParserOutputGenerator and EntityParserOutputDataUpdater. The current split and boundaries between these two classes are not the best we can do. * Introduce a more general StatementMatcher interface and make use of it, and make the PropertyDataTypeMatcher an implementation of that. * Find better class names for several of these classes and interfaces. Bug: T114220 Change-Id: I1aa589038bc018ad243fbaa101a5bbcd9e844281 --- M lib/includes/store/PropertyDataTypeMatcher.php M repo/includes/DataUpdates/EntityParserOutputDataUpdater.php M repo/includes/DataUpdates/ParserOutputDataUpdate.php M repo/includes/DataUpdates/ReferencedEntitiesDataUpdate.php M repo/includes/EntityParserOutputGenerator.php M repo/includes/EntityParserOutputGeneratorFactory.php M repo/tests/phpunit/includes/EntityParserOutputGeneratorTest.php 7 files changed, 135 insertions(+), 72 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase refs/changes/95/245895/1 diff --git a/lib/includes/store/PropertyDataTypeMatcher.php b/lib/includes/store/PropertyDataTypeMatcher.php index 96e8926..06e681f 100644 --- a/lib/includes/store/PropertyDataTypeMatcher.php +++ b/lib/includes/store/PropertyDataTypeMatcher.php @@ -10,6 +10,16 @@ * Check if a PropertyId is for a Property with a specific data type. * As well, in-process caching of the lookups is done for performance reasons. * + * @fixme The caching aspect of this class should be extracted to a + * CachingPropertyDataTypeLookup( PropertyDataTypeLookup, BagOStuff ) + * This new class should live in Data Model Services. + * @see https://github.com/wmde/WikibaseDataModelServices/pull/75 + * + * @fixme The matching aspect of this class should be reworked into a StatementMatcher interface. + * The only method in this interface would be a matches( Statement ). One implementation of this + * interface can be a DataTypeStatementMatcher, other uses include a CommonsMediaStatementMatcher + * and an ImageStatementMatcher. + * * @since 0.5 * * @license GNU GPL v2+ diff --git a/repo/includes/DataUpdates/EntityParserOutputDataUpdater.php b/repo/includes/DataUpdates/EntityParserOutputDataUpdater.php index 3b63ef3..970ce36 100644 --- a/repo/includes/DataUpdates/EntityParserOutputDataUpdater.php +++ b/repo/includes/DataUpdates/EntityParserOutputDataUpdater.php @@ -11,6 +11,10 @@ /** * @todo have ItemParserOutputDataUpdate, etc. instead. * + * @fixme The split between EntityParserOutputDataUpdater and EntityParserOutputGenerator is + * arbitrary. Which concerns belong where, and how is that reflected by the names? + * @see https://gerrit.wikimedia.org/r/#/c/243613/15/repo/includes/EntityParserOutputGenerator.php + * * @since 0.5 * * @license GNU GPL v2+ @@ -27,30 +31,30 @@ /** * @var ParserOutputDataUpdate[] */ - private $dataUpdates; + private $dataUpdaters; /** * @var StatementDataUpdate[] */ - private $statementDataUpdates = array(); + private $statementDataUpdaters = array(); /** * @var SiteLinkDataUpdate[] */ - private $siteLinkDataUpdates = array(); + private $siteLinkDataUpdaters = array(); /** * @param ParserOutput $parserOutput - * @param ParserOutputDataUpdate[] $dataUpdates + * @param ParserOutputDataUpdate[] $dataUpdaters * * @throws InvalidArgumentException */ - public function __construct( ParserOutput $parserOutput, array $dataUpdates ) { - foreach ( $dataUpdates as $dataUpdate ) { + public function __construct( ParserOutput $parserOutput, array $dataUpdaters ) { + foreach ( $dataUpdaters as $dataUpdate ) { if ( $dataUpdate instanceof StatementDataUpdate ) { - $this->statementDataUpdates[] = $dataUpdate; + $this->statementDataUpdaters[] = $dataUpdate; } elseif ( $dataUpdate instanceof SiteLinkDataUpdate ) { - $this->siteLinkDataUpdates[] = $dataUpdate; + $this->siteLinkDataUpdaters[] = $dataUpdate; } else { throw new InvalidArgumentException( 'Each $dataUpdates element must be a ' . 'StatementDataUpdate, SiteLinkDataUpdate or both' ); @@ -58,7 +62,7 @@ } $this->parserOutput = $parserOutput; - $this->dataUpdates = $dataUpdates; + $this->dataUpdaters = $dataUpdaters; } /** @@ -78,12 +82,12 @@ * @param StatementListProvider $entity */ private function processStatementListProvider( StatementListProvider $entity ) { - if ( empty( $this->statementDataUpdates ) ) { + if ( empty( $this->statementDataUpdaters ) ) { return; } foreach ( $entity->getStatements() as $statement ) { - foreach ( $this->statementDataUpdates as $dataUpdate ) { + foreach ( $this->statementDataUpdaters as $dataUpdate ) { $dataUpdate->processStatement( $statement ); } } @@ -93,19 +97,19 @@ * @param Item $item */ private function processItem( Item $item ) { - if ( empty( $this->siteLinkDataUpdates ) ) { + if ( empty( $this->siteLinkDataUpdaters ) ) { return; } foreach ( $item->getSiteLinkList() as $siteLink ) { - foreach ( $this->siteLinkDataUpdates as $dataUpdate ) { + foreach ( $this->siteLinkDataUpdaters as $dataUpdate ) { $dataUpdate->processSiteLink( $siteLink ); } } } public function flush() { - foreach ( $this->dataUpdates as $dataUpdate ) { + foreach ( $this->dataUpdaters as $dataUpdate ) { $dataUpdate->updateParserOutput( $this->parserOutput ); } } diff --git a/repo/includes/DataUpdates/ParserOutputDataUpdate.php b/repo/includes/DataUpdates/ParserOutputDataUpdate.php index 56946cd..5adcd92 100644 --- a/repo/includes/DataUpdates/ParserOutputDataUpdate.php +++ b/repo/includes/DataUpdates/ParserOutputDataUpdate.php @@ -17,6 +17,12 @@ * These updates are invoked when EntityContent::getParserOutput is called. * * @param ParserOutput $parserOutput + * + * @fixme This should be turned into a flush() method with no parameter. The ParserOutput would + * then be a constructor parameter in all implementations of this interface. This also means + * that the concrete updaters must be constructed inside of the EntityParserOutputGenerator + * class. Otherwise they can not get the ParserOutput during construction time. Or this needs an + * additional factory. */ public function updateParserOutput( ParserOutput $parserOutput ); diff --git a/repo/includes/DataUpdates/ReferencedEntitiesDataUpdate.php b/repo/includes/DataUpdates/ReferencedEntitiesDataUpdate.php index 7f3a72b..ecc304d 100644 --- a/repo/includes/DataUpdates/ReferencedEntitiesDataUpdate.php +++ b/repo/includes/DataUpdates/ReferencedEntitiesDataUpdate.php @@ -146,8 +146,13 @@ * @param ParserOutput $parserOutput */ public function updateParserOutput( ParserOutput $parserOutput ) { - // needed and used in EntityParserOutputGenerator, for getEntityInfo, - // to allow this data to be accessed later in processing. + /** + * Needed and used in EntityParserOutputGenerator, for getEntityInfo, to allow this data to + * be accessed later in processing. + * + * @see EntityParserOutputGenerator::getEntityInfo + * @fixme Simply add a public method self::getEntityIds or similar instead. + */ $parserOutput->setExtensionData( 'referenced-entities', $this->entityIds ); $this->addLinksToParserOutput( $parserOutput ); } diff --git a/repo/includes/EntityParserOutputGenerator.php b/repo/includes/EntityParserOutputGenerator.php index c95d50e..7f3936e 100644 --- a/repo/includes/EntityParserOutputGenerator.php +++ b/repo/includes/EntityParserOutputGenerator.php @@ -8,7 +8,9 @@ use SpecialPage; use Wikibase\DataModel\Entity\EntityDocument; use Wikibase\DataModel\Entity\EntityId; +use Wikibase\DataModel\Entity\EntityIdParser; use Wikibase\DataModel\Entity\Item; +use Wikibase\DataModel\Services\Lookup\PropertyDataTypeLookup; use Wikibase\DataModel\SiteLink; use Wikibase\DataModel\SiteLinkList; use Wikibase\DataModel\Statement\StatementList; @@ -19,9 +21,13 @@ use Wikibase\Lib\Store\EntityInfoTermLookup; use Wikibase\Lib\Store\EntityTitleLookup; use Wikibase\Lib\Store\LanguageFallbackLabelDescriptionLookup; +use Wikibase\Lib\Store\PropertyDataTypeMatcher; use Wikibase\Repo\DataUpdates\EntityParserOutputDataUpdater; +use Wikibase\Repo\DataUpdates\ExternalLinksDataUpdate; +use Wikibase\Repo\DataUpdates\GeoDataDataUpdate; +use Wikibase\Repo\DataUpdates\ImageLinksDataUpdate; use Wikibase\Repo\DataUpdates\PageImagesDataUpdate; -use Wikibase\Repo\DataUpdates\ParserOutputDataUpdate; +use Wikibase\Repo\DataUpdates\ReferencedEntitiesDataUpdate; use Wikibase\Repo\LinkedData\EntityDataFormatProvider; use Wikibase\Repo\View\RepoSpecialPageLinker; use Wikibase\Repo\WikibaseRepo; @@ -80,15 +86,38 @@ private $entityDataFormatProvider; /** - * @var ParserOutputDataUpdate[] + * @var PropertyDataTypeLookup */ - private $dataUpdates; + private $propertyDataTypeLookup; + + /** + * @var EntityIdParser + */ + private $externalEntityIdParser; + + /** + * @var string[] + */ + private $preferredGeoDataProperties; /** * @var string */ private $languageCode; + /** + * @param EntityViewFactory $entityViewFactory + * @param ParserOutputJsConfigBuilder $configBuilder + * @param EntityTitleLookup $entityTitleLookup + * @param EntityInfoBuilderFactory $entityInfoBuilderFactory + * @param LanguageFallbackChain $languageFallbackChain + * @param TemplateFactory $templateFactory + * @param EntityDataFormatProvider $entityDataFormatProvider + * @param PropertyDataTypeLookup $propertyDataTypeLookup + * @param EntityIdParser $externalEntityIdParser + * @param string[] $preferredGeoDataProperties + * @param string $languageCode + */ public function __construct( EntityViewFactory $entityViewFactory, ParserOutputJsConfigBuilder $configBuilder, @@ -97,7 +126,9 @@ LanguageFallbackChain $languageFallbackChain, TemplateFactory $templateFactory, EntityDataFormatProvider $entityDataFormatProvider, - array $dataUpdates, + PropertyDataTypeLookup $propertyDataTypeLookup, + EntityIdParser $externalEntityIdParser, + array $preferredGeoDataProperties, $languageCode ) { $this->entityViewFactory = $entityViewFactory; @@ -108,7 +139,9 @@ $this->languageCode = $languageCode; $this->templateFactory = $templateFactory; $this->entityDataFormatProvider = $entityDataFormatProvider; - $this->dataUpdates = $dataUpdates; + $this->propertyDataTypeLookup = $propertyDataTypeLookup; + $this->externalEntityIdParser = $externalEntityIdParser; + $this->preferredGeoDataProperties = $preferredGeoDataProperties; $this->languageCode = $languageCode; } @@ -146,7 +179,7 @@ $entity = $entityRevision->getEntity(); - $dataUpdater = new EntityParserOutputDataUpdater( $parserOutput, $this->dataUpdates ); + $dataUpdater = $this->getEntityParserOutputDataUpdater( $parserOutput ); $dataUpdater->processEntity( $entity ); $dataUpdater->flush(); @@ -194,6 +227,36 @@ return $parserOutput; } + private function getEntityParserOutputDataUpdater( ParserOutput $parserOutput ) { + $propertyDataTypeMatcher = new PropertyDataTypeMatcher( $this->propertyDataTypeLookup ); + + /** + * @fixme Each updater should get the ParserOutput as its first constructor parameter. Thats + * the reason why this array is constructed here and not in the factory. + * @see ParserOutputDataUpdate + */ + $dataUpdaters = array( + new ReferencedEntitiesDataUpdate( + $this->entityTitleLookup, + $this->externalEntityIdParser + ), + new ExternalLinksDataUpdate( $propertyDataTypeMatcher ), + new ImageLinksDataUpdate( $propertyDataTypeMatcher ) + ); + + if ( class_exists( 'GeoData' ) ) { + $dataUpdaters[] = new GeoDataDataUpdate( + $propertyDataTypeMatcher, + $this->preferredGeoDataProperties + ); + } + + return new EntityParserOutputDataUpdater( $parserOutput, $dataUpdaters ); + } + + /** + * @fixme Turn this into an updater and add it to the updaters array above. + */ private function addBestImageToParserOutput( ParserOutput $parserOutput, StatementList $statements ) { $repo = WikibaseRepo::getDefaultInstance(); // TODO: Inject this setting! @@ -215,7 +278,13 @@ * @return EntityInfo */ private function getEntityInfo( ParserOutput $parserOutput ) { - // set in ReferencedEntitiesDataUpdate + /** + * Set in ReferencedEntitiesDataUpdate. + * + * @see ReferencedEntitiesDataUpdate::updateParserOutput + * @fixme Simply add a public method ReferencedEntitiesDataUpdate::getEntityIds or similar + * to the data updater that does the job of finding all used entities. + */ $entityIds = $parserOutput->getExtensionData( 'referenced-entities' ); if ( !is_array( $entityIds ) ) { @@ -272,7 +341,7 @@ if ( !is_string( $titleText ) ) { $entityId = $entity->getId(); - if ( $entityId !== null ) { + if ( $entityId instanceof EntityId ) { $titleText = $entityId->getSerialization(); } } diff --git a/repo/includes/EntityParserOutputGeneratorFactory.php b/repo/includes/EntityParserOutputGeneratorFactory.php index 7e0fbd4..09be174 100644 --- a/repo/includes/EntityParserOutputGeneratorFactory.php +++ b/repo/includes/EntityParserOutputGeneratorFactory.php @@ -7,12 +7,6 @@ use Wikibase\DataModel\Services\Lookup\PropertyDataTypeLookup; use Wikibase\Lib\Store\EntityInfoBuilderFactory; use Wikibase\Lib\Store\EntityTitleLookup; -use Wikibase\Lib\Store\PropertyDataTypeMatcher; -use Wikibase\Repo\DataUpdates\ExternalLinksDataUpdate; -use Wikibase\Repo\DataUpdates\GeoDataDataUpdate; -use Wikibase\Repo\DataUpdates\ImageLinksDataUpdate; -use Wikibase\Repo\DataUpdates\ParserOutputDataUpdate; -use Wikibase\Repo\DataUpdates\ReferencedEntitiesDataUpdate; use Wikibase\Repo\LinkedData\EntityDataFormatProvider; use Wikibase\View\EntityViewFactory; use Wikibase\View\Template\TemplateFactory; @@ -70,6 +64,17 @@ */ private $preferredGeoDataProperties; + /** + * @param EntityViewFactory $entityViewFactory + * @param EntityInfoBuilderFactory $entityInfoBuilderFactory + * @param EntityTitleLookup $entityTitleLookup + * @param LanguageFallbackChainFactory $languageFallbackChainFactory + * @param TemplateFactory $templateFactory + * @param EntityDataFormatProvider $entityDataFormatProvider + * @param PropertyDataTypeLookup $propertyDataTypeLookup + * @param EntityIdParser $externalEntityIdParser + * @param string[] $preferredGeoDataProperties + */ public function __construct( EntityViewFactory $entityViewFactory, EntityInfoBuilderFactory $entityInfoBuilderFactory, @@ -110,7 +115,9 @@ $this->getLanguageFallbackChain( $languageCode ), $this->templateFactory, $this->entityDataFormatProvider, - $this->getDataUpdates(), + $this->propertyDataTypeLookup, + $this->externalEntityIdParser, + $this->preferredGeoDataProperties, $languageCode ); } @@ -133,31 +140,6 @@ return $this->languageFallbackChainFactory->newFromLanguageCode( $languageCode ); - } - - /** - * @return ParserOutputDataUpdate[] - */ - private function getDataUpdates() { - $propertyDataTypeMatcher = new PropertyDataTypeMatcher( $this->propertyDataTypeLookup ); - - $dataUpdates = array( - new ReferencedEntitiesDataUpdate( - $this->entityTitleLookup, - $this->externalEntityIdParser - ), - new ExternalLinksDataUpdate( $propertyDataTypeMatcher ), - new ImageLinksDataUpdate( $propertyDataTypeMatcher ) - ); - - if ( class_exists( 'GeoData' ) ) { - $dataUpdates[] = new GeoDataDataUpdate( - $propertyDataTypeMatcher, - $this->preferredGeoDataProperties - ); - } - - return $dataUpdates; } } diff --git a/repo/tests/phpunit/includes/EntityParserOutputGeneratorTest.php b/repo/tests/phpunit/includes/EntityParserOutputGeneratorTest.php index 7540e31..e550c56 100644 --- a/repo/tests/phpunit/includes/EntityParserOutputGeneratorTest.php +++ b/repo/tests/phpunit/includes/EntityParserOutputGeneratorTest.php @@ -17,11 +17,7 @@ use Wikibase\DataModel\Snak\PropertyValueSnak; use Wikibase\EntityParserOutputGenerator; use Wikibase\EntityRevision; -use Wikibase\Lib\Store\PropertyDataTypeMatcher; use Wikibase\Lib\Store\Sql\SqlEntityInfoBuilderFactory; -use Wikibase\Repo\DataUpdates\ExternalLinksDataUpdate; -use Wikibase\Repo\DataUpdates\ImageLinksDataUpdate; -use Wikibase\Repo\DataUpdates\ReferencedEntitiesDataUpdate; use Wikibase\Repo\LinkedData\EntityDataFormatProvider; use Wikibase\View\Template\TemplateFactory; @@ -141,17 +137,6 @@ $entityTitleLookup = $this->getEntityTitleLookupMock(); - $propertyDataTypeMatcher = new PropertyDataTypeMatcher( $this->getPropertyDataTypeLookup() ); - - $dataUpdates = array( - new ExternalLinksDataUpdate( $propertyDataTypeMatcher ), - new ImageLinksDataUpdate( $propertyDataTypeMatcher ), - new ReferencedEntitiesDataUpdate( - $entityTitleLookup, - new BasicEntityIdParser() - ) - ); - return new EntityParserOutputGenerator( $this->getEntityViewFactory(), $this->getConfigBuilderMock(), @@ -160,7 +145,9 @@ $this->newLanguageFallbackChain(), TemplateFactory::getDefaultInstance(), $entityDataFormatProvider, - $dataUpdates, + $this->getPropertyDataTypeLookup(), + new BasicEntityIdParser(), + array(), 'en' ); } -- To view, visit https://gerrit.wikimedia.org/r/245895 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I1aa589038bc018ad243fbaa101a5bbcd9e844281 Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/Wikibase Gerrit-Branch: master Gerrit-Owner: Thiemo Mättig (WMDE) <[email protected]> _______________________________________________ MediaWiki-commits mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
