Thiemo Mättig (WMDE) has uploaded a new change for review.
https://gerrit.wikimedia.org/r/177206
Change subject: Fix mistakes in AffectedPagesFinder and UsageAspectTransformer
......................................................................
Fix mistakes in AffectedPagesFinder and UsageAspectTransformer
This is a follow up for the small mistakes I found in I9ce6a2e.
Change-Id: Ie620cb12220e672d435bb53c72eb8f15d8ea1ac7
---
M client/includes/Changes/AffectedPagesFinder.php
M client/includes/Usage/NullUsageTracker.php
M client/includes/Usage/UsageAspectTransformer.php
3 files changed, 97 insertions(+), 51 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase
refs/changes/06/177206/1
diff --git a/client/includes/Changes/AffectedPagesFinder.php
b/client/includes/Changes/AffectedPagesFinder.php
index 8355d35..78e7768 100644
--- a/client/includes/Changes/AffectedPagesFinder.php
+++ b/client/includes/Changes/AffectedPagesFinder.php
@@ -18,11 +18,11 @@
use Wikibase\Client\Usage\PageEntityUsages;
use Wikibase\Client\Usage\UsageAspectTransformer;
use Wikibase\Client\Usage\UsageLookup;
-use Wikibase\EntityChange;
-use Wikibase\ItemChange;
use Wikibase\DataModel\Entity\Diff\EntityDiff;
use Wikibase\DataModel\Entity\Diff\ItemDiff;
use Wikibase\DataModel\Entity\EntityId;
+use Wikibase\EntityChange;
+use Wikibase\ItemChange;
use Wikibase\Lib\Store\StorageException;
use Wikibase\NamespaceChecker;
@@ -45,6 +45,11 @@
private $namespaceChecker;
/**
+ * @var TitleFactory
+ */
+ private $titleFactory;
+
+ /**
* @var string
*/
private $siteId;
@@ -55,14 +60,9 @@
private $contentLanguageCode;
/**
- * @var boolean
+ * @var bool
*/
private $checkPageExistence;
-
- /**
- * @var TitleFactory
- */
- private $titleFactory;
/**
* @param UsageLookup $usageLookup
@@ -70,7 +70,7 @@
* @param TitleFactory $titleFactory
* @param string $siteId
* @param string $contentLanguageCode
- * @param boolean $checkPageExistence
+ * @param bool $checkPageExistence
*
* @throws InvalidArgumentException
*/
@@ -96,10 +96,10 @@
$this->usageLookup = $usageLookup;
$this->namespaceChecker = $namespaceChecker;
+ $this->titleFactory = $titleFactory;
$this->siteId = $siteId;
$this->contentLanguageCode = $contentLanguageCode;
$this->checkPageExistence = $checkPageExistence;
- $this->titleFactory = $titleFactory;
}
/**
@@ -110,14 +110,12 @@
* @return Iterator<PageEntityUsages>
*/
public function getAffectedUsagesByPage( Change $change ) {
- if ( ! ( $change instanceof EntityChange ) ) {
- return array();
+ if ( $change instanceof EntityChange ) {
+ $usages = $this->getAffectedPages( $change );
+ return $this->filterUpdates( $usages );
}
- $pageUpdates = $this->getAffectedPages( $change );
- $pageUpdates = $this->filterUpdates( $pageUpdates );
-
- return $pageUpdates;
+ return array();
}
/**
@@ -128,25 +126,26 @@
public function getChangedAspects( EntityChange $change ) {
$aspects = array();
- /** @var EntityDiff $diff */
$diff = $change->getDiff();
$remainingDiffOps = count( $diff ); // this is a "deep" count!
- if ( $diff instanceof ItemDiff &&
!$diff->getSiteLinkDiff()->isEmpty() ) {
- $sitelinkDiff = $diff->getSiteLinkDiff();
+ if ( $diff instanceof ItemDiff &&
$diff->getSiteLinkDiff()->isEmpty() ) {
+ $siteLinkDiff = $diff->getSiteLinkDiff();
$aspects[] = EntityUsage::SITELINK_USAGE;
- $remainingDiffOps-= count( $sitelinkDiff );
+ $remainingDiffOps -= count( $siteLinkDiff );
- if ( isset( $sitelinkDiff[$this->siteId] ) &&
!$this->isBadgesOnlyChange( $sitelinkDiff[$this->siteId] ) ) {
+ if ( isset( $siteLinkDiff[$this->siteId] )
+ && !$this->isBadgesOnlyChange(
$siteLinkDiff[$this->siteId] )
+ ) {
$aspects[] = EntityUsage::TITLE_USAGE;
}
}
- if ( !$diff->getLabelsDiff()->isEmpty() ) {
- $labelDiff = $diff->getLabelsDiff();
+ if ( $diff instanceof EntityDiff &&
!$diff->getLabelsDiff()->isEmpty() ) {
+ $labelsDiff = $diff->getLabelsDiff();
- if ( isset( $labelDiff[$this->contentLanguageCode] ) ) {
+ if ( isset( $labelsDiff[$this->contentLanguageCode] ) )
{
$aspects[] = EntityUsage::LABEL_USAGE;
$remainingDiffOps--;
}
@@ -156,7 +155,6 @@
$aspects[] = EntityUsage::OTHER_USAGE;
}
- sort( $aspects );
return $aspects;
}
@@ -168,23 +166,26 @@
* @return Iterator<PageEntityUsages>
*/
private function getAffectedPages( EntityChange $change ) {
- $itemId = $change->getEntityId();
+ $entityId = $change->getEntityId();
$changedAspects = $this->getChangedAspects( $change );
- // @todo: more than one item at once!
- $relevantAspects = array_merge( array( 'X' ), $changedAspects
); // X implies all!
- $usages = $this->usageLookup->getPagesUsing( array( $itemId ),
$relevantAspects );
+ $usages = $this->usageLookup->getPagesUsing(
+ // @todo: more than one entity at once!
+ array( $entityId ),
+ // Look up pages that are marked as either using one of
the changed or all aspects
+ $changedAspects + array( EntityUsage::ALL_USAGE )
+ );
// @todo: use iterators throughout!
$usages = iterator_to_array( $usages, true );
- $usages = $this->transformAllPageEntityUsages( $usages,
$itemId, $changedAspects );
+ $usages = $this->transformAllPageEntityUsages( $usages,
$entityId, $changedAspects );
if ( $change instanceof ItemChange && in_array(
EntityUsage::TITLE_USAGE, $changedAspects ) ) {
$siteLinkDiff = $change->getSiteLinkDiff();
$namesFromDiff = $this->getPagesReferencedInDiff(
$siteLinkDiff );
$titlesFromDiff = $this->getTitlesFromTexts(
$namesFromDiff );
- $usagesFromDiff = $this->makeVirtualUsages(
$titlesFromDiff, $itemId, array( EntityUsage::SITELINK_USAGE ) );
+ $usagesFromDiff = $this->makeVirtualUsages(
$titlesFromDiff, $entityId, array( EntityUsage::SITELINK_USAGE ) );
//FIXME: we can't really merge if $usages is an
iterator, not an array.
//TODO: Inject $usagesFromDiff "on the fly" while
streaming other usages.
@@ -199,11 +200,10 @@
}
/**
- * @param PageEntityUsages[] $from PageEntityUsages
+ * @param PageEntityUsages[] $from
* @param PageEntityUsages[] &$into Array to merge into
*/
private function mergeUsagesInto( array $from, array &$into ) {
- /** @var PageEntityUsages $pageEntityUsages */
foreach ( $from as $pageEntityUsages ) {
$key = $pageEntityUsages->getPageId();
@@ -219,7 +219,7 @@
* @param Diff $siteLinkDiff
*
* @throws UnexpectedValueException
- * @return array
+ * @return string[]
*/
private function getPagesReferencedInDiff( Diff $siteLinkDiff ) {
$pagesToUpdate = array();
@@ -228,7 +228,7 @@
// containing map diffs. For B/C, handle both cases.
$siteLinkDiffOp = $siteLinkDiff[$this->siteId];
- if ( ( $siteLinkDiffOp instanceof Diff ) && ( array_key_exists(
'name', $siteLinkDiffOp ) ) ) {
+ if ( $siteLinkDiffOp instanceof Diff && array_key_exists(
'name', $siteLinkDiffOp ) ) {
$siteLinkDiffOp = $siteLinkDiffOp['name'];
}
@@ -251,11 +251,10 @@
/**
* @param DiffOp $siteLinkDiffOp
*
- * @return boolean
+ * @return bool
*/
private function isBadgesOnlyChange( DiffOp $siteLinkDiffOp ) {
-
- return ( $siteLinkDiffOp instanceof Diff && !array_key_exists(
'name', $siteLinkDiffOp ) );
+ return $siteLinkDiffOp instanceof Diff && !array_key_exists(
'name', $siteLinkDiffOp );
}
/**
@@ -266,11 +265,12 @@
*
* @return Iterator<PageEntityUsages>
*/
- private function filterUpdates( $updates ) {
+ private function filterUpdates( $usages ) {
$titlesToUpdate = array();
- foreach ( $updates as $pageUpdates ) {
- $title = $this->titleFactory->newFromID(
$pageUpdates->getPageId() );
+ /** @var PageEntityUsages $pageEntityUsages */
+ foreach ( $usages as $pageEntityUsages ) {
+ $title = $this->titleFactory->newFromID(
$pageEntityUsages->getPageId() );
if ( $this->checkPageExistence && !$title->exists() ) {
continue;
@@ -283,7 +283,7 @@
}
$key = $title->getArticleID();
- $titlesToUpdate[$key] = $pageUpdates;
+ $titlesToUpdate[$key] = $pageEntityUsages;
}
return new ArrayIterator( $titlesToUpdate );
@@ -294,7 +294,7 @@
*
* @return Title[]
*/
- private function getTitlesFromTexts( $names ) {
+ private function getTitlesFromTexts( array $names ) {
$titles = array();
foreach ( $names as $name ) {
@@ -323,21 +323,29 @@
$usagesPerPage = array();
foreach ( $titles as $title ) {
- $pid = $title->getArticleID();
- $usagesPerPage[$pid] = new PageEntityUsages( $pid,
$usagesForItem );
+ $pageId = $title->getArticleID();
+ $usagesPerPage[$pageId] = new PageEntityUsages(
$pageId, $usagesForItem );
}
return $usagesPerPage;
}
+ /**
+ * @param PageEntityUsages[] $usages
+ * @param EntityId $entityId
+ * @param string[] $changedAspects
+ *
+ * @return PageEntityUsages[]
+ */
private function transformAllPageEntityUsages( array $usages, EntityId
$entityId, array $changedAspects ) {
$aspectTransformer = new UsageAspectTransformer();
$aspectTransformer->setRelevantAspects( $entityId,
$changedAspects );
$transformed = array();
- foreach( $usages as $key => $usagesOnPage ) {
+ foreach ( $usages as $key => $usagesOnPage ) {
$transformedUsagesOnPage =
$aspectTransformer->transformPageEntityUsages( $usagesOnPage );
+
if ( !$transformedUsagesOnPage->isEmpty() ) {
$transformed[$key] = $transformedUsagesOnPage;
}
diff --git a/client/includes/Usage/NullUsageTracker.php
b/client/includes/Usage/NullUsageTracker.php
index db6bf3b..352ec0a 100644
--- a/client/includes/Usage/NullUsageTracker.php
+++ b/client/includes/Usage/NullUsageTracker.php
@@ -3,6 +3,8 @@
namespace Wikibase\Client\Usage;
use ArrayIterator;
+use Iterator;
+use Wikibase\DataModel\Entity\EntityId;
/**
* No-op implementation of the UsageTracker and UsageLookup interfaces.
@@ -12,23 +14,59 @@
*/
class NullUsageTracker implements UsageTracker, UsageLookup {
+ /**
+ * @see UsageTracker::trackUsedEntities
+ *
+ * @param int $pageId
+ * @param EntityUsage[] $usages
+ *
+ * @return EntityUsage[]
+ */
public function trackUsedEntities( $pageId, array $usages ) {
return array();
}
- public function removeEntities( array $entities ) {
+ /**
+ * @see UsageTracker::removeEntities
+ *
+ * @param EntityId[] $entityIds
+ */
+ public function removeEntities( array $entityIds ) {
// no-op
}
+ /**
+ * @see UsageTracker::getUsagesForPage
+ *
+ * @param int $pageId
+ *
+ * @return EntityUsage[]
+ */
public function getUsagesForPage( $pageId ) {
return array();
}
- public function getUnusedEntities( array $entities ) {
+ /**
+ * @see UsageLookup::getUnusedEntities
+ *
+ * @param EntityId[] $entityIds
+ *
+ * @return EntityId[]
+ */
+ public function getUnusedEntities( array $entityIds ) {
return array();
}
+ /**
+ * @see UsageLookup::getPagesUsing
+ *
+ * @param EntityId[] $entities
+ * @param string[] $aspects
+ *
+ * @return Iterator<PageEntityUsages>
+ */
public function getPagesUsing( array $entities, array $aspects =
array() ) {
return new ArrayIterator( array() );
}
+
}
diff --git a/client/includes/Usage/UsageAspectTransformer.php
b/client/includes/Usage/UsageAspectTransformer.php
index 83df0ad..06cbff0 100644
--- a/client/includes/Usage/UsageAspectTransformer.php
+++ b/client/includes/Usage/UsageAspectTransformer.php
@@ -32,7 +32,7 @@
/**
* @param EntityId $entityId
- * @param array[] $aspects
+ * @param string[] $aspects
*/
public function setRelevantAspects( EntityId $entityId, array $aspects
) {
$key = $entityId->getSerialization();
@@ -61,7 +61,7 @@
* @param EntityId $entityId
* @param string[] $aspects
*
- * @return EntityUsage[] $usages;
+ * @return EntityUsage[]
*/
public function getFilteredUsages( EntityId $entityId, array $aspects )
{
$relevant = $this->getRelevantAspects( $entityId );
@@ -96,7 +96,7 @@
/**
* @param EntityId $entityId
- * @param array[] $aspects
+ * @param string[] $aspects
*
* @return EntityUsage[]
*/
--
To view, visit https://gerrit.wikimedia.org/r/177206
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie620cb12220e672d435bb53c72eb8f15d8ea1ac7
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
