jenkins-bot has submitted this change and it was merged.
Change subject: Rework and cleanup ApiWikibase and subclasses
......................................................................
Rework and cleanup ApiWikibase and subclasses
Main changes are:
* Narrow interfaces from Entity to EntityDocument.
* Remove hard coded lists of entity types if possible.
* Make stuff private if possible.
* Fix docs.
Change-Id: Ic506adc1145d04e3f4af8f0aa5d4b65ea0d4de28
---
M repo/includes/EditEntity.php
M repo/includes/api/ApiWikibase.php
M repo/includes/api/EditEntity.php
M repo/includes/api/MergeItems.php
M repo/includes/api/ModifyEntity.php
M repo/includes/api/ModifyTerm.php
M repo/includes/api/SetAliases.php
7 files changed, 63 insertions(+), 78 deletions(-)
Approvals:
Daniel Kinzler: Looks good to me, approved
JanZerebecki: Looks good to me, approved
jenkins-bot: Verified
diff --git a/repo/includes/EditEntity.php b/repo/includes/EditEntity.php
index cae22a7..72bc997 100644
--- a/repo/includes/EditEntity.php
+++ b/repo/includes/EditEntity.php
@@ -65,9 +65,9 @@
private $baseRev = null;
/**
- * @var int|null
+ * @var int|bool
*/
- private $baseRevId = null;
+ private $baseRevId;
/**
* @var EntityRevision|null
@@ -187,7 +187,7 @@
$baseRevId = intval( $baseRevId );
}
- if ( $baseRevId === '' || $baseRevId === 0 ) {
+ if ( $baseRevId === 0 ) {
$baseRevId = false;
}
diff --git a/repo/includes/api/ApiWikibase.php
b/repo/includes/api/ApiWikibase.php
index 207d251..dd2cc60 100644
--- a/repo/includes/api/ApiWikibase.php
+++ b/repo/includes/api/ApiWikibase.php
@@ -10,6 +10,7 @@
use UsageException;
use User;
use Wikibase\DataModel\Entity\Entity;
+use Wikibase\DataModel\Entity\EntityDocument;
use Wikibase\DataModel\Entity\EntityId;
use Wikibase\DataModel\Entity\EntityIdParser;
use Wikibase\DataModel\Entity\PropertyDataTypeLookup;
@@ -174,6 +175,8 @@
/**
* @see ApiBase::needsToken()
+ *
+ * @return string|false
*/
public function needsToken() {
return $this->isWriteMode() ? 'csrf' : false;
@@ -181,6 +184,8 @@
/**
* @see ApiBase::getTokenSalt()
+ *
+ * @return string|false
*/
public function getTokenSalt() {
return $this->needsToken() ? '' : false;
@@ -188,6 +193,8 @@
/**
* @see ApiBase::mustBePosted()
+ *
+ * @return bool
*/
public function mustBePosted() {
return $this->isWriteMode();
@@ -195,6 +202,8 @@
/**
* @see ApiBase::isReadMode
+ *
+ * @return bool Always true in this abstract base implementation.
*/
public function isReadMode() {
return true;
@@ -207,12 +216,11 @@
* Per default, this will include the 'read' permission if
$this->isReadMode() returns true,
* and the 'edit' permission if $this->isWriteMode() returns true,
*
- * @param Entity $entity The entity to check permissions for
- * @param array $params Arguments for the module, describing the
operation to be performed
+ * @param EntityDocument $entity The entity to check permissions for
*
* @return string[] A list of permissions
*/
- protected function getRequiredPermissions( Entity $entity, array
$params ) {
+ protected function getRequiredPermissions( EntityDocument $entity ) {
$permissions = array();
if ( $this->isReadMode() ) {
@@ -231,13 +239,12 @@
*
* @param $entity Entity the entity to check
* @param $user User doing the action
- * @param $params array of arguments for the module, passed for
ModifyItem
*
* @return Status the check's result
* @todo: use this also to check for read access in ApiGetEntities, etc
*/
- protected function checkPermissions( Entity $entity, User $user, array
$params ) {
- $permissions = $this->getRequiredPermissions( $entity, $params
);
+ protected function checkPermissions( Entity $entity, User $user ) {
+ $permissions = $this->getRequiredPermissions( $entity );
$status = Status::newGood();
foreach ( array_unique( $permissions ) as $perm ) {
@@ -393,7 +400,7 @@
$flags |= EDIT_FORCE_BOT;
}
- $baseRevisionId = $this->evaluateBaseRevisionParam( $params );
+ $baseRevisionId = isset( $params['baserevid'] ) ? intval(
$params['baserevid'] ) : null;
$editEntity = new EditEntity(
$this->titleLookup,
@@ -421,30 +428,16 @@
/**
* @param array $params
*
- * @return false|null|string
+ * @return string|bool|null Token string, or false if not needed, or
null if not set.
*/
private function evaluateTokenParam( array $params ) {
if ( !$this->needsToken() ) {
- // false disabled the token check
- $token = false;
- } else {
- // null fails the token check
- $token = isset( $params['token'] ) ? $params['token'] :
null;
+ // False disables the token check.
+ return false;
}
- return $token;
- }
-
- /**
- * @param array $params
- *
- * @return null|false|int
- */
- private function evaluateBaseRevisionParam( array $params ) {
- $baseRevisionId = isset( $params['baserevid'] ) ? intval(
$params['baserevid'] ) : null;
- $baseRevisionId = $baseRevisionId > 0 ? $baseRevisionId : false;
-
- return $baseRevisionId;
+ // Null fails the token check.
+ return isset( $params['token'] ) ? $params['token'] : null;
}
/**
diff --git a/repo/includes/api/EditEntity.php b/repo/includes/api/EditEntity.php
index 3181c72..d4ff97a 100644
--- a/repo/includes/api/EditEntity.php
+++ b/repo/includes/api/EditEntity.php
@@ -70,6 +70,8 @@
* @param ApiMain $mainModule
* @param string $moduleName
* @param string $modulePrefix
+ *
+ * @throws MWException
*/
public function __construct( ApiMain $mainModule, $moduleName,
$modulePrefix = '' ) {
parent::__construct( $mainModule, $moduleName, $modulePrefix );
@@ -85,19 +87,21 @@
/**
* @see ApiWikibase::getRequiredPermissions
*
- * @param Entity $entity
- * @param array $params
+ * @param EntityDocument $entity
*
+ * @throws InvalidArgumentException
* @return string[]
*/
- protected function getRequiredPermissions( Entity $entity, array
$params ) {
- $permissions = parent::getRequiredPermissions( $entity, $params
);
+ protected function getRequiredPermissions( EntityDocument $entity ) {
+ $permissions = parent::getRequiredPermissions( $entity );
if ( !$this->entityExists( $entity ) ) {
$permissions[] = 'createpage';
- if ( $entity instanceof Property ) {
- $permissions[] = 'property-create';
+ switch ( $entity->getType() ) {
+ case 'property':
+ $permissions[] = $entity->getType() .
'-create'; //property-create
+ break;
}
}
@@ -797,10 +801,9 @@
}
if ( !$this->termsLanguages->hasLanguage( $arg['language'] ) ) {
- $this->dieError(
- "unknown language: {$arg['language']}",
- 'not-recognized-language' );
+ $this->dieError( 'Unknown language: ' .
$arg['language'], 'not-recognized-language' );
}
+
if ( !array_key_exists( 'remove', $arg ) && !is_string(
$arg['value'] ) ) {
$this->dieError(
"A string was expected, but not found in the
json for the langCode {$langCode} and argument 'value'",
@@ -813,7 +816,7 @@
*
* @param array $arg The argument array to verify
* @param string $siteCode The site code used in the argument
- * @param SiteList $sites The valid site codes as an assoc array
+ * @param SiteList|null $sites The valid sites.
*/
private function checkSiteLinks( $arg, $siteCode, SiteList &$sites =
null ) {
if ( !is_array( $arg ) ) {
@@ -827,8 +830,8 @@
$this->dieError( "inconsistent site:
{$siteCode} is not equal to {$arg['site']}", 'inconsistent-site' );
}
}
- if ( isset( $sites ) && !$sites->hasSite( $arg['site'] ) ) {
- $this->dieError( "unknown site: {$arg['site']}",
'not-recognized-site' );
+ if ( $sites !== null && !$sites->hasSite( $arg['site'] ) ) {
+ $this->dieError( 'Unknown site: ' . $arg['site'],
'not-recognized-site' );
}
if ( isset( $arg['title'] ) && !is_string( $arg['title'] ) ) {
$this->dieError( 'A string was expected, but not
found', 'not-recognized-string' );
diff --git a/repo/includes/api/MergeItems.php b/repo/includes/api/MergeItems.php
index fa66c54..4133c14 100644
--- a/repo/includes/api/MergeItems.php
+++ b/repo/includes/api/MergeItems.php
@@ -4,6 +4,7 @@
use ApiBase;
use ApiMain;
+use InvalidArgumentException;
use LogicException;
use UsageException;
use Wikibase\DataModel\Entity\EntityIdParser;
@@ -101,9 +102,9 @@
try {
return new ItemId( $value );
- } catch ( \InvalidArgumentException $ex ) {
+ } catch ( InvalidArgumentException $ex ) {
$this->errorReporter->dieError( $ex->getMessage(),
'invalid-entity-id' );
- throw new \LogicException( 'ErrorReporter::dieError did
not throw an exception' );
+ throw new LogicException( 'ErrorReporter::dieError did
not throw an exception' );
}
}
@@ -225,7 +226,8 @@
/**
* @see ApiBase::isWriteMode
- * @return bool true
+ *
+ * @return bool Always true.
*/
public function isWriteMode() {
return true;
diff --git a/repo/includes/api/ModifyEntity.php
b/repo/includes/api/ModifyEntity.php
index c39adcf..a40af79 100644
--- a/repo/includes/api/ModifyEntity.php
+++ b/repo/includes/api/ModifyEntity.php
@@ -341,7 +341,7 @@
}
// At this point only change/edit rights should be checked
- $status = $this->checkPermissions( $entity, $user, $params );
+ $status = $this->checkPermissions( $entity, $user );
if ( !$status->isOK() ) {
$this->dieError( 'You do not have sufficient
permissions', 'permissiondenied' );
@@ -403,6 +403,8 @@
/**
* @see ApiBase::isWriteMode()
+ *
+ * @return bool Always true.
*/
public function isWriteMode() {
return true;
diff --git a/repo/includes/api/ModifyTerm.php b/repo/includes/api/ModifyTerm.php
index 526ef62..d664bcd 100644
--- a/repo/includes/api/ModifyTerm.php
+++ b/repo/includes/api/ModifyTerm.php
@@ -4,10 +4,7 @@
use ApiBase;
use InvalidArgumentException;
-use Status;
-use Wikibase\DataModel\Entity\Entity;
-use Wikibase\DataModel\Entity\Item;
-use Wikibase\DataModel\Entity\Property;
+use Wikibase\DataModel\Entity\EntityDocument;
use Wikibase\Repo\WikibaseRepo;
use Wikibase\Summary;
@@ -47,22 +44,14 @@
/**
* @see ApiWikibase::getRequiredPermissions
*
- * @param Entity $entity
- * @param array $params
+ * @param EntityDocument $entity
*
* @throws InvalidArgumentException
* @return string[]
*/
- protected function getRequiredPermissions( Entity $entity, array
$params ) {
- $permissions = parent::getRequiredPermissions( $entity, $params
);
- if( $entity instanceof Item ) {
- $type = 'item';
- } else if ( $entity instanceof Property ) {
- $type = 'property';
- } else {
- throw new InvalidArgumentException( 'Unexpected Entity
type when checking special page term change permissions' );
- }
- $permissions[] = $type . '-term';
+ protected function getRequiredPermissions( EntityDocument $entity ) {
+ $permissions = parent::getRequiredPermissions( $entity );
+ $permissions[] = $entity->getType() . '-term';
return $permissions;
}
diff --git a/repo/includes/api/SetAliases.php b/repo/includes/api/SetAliases.php
index 845e7f9..bc8d740 100644
--- a/repo/includes/api/SetAliases.php
+++ b/repo/includes/api/SetAliases.php
@@ -5,14 +5,12 @@
use ApiBase;
use ApiMain;
use InvalidArgumentException;
-use Status;
use Wikibase\ChangeOp\ChangeOp;
use Wikibase\ChangeOp\ChangeOpAliases;
use Wikibase\ChangeOp\ChangeOps;
use Wikibase\ChangeOp\FingerprintChangeOpFactory;
use Wikibase\DataModel\Entity\Entity;
-use Wikibase\DataModel\Entity\Item;
-use Wikibase\DataModel\Entity\Property;
+use Wikibase\DataModel\Entity\EntityDocument;
use Wikibase\Repo\WikibaseRepo;
/**
@@ -49,22 +47,14 @@
/**
* @see ApiWikibase::getRequiredPermissions
*
- * @param Entity $entity
- * @param array $params
+ * @param EntityDocument $entity
*
* @throws InvalidArgumentException
* @return string[]
*/
- protected function getRequiredPermissions( Entity $entity, array
$params ) {
- $permissions = parent::getRequiredPermissions( $entity, $params
);
- if( $entity instanceof Item ) {
- $type = 'item';
- } else if ( $entity instanceof Property ) {
- $type = 'property';
- } else {
- throw new InvalidArgumentException( 'Unexpected Entity
type when checking special page term change permissions' );
- }
- $permissions[] = $type . '-term';
+ protected function getRequiredPermissions( EntityDocument $entity ) {
+ $permissions = parent::getRequiredPermissions( $entity );
+ $permissions[] = $entity->getType() . '-term';
return $permissions;
}
@@ -83,7 +73,7 @@
* @see ModifyEntity::createEntity
*/
protected function createEntity( array $params ) {
- $this->dieError( 'Could not find an existing entity' ,
'no-such-entity' );
+ $this->dieError( 'Could not find an existing entity',
'no-such-entity' );
}
/**
@@ -120,7 +110,12 @@
return $summary;
}
- private function normalizeAliases( $aliases ) {
+ /**
+ * @param string[] $aliases
+ *
+ * @return string[]
+ */
+ private function normalizeAliases( array $aliases ) {
$stringNormalizer = $this->stringNormalizer; // hack for PHP
fail.
$aliases = array_map(
@@ -142,6 +137,7 @@
/**
* @param array $params
+ *
* @return ChangeOpAliases
*/
private function getChangeOps( array $params ) {
--
To view, visit https://gerrit.wikimedia.org/r/179881
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic506adc1145d04e3f4af8f0aa5d4b65ea0d4de28
Gerrit-PatchSet: 5
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: Thiemo Mättig (WMDE) <[email protected]>
Gerrit-Reviewer: Addshore <[email protected]>
Gerrit-Reviewer: Aude <[email protected]>
Gerrit-Reviewer: Daniel Kinzler <[email protected]>
Gerrit-Reviewer: JanZerebecki <[email protected]>
Gerrit-Reviewer: Jeroen De Dauw <[email protected]>
Gerrit-Reviewer: Thiemo Mättig (WMDE) <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
