Thiemo Mättig (WMDE) has uploaded a new change for review. https://gerrit.wikimedia.org/r/180762
Change subject: Refactoring of ChangeOpSiteLink ...................................................................... Refactoring of ChangeOpSiteLink Change-Id: I94006953ede551aa9b301fa4f02a2afe8383b223 --- M repo/includes/ChangeOp/ChangeOpSiteLink.php 1 file changed, 56 insertions(+), 104 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase refs/changes/62/180762/1 diff --git a/repo/includes/ChangeOp/ChangeOpSiteLink.php b/repo/includes/ChangeOp/ChangeOpSiteLink.php index 0cbe137..0375186 100644 --- a/repo/includes/ChangeOp/ChangeOpSiteLink.php +++ b/repo/includes/ChangeOp/ChangeOpSiteLink.php @@ -7,7 +7,7 @@ use Wikibase\DataModel\Entity\Entity; use Wikibase\DataModel\Entity\Item; use Wikibase\DataModel\Entity\ItemId; -use Wikibase\DataModel\SiteLink; +use Wikibase\DataModel\SiteLinkList; use Wikibase\Repo\WikibaseRepo; use Wikibase\Summary; @@ -19,40 +19,35 @@ * @author Tobias Gritschacher < tobias.gritschac...@wikimedia.de > * @author Michał Łazowik * @author Bene* < benestar.wikime...@gmail.com > + * @author Thiemo Mättig */ class ChangeOpSiteLink extends ChangeOpBase { /** - * @since 0.4 - * * @var string */ - protected $siteId; + private $siteId; /** - * @since 0.4 - * * @var string|null */ - protected $pageName; + private $pageName; /** - * @since 0.5 - * * @var ItemId[]|null */ - protected $badges; + private $badges; /** * @since 0.4 * * @param string $siteId * @param string|null $pageName Null to remove the sitelink (if $badges are also null) - * @param array|null $badges Null for no-op + * @param ItemId[]|null $badges Null for no-op * * @throws InvalidArgumentException */ - public function __construct( $siteId, $pageName, $badges = null ) { + public function __construct( $siteId, $pageName, array $badges = null ) { if ( !is_string( $siteId ) ) { throw new InvalidArgumentException( '$siteId needs to be a string' ); } @@ -61,14 +56,8 @@ throw new InvalidArgumentException( '$linkPage needs to be a string or null' ); } - if ( !is_array( $badges ) && $badges !== null ) { - throw new InvalidArgumentException( '$badges need to be an array of ItemIds or null' ); - } - if ( $badges !== null ) { - $this->validateBadges( $badges ); - - $badges = $this->removeDuplicateBadges( $badges ); + $badges = $this->validateBadges( $badges ); } $this->siteId = $siteId; @@ -77,116 +66,80 @@ } /** - * @param array $badges + * @param ItemId[] $badges * * @throws InvalidArgumentException + * @return ItemId[] */ private function validateBadges( array $badges ) { $badgeItems = WikibaseRepo::getDefaultInstance()->getSettings()->getSetting( 'badgeItems' ); + $uniqueBadges = array(); - foreach ( $badges as $badge ) { - if ( !( $badge instanceof ItemId ) ) { - throw new InvalidArgumentException( '$badge needs to be an ItemId' ); + foreach ( $badges as $id ) { + if ( !( $id instanceof ItemId ) ) { + throw new InvalidArgumentException( '$badges needs to be an array of ItemId instances' ); } - if ( !array_key_exists( $badge->getSerialization(), $badgeItems ) ) { - throw new InvalidArgumentException( 'Only items specified in the config can be badges' ); + if ( !array_key_exists( $id->getSerialization(), $badgeItems ) ) { + throw new InvalidArgumentException( 'Only items specified in the badgeItems setting can be badges' ); } + + $uniqueBadges[$id->getSerialization()] = $id; } + + return array_values( $uniqueBadges ); } /** - * Removes duplicate badges from the array and returns the new list of badges. - * - * @param ItemId[] $badges - * - * @return ItemId[] - */ - private function removeDuplicateBadges( array $badges ) { - $final = array(); - foreach ( $badges as $badge ) { - if ( !$this->containsBadge( $final, $badge ) ) { - $final[] = $badge; - } - } - return $final; - } - - /** - * Checks whether the list of badges contains the badge. - * - * @param ItemId[] $badges - * @param ItemId $badge - * - * @return boolean - */ - private function containsBadge( array $badges, ItemId $badge ) { - foreach ( $badges as $other ) { - if ( $badge->equals( $other ) ) { - return true; - } - } - return false; - } - - /** - * Whether an entity's sitelink has badges - * - * @param Entity $entity + * @param SiteLinkList $siteLinks * * @return bool */ - private function entityHasBadges( Entity $entity ) { - return $entity->getSiteLinkList()->hasLinkWithSiteId( $this->siteId ) && - count( $entity->getSiteLinkList()->getBySiteId( $this->siteId )->getBadges() ); + private function badgesAreEmptyAndUnchanged( SiteLinkList $siteLinks ) { + return ( !$siteLinks->hasLinkWithSiteId( $this->siteId ) + || $siteLinks->getBySiteId( $this->siteId )->getBadges() === array() ) + && $this->badges === array(); } /** - * Apply badges to an entity - * - * @param Entity $entity - * @param array &$commentArgs + * @param SiteLinkList $siteLinks * @param string &$action + * @param array &$commentArgs * - * @return array + * @return ItemId[] */ - private function applyBadges( Entity $entity, array &$commentArgs, &$action ) { + private function applyBadges( SiteLinkList $siteLinks, &$action, array &$commentArgs ) { + // If badges are not set in the change make sure they remain intact if ( $this->badges === null ) { - // If badges are not set in the change make sure that they remain intact - if ( $entity->hasLinkToSite( $this->siteId ) ) { - $badges = $entity->getSiteLink( $this->siteId )->getBadges(); - } else { - $badges = array(); - } - } elseif ( !$this->entityHasBadges( $entity, $this->siteId ) && $this->badges === array() ) { - // If we didn't have badges before and aren't adding any, don't claim we changed badges - $badges = array(); - } else { - if ( $this->pageName === null ) { - $action .= '-badges'; - } else { - $action .= '-both'; - } - - $badges = $this->badges; - $commentArgs[] = $badges; + return $siteLinks->hasLinkWithSiteId( $this->siteId ) + ? $siteLinks->getBySiteId( $this->siteId )->getBadges() + : array(); } - return $badges; + if ( $this->badgesAreEmptyAndUnchanged( $siteLinks ) ) { + return array(); + } + + $action .= $this->pageName === null ? '-badges' : '-both'; + $commentArgs[] = $this->badges; + + return $this->badges; } /** - * @see ChangeOp::apply() + * @see ChangeOp::apply */ public function apply( Entity $entity, Summary $summary = null ) { if ( !( $entity instanceof Item ) ) { throw new InvalidArgumentException( 'ChangeOpSiteLink can only be applied to Item instances' ); } + $siteLinks = $entity->getSiteLinkList(); + if ( ( $this->pageName === null && $this->badges === null ) || $this->pageName === '' ) { - if ( $entity->hasLinkToSite( $this->siteId ) ) { - $this->updateSummary( $summary, 'remove', $this->siteId, $entity->getSiteLink( $this->siteId )->getPageName() ); - $entity->removeSiteLink( $this->siteId ); + if ( $siteLinks->hasLinkWithSiteId( $this->siteId ) ) { + $this->updateSummary( $summary, 'remove', $this->siteId, $siteLinks->getBySiteId( $this->siteId )->getPageName() ); + $siteLinks->removeLinkWithSiteId( $this->siteId ); } else { //TODO: throw error, or ignore silently? } @@ -194,31 +147,29 @@ $commentArgs = array(); if ( $this->pageName === null ) { - // If page name is not set (but badges are) make sure that it remains intact - if ( $entity->hasLinkToSite( $this->siteId ) ) { - $pageName = $entity->getSiteLink( $this->siteId )->getPageName(); - } else { + if ( !$siteLinks->hasLinkWithSiteId( $this->siteId ) ) { throw new InvalidArgumentException( 'The sitelink does not exist' ); } + + // If page name is not set (but badges are) make sure that it remains intact + $pageName = $siteLinks->getBySiteId( $this->siteId )->getPageName(); } else { $pageName = $this->pageName; $commentArgs[] = $pageName; } - $action = $entity->hasLinkToSite( $this->siteId ) ? 'set' : 'add'; - - $badges = $this->applyBadges( $entity, $commentArgs, $action ); + $action = $siteLinks->hasLinkWithSiteId( $this->siteId ) ? 'set' : 'add'; + $badges = $this->applyBadges( $siteLinks, $action, $commentArgs ); $this->updateSummary( $summary, $action, $this->siteId, $commentArgs ); - - $entity->addSiteLink( new SiteLink( $this->siteId, $pageName, $badges ) ); + $siteLinks->addNewSiteLink( $this->siteId, $pageName, $badges ); } return true; } /** - * @see ChangeOp::validate() + * @see ChangeOp::validate * * @since 0.5 * @@ -232,4 +183,5 @@ //TODO: move validation logic from apply() here. return parent::validate( $entity ); } + } -- To view, visit https://gerrit.wikimedia.org/r/180762 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I94006953ede551aa9b301fa4f02a2afe8383b223 Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/Wikibase Gerrit-Branch: master Gerrit-Owner: Thiemo Mättig (WMDE) <thiemo.maet...@wikimedia.de> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits