Thiemo Mättig (WMDE) has uploaded a new change for review.
https://gerrit.wikimedia.org/r/186151
Change subject: Make everything in Api\EditEntity private
......................................................................
Make everything in Api\EditEntity private
Plus:
* Made everything private (note that this class does not have
subclasses anyway).
* Rewrote some deprecated code.
* Narrowed method interfaces to use EntityDocument, if possible.
* Added missing type hints to methods.
* Made inline doc tags as specific as possible.
* Droped unused/redundant code.
* Dropped @since tags from private stuff.
* Added missing spaces.
Change-Id: If8fdb6719e141d1569c4b788811df35deb0126e1
---
M repo/includes/api/ApiWikibase.php
M repo/includes/api/EditEntity.php
2 files changed, 125 insertions(+), 117 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase
refs/changes/51/186151/1
diff --git a/repo/includes/api/ApiWikibase.php
b/repo/includes/api/ApiWikibase.php
index 386dd7a..26eead7 100644
--- a/repo/includes/api/ApiWikibase.php
+++ b/repo/includes/api/ApiWikibase.php
@@ -173,14 +173,6 @@
}
/**
- * @see ApiBase::getAllowedParams()
- */
- public function getAllowedParams() {
- return array(
- );
- }
-
- /**
* @see ApiBase::needsToken()
*/
public function needsToken() {
diff --git a/repo/includes/api/EditEntity.php b/repo/includes/api/EditEntity.php
index f4731fd..e429169 100644
--- a/repo/includes/api/EditEntity.php
+++ b/repo/includes/api/EditEntity.php
@@ -8,7 +8,6 @@
use InvalidArgumentException;
use LogicException;
use MWException;
-use Site;
use SiteList;
use Title;
use UsageException;
@@ -19,9 +18,11 @@
use Wikibase\ChangeOp\SiteLinkChangeOpFactory;
use Wikibase\DataModel\Claim\Claim;
use Wikibase\DataModel\Entity\Entity;
+use Wikibase\DataModel\Entity\EntityDocument;
use Wikibase\DataModel\Entity\EntityId;
use Wikibase\DataModel\Entity\Item;
use Wikibase\DataModel\Entity\Property;
+use Wikibase\DataModel\Term\FingerprintProvider;
use Wikibase\EntityFactory;
use Wikibase\Lib\Serializers\SerializerFactory;
use Wikibase\Lib\Store\EntityRevisionLookup;
@@ -30,7 +31,8 @@
use Wikibase\Utils;
/**
- * Derived class for API modules modifying a single entity identified by id
xor a combination of site and page title.
+ * Derived class for API modules modifying a single entity identified by id
xor a combination of
+ * site and page title.
*
* @since 0.1
*
@@ -44,40 +46,31 @@
class EditEntity extends ModifyEntity {
/**
- * @since 0.4
- *
* @var string[]
*/
- protected $validLanguageCodes;
-
- /**
- * @since 0.5
- *
- * @var EntityRevisionLookup
- */
- protected $entityRevisionLookup;
+ private $validLanguageCodes;
/**
* @var FingerprintChangeOpFactory
*/
- protected $termChangeOpFactory;
+ private $termChangeOpFactory;
/**
* @var ClaimChangeOpFactory
*/
- protected $claimChangeOpFactory;
+ private $claimChangeOpFactory;
/**
* @var SiteLinkChangeOpFactory
*/
- protected $siteLinkChangeOpFactory;
+ private $siteLinkChangeOpFactory;
/**
+ * @see ModifyEntity::__construct
+ *
* @param ApiMain $mainModule
* @param string $moduleName
* @param string $modulePrefix
- *
- * @see ApiBase::__construct
*/
public function __construct( ApiMain $mainModule, $moduleName,
$modulePrefix = '' ) {
parent::__construct( $mainModule, $moduleName, $modulePrefix );
@@ -103,7 +96,8 @@
if ( !$this->entityExists( $entity ) ) {
$permissions[] = 'createpage';
- if ( $entity->getType() === 'property' ) {
+
+ if ( $entity instanceof Property ) {
$permissions[] = 'property-create';
}
}
@@ -112,13 +106,13 @@
}
/**
+ * @see ModifyEntity::createEntity
+ *
* @param array $params
*
* @throws UsageException
* @throws LogicException
* @return Entity
- *
- * @see ModifyEntity::createEntity
*/
protected function createEntity( array $params ) {
$type = $params['new'];
@@ -127,7 +121,7 @@
try {
return $entityFactory->newEmpty( $type );
- } catch ( InvalidArgumentException $e ) {
+ } catch ( InvalidArgumentException $ex ) {
$this->dieError( "No such entity type: '$type'",
'no-such-entity-type' );
}
@@ -144,7 +138,7 @@
$hasSiteLinkPart = isset( $params['site'] ) || isset(
$params['title'] );
if ( !( $hasId XOR $hasSiteLink XOR $hasNew ) ) {
- $this->dieError( 'Either provide the item "id" or pairs
of "site" and "title" or a "new" type for an entity' , 'param-missing' );
+ $this->dieError( 'Either provide the item "id" or pairs
of "site" and "title" or a "new" type for an entity', 'param-missing' );
}
if ( $hasId && $hasSiteLink ) {
$this->dieError( "Parameter 'id' and 'site', 'title'
combination are not allowed to be both set in the same request",
'param-illegal' );
@@ -168,13 +162,13 @@
$exists = $this->entityExists( $entity );
if ( $params['clear'] ) {
- if( $params['baserevid'] && $exists ) {
+ if ( $params['baserevid'] && $exists ) {
$latestRevision =
$revisionLookup->getLatestRevisionId(
$entity->getId(),
EntityRevisionLookup::LATEST_FROM_MASTER
);
- if( !$baseRevId === $latestRevision ) {
+ if ( !$baseRevId === $latestRevision ) {
wfProfileOut( __METHOD__ );
$this->dieError(
'Tried to clear entity using
baserevid of entity not equal to current revision',
@@ -186,7 +180,7 @@
}
// if we create a new property, make sure we set the datatype
- if( !$exists && $entity instanceof Property ){
+ if ( !$exists && $entity instanceof Property ) {
if ( !isset( $data['datatype'] ) ) {
wfProfileOut( __METHOD__ );
$this->dieError( 'No datatype given',
'param-illegal' );
@@ -208,9 +202,10 @@
/**
* @param array $params
+ *
* @return Summary
*/
- private function getSummary( $params ) {
+ private function getSummary( array $params ) {
//TODO: Construct a nice and meaningful summary from the
changes that get applied!
// Perhaps that could be based on the resulting diff?]
$summary = $this->createSummary( $params );
@@ -224,11 +219,11 @@
/**
* @param array $data
- * @param Entity $entity
+ * @param EntityDocument $entity
*
* @return ChangeOps
*/
- protected function getChangeOps( array $data, Entity $entity ) {
+ private function getChangeOps( array $data, EntityDocument $entity ) {
$changeOps = new ChangeOps();
//FIXME: Use a ChangeOpBuilder so we can batch fingerprint ops
etc,
@@ -254,7 +249,7 @@
$changeOps->add( $this->getSiteLinksChangeOps(
$data['sitelinks'], $entity ) );
}
- if( array_key_exists( 'claims', $data ) ) {
+ if ( array_key_exists( 'claims', $data ) ) {
$changeOps->add(
$this->getClaimsChangeOps( $data['claims'] )
);
@@ -264,11 +259,11 @@
}
/**
- * @since 0.4
- * @param array $labels
+ * @param array[] $labels
+ *
* @return ChangeOp[]
*/
- protected function getLabelChangeOps( $labels ) {
+ private function getLabelChangeOps( $labels ) {
$labelChangeOps = array();
if ( !is_array( $labels ) ) {
@@ -294,11 +289,11 @@
}
/**
- * @since 0.4
- * @param array $descriptions
+ * @param array[] $descriptions
+ *
* @return ChangeOp[]
*/
- protected function getDescriptionChangeOps( $descriptions ) {
+ private function getDescriptionChangeOps( $descriptions ) {
$descriptionChangeOps = array();
if ( !is_array( $descriptions ) ) {
@@ -324,11 +319,11 @@
}
/**
- * @since 0.4
- * @param array $aliases
+ * @param array[] $aliases
+ *
* @return ChangeOp[]
*/
- protected function getAliasesChangeOps( $aliases ) {
+ private function getAliasesChangeOps( $aliases ) {
if ( !is_array( $aliases ) ) {
$this->dieError( "List of aliases must be an array",
'not-recognized-array' );
}
@@ -340,17 +335,18 @@
}
/**
- * @param array $aliases
- * @return array
+ * @param array[] $aliases
+ *
+ * @return array[]
*/
- protected function getIndexedAliases( array $aliases ) {
+ private function getIndexedAliases( array $aliases ) {
$indexedAliases = array();
foreach ( $aliases as $langCode => $arg ) {
if ( intval( $langCode ) ) {
- $indexedAliases[] = ( array_values($arg) ===
$arg ) ? $arg : array( $arg );
+ $indexedAliases[] = ( array_values( $arg ) ===
$arg ) ? $arg : array( $arg );
} else {
- $indexedAliases[$langCode] = (
array_values($arg) === $arg ) ? $arg : array( $arg );
+ $indexedAliases[$langCode] = ( array_values(
$arg ) === $arg ) ? $arg : array( $arg );
}
}
@@ -358,10 +354,11 @@
}
/**
- * @param array $indexedAliases
+ * @param array[] $indexedAliases
+ *
* @return ChangeOp[]
*/
- protected function getIndexedAliasesChangeOps( array $indexedAliases ) {
+ private function getIndexedAliasesChangeOps( array $indexedAliases ) {
$aliasesChangeOps = array();
foreach ( $indexedAliases as $langCode => $args ) {
$aliasesToSet = array();
@@ -391,14 +388,12 @@
}
/**
- * @since 0.4
- *
- * @param array $siteLinks
+ * @param array[] $siteLinks
* @param Item $item
*
* @return ChangeOp[]
*/
- protected function getSiteLinksChangeOps( $siteLinks, Item $item ) {
+ private function getSiteLinksChangeOps( $siteLinks, Item $item ) {
$siteLinksChangeOps = array();
if ( !is_array( $siteLinks ) ) {
@@ -411,16 +406,14 @@
$this->checkSiteLinks( $arg, $siteId, $sites );
$globalSiteId = $arg['site'];
+ if ( !$sites->hasSite( $globalSiteId ) ) {
+ $this->dieError( "There is no site for global
site id '$globalSiteId'", 'no-such-site' );
+ }
+
+ $linkSite = $sites->getSite( $globalSiteId );
$shouldRemove = array_key_exists( 'remove', $arg )
|| ( !isset( $arg['title'] ) && !isset(
$arg['badges'] ) )
|| ( isset( $arg['title'] ) && $arg['title']
=== '' );
-
- if ( $sites->hasSite( $globalSiteId ) ) {
- $linkSite = $sites->getSite( $globalSiteId );
- } else {
- $this->dieError( "There is no site for global
site id '$globalSiteId'", 'no-such-site' );
- }
- /** @var Site $linkSite */
if ( $shouldRemove ) {
$siteLinksChangeOps[] =
$this->siteLinkChangeOpFactory->newRemoveSiteLinkOp( $globalSiteId );
@@ -442,7 +435,7 @@
} else {
$linkPage = null;
- if ( !$item->hasLinkToSite(
$globalSiteId ) ) {
+ if (
!$item->getSiteLinkList()->hasLinkWithSiteId( $globalSiteId ) ) {
$this->dieMessage(
'no-such-sitelink', $globalSiteId );
}
}
@@ -455,20 +448,19 @@
}
/**
- * @since 0.5
- *
* @param array $claims
+ *
* @return ChangeOp[]
*/
- protected function getClaimsChangeOps( $claims ) {
+ private function getClaimsChangeOps( $claims ) {
if ( !is_array( $claims ) ) {
$this->dieError( "List of claims must be an array",
'not-recognized-array' );
}
$changeOps = array();
//check if the array is associative or in arrays by property
- if( array_keys( $claims ) !== range( 0, count( $claims ) - 1 )
){
- foreach( $claims as $subClaims ){
+ if ( array_keys( $claims ) !== range( 0, count( $claims ) - 1 )
) {
+ foreach ( $claims as $subClaims ) {
$changeOps = array_merge( $changeOps,
$this->getRemoveClaimsChangeOps(
$subClaims ),
$this->getModifyClaimsChangeOps(
$subClaims ) );
@@ -487,14 +479,14 @@
*
* @return ChangeOp[]
*/
- private function getModifyClaimsChangeOps( $claims ){
+ private function getModifyClaimsChangeOps( array $claims ) {
$opsToReturn = array();
$serializerFactory = new SerializerFactory();
$unserializer = $serializerFactory->newUnserializerForClass(
'Wikibase\DataModel\Claim\Claim' );
foreach ( $claims as $claimArray ) {
- if( !array_key_exists( 'remove', $claimArray ) ){
+ if ( !array_key_exists( 'remove', $claimArray ) ) {
try {
$claim =
$unserializer->newFromSerialization( $claimArray );
@@ -520,11 +512,11 @@
*
* @return ChangeOp[]
*/
- private function getRemoveClaimsChangeOps( $claims ) {
+ private function getRemoveClaimsChangeOps( array $claims ) {
$opsToReturn = array();
foreach ( $claims as $claimArray ) {
- if( array_key_exists( 'remove', $claimArray ) ){
- if( array_key_exists( 'id', $claimArray ) ){
+ if ( array_key_exists( 'remove', $claimArray ) ) {
+ if ( array_key_exists( 'id', $claimArray ) ) {
$opsToReturn[] =
$this->claimChangeOpFactory->newRemoveClaimOp( $claimArray['id'] );
} else {
$this->dieError( 'Cannot remove a claim
with no GUID', 'invalid-claim' );
@@ -537,22 +529,28 @@
/**
* @param Entity $entity
*/
- protected function buildResult( Entity $entity ) {
- $this->getResultBuilder()->addLabels( $entity->getLabels(),
'entity' );
- $this->getResultBuilder()->addDescriptions(
$entity->getDescriptions(), 'entity' );
- $this->getResultBuilder()->addAliases(
$entity->getAllAliases(), 'entity' );
+ private function buildResult( Entity $entity ) {
+ $builder = $this->getResultBuilder();
- if ( $entity instanceof Item ) {
- $this->getResultBuilder()->addSiteLinks(
$entity->getSiteLinks(), 'entity' );
+ if ( $entity instanceof FingerprintProvider ) {
+ $fingerprint = $entity->getFingerprint();
+
+ $builder->addLabels(
$fingerprint->getLabels()->toTextArray(), 'entity' );
+ $builder->addDescriptions(
$fingerprint->getDescriptions()->toTextArray(), 'entity' );
+ $builder->addAliases(
$fingerprint->getAliasGroups()->toTextArray(), 'entity' );
}
- $this->getResultBuilder()->addClaims( $entity->getClaims(),
'entity' );
+ if ( $entity instanceof Item ) {
+ $builder->addSiteLinks( $entity->getSiteLinks(),
'entity' );
+ }
+
+ $builder->addClaims( $entity->getClaims(), 'entity' );
}
/**
* @param array $params
*/
- private function validateDataParameter( $params ) {
+ private function validateDataParameter( array $params ) {
if ( !isset( $params['data'] ) ) {
wfProfileOut( __METHOD__ );
$this->dieError( 'No data to operate upon', 'no-data' );
@@ -560,13 +558,11 @@
}
/**
- * @since 0.4
- *
* @param mixed $data
- * @param Entity $entity
- * @param int $revId
+ * @param EntityDocument $entity
+ * @param int $revisionId
*/
- protected function validateDataProperties( $data, Entity $entity,
$revId = 0 ) {
+ private function validateDataProperties( $data, EntityDocument $entity,
$revisionId = 0 ) {
$entityId = $entity->getId();
$title = $entityId === null ? null :
$this->getTitleLookup()->getTitleForId( $entityId );
@@ -597,17 +593,17 @@
$this->checkPageIdProp( $data, $title );
$this->checkNamespaceProp( $data, $title );
$this->checkTitleProp( $data, $title );
- $this->checkRevisionProp( $data, $revId );
+ $this->checkRevisionProp( $data, $revisionId );
}
/**
- * @param array $data
+ * @param mixed $data
* @param array $allowedProps
*/
- protected function checkValidJson( $data, array $allowedProps ) {
+ private function checkValidJson( $data, array $allowedProps ) {
if ( is_null( $data ) ) {
$this->dieError( 'Invalid json: The supplied JSON
structure could not be parsed or '
- . 'recreated as a valid structure' ,
'invalid-json' );
+ . 'recreated as a valid structure',
'invalid-json' );
}
// NOTE: json_decode will decode any JS literal or structure,
not just objects!
@@ -630,9 +626,11 @@
* @param array $data
* @param Title|null $title
*/
- protected function checkPageIdProp( $data, $title ) {
+ private function checkPageIdProp( array $data, Title $title = null ) {
if ( isset( $data['pageid'] )
- && ( is_object( $title ) ? $title->getArticleID() !==
$data['pageid'] : true ) ) {
+ && $title !== null
+ && $title->getArticleID() !== $data['pageid']
+ ) {
$this->dieError(
'Illegal field used in call, "pageid", must
either be correct or not given',
'param-illegal'
@@ -644,10 +642,12 @@
* @param array $data
* @param Title|null $title
*/
- protected function checkNamespaceProp( $data, $title ) {
+ private function checkNamespaceProp( array $data, Title $title = null )
{
// not completely convinced that we can use title to get the
namespace in this case
if ( isset( $data['ns'] )
- && ( is_object( $title ) ? $title->getNamespace() !==
$data['ns'] : true ) ) {
+ && $title !== null
+ && $title->getNamespace() !== $data['ns']
+ ) {
$this->dieError(
'Illegal field used in call: "namespace", must
either be correct or not given',
'param-illegal'
@@ -659,9 +659,11 @@
* @param array $data
* @param Title|null $title
*/
- protected function checkTitleProp( $data, $title ) {
+ private function checkTitleProp( array $data, Title $title = null ) {
if ( isset( $data['title'] )
- && ( is_object( $title ) ? $title->getPrefixedText()
!== $data['title'] : true ) ) {
+ && $title !== null
+ && $title->getPrefixedText() !== $data['title']
+ ) {
$this->dieError(
'Illegal field used in call: "title", must
either be correct or not given',
'param-illegal'
@@ -673,9 +675,11 @@
* @param array $data
* @param int|null $revisionId
*/
- protected function checkRevisionProp( $data, $revisionId ) {
+ private function checkRevisionProp( array $data, $revisionId ) {
if ( isset( $data['lastrevid'] )
- && ( is_int( $revisionId ) ? $revisionId !==
$data['lastrevid'] : true ) ) {
+ && $revisionId !== null
+ && $revisionId !== $data['lastrevid']
+ ) {
$this->dieError(
'Illegal field used in call: "lastrevid", must
either be correct or not given',
'param-illegal'
@@ -683,7 +687,11 @@
}
}
- private function checkEntityId( $data, EntityId $entityId = null ) {
+ /**
+ * @param array $data
+ * @param EntityId|null $entityId
+ */
+ private function checkEntityId( array $data, EntityId $entityId = null
) {
if ( isset( $data['id'] ) ) {
if ( !$entityId ) {
$this->dieError(
@@ -693,7 +701,7 @@
}
$dataId = $this->getIdParser()->parse( $data['id'] );
- if( !$entityId->equals( $dataId ) ) {
+ if ( !$entityId->equals( $dataId ) ) {
$this->dieError(
'Invalid field used in call: "id", must
match id parameter',
'param-invalid'
@@ -702,9 +710,14 @@
}
}
- private function checkEntityType( $data, Entity $entity ) {
+ /**
+ * @param array $data
+ * @param EntityDocument $entity
+ */
+ private function checkEntityType( array $data, EntityDocument $entity )
{
if ( isset( $data['type'] )
- && $entity->getType() !== $data['type'] ) {
+ && $entity->getType() !== $data['type']
+ ) {
$this->dieError(
'Invalid field used in call: "type", must match
type associated with id',
'param-invalid'
@@ -714,8 +727,10 @@
/**
* @see ApiBase::getAllowedParams
+ *
+ * @return array[]
*/
- public function getAllowedParams() {
+ protected function getAllowedParams() {
return array_merge(
parent::getAllowedParams(),
parent::getAllowedParamsForId(),
@@ -737,9 +752,9 @@
}
/**
- * @see ApiBase:getExamplesMessages()
+ * @see ApiBase:getExamplesMessages
*
- * @return array
+ * @return string[]
*/
protected function getExamplesMessages() {
return array(
@@ -771,13 +786,14 @@
/**
* Check some of the supplied data for multilang arg
+ *
* @param array $arg The argument array to verify
* @param string $langCode The language code used in the value part
*/
- public function validateMultilangArgs( $arg, $langCode ) {
+ private function validateMultilangArgs( $arg, $langCode ) {
if ( !is_array( $arg ) ) {
$this->dieError(
- "An array was expected, but not found in the
json for the langCode {$langCode}" ,
+ "An array was expected, but not found in the
json for the langCode {$langCode}",
'not-recognized-array' );
}
@@ -789,7 +805,7 @@
if ( !is_string( $arg['language'] ) ) {
$this->dieError(
- "A string was expected, but not found in the
json for the langCode {$langCode} and argument 'language'" ,
+ "A string was expected, but not found in the
json for the langCode {$langCode} and argument 'language'",
'not-recognized-string' );
}
if ( !is_numeric( $langCode ) ) {
@@ -806,7 +822,7 @@
}
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'" ,
+ "A string was expected, but not found in the
json for the langCode {$langCode} and argument 'value'",
'not-recognized-string' );
}
}
@@ -818,12 +834,12 @@
* @param string $siteCode The site code used in the argument
* @param SiteList $sites The valid site codes as an assoc array
*/
- public function checkSiteLinks( $arg, $siteCode, SiteList &$sites =
null ) {
+ private function checkSiteLinks( $arg, $siteCode, SiteList &$sites =
null ) {
if ( !is_array( $arg ) ) {
- $this->dieError( 'An array was expected, but not found'
, 'not-recognized-array' );
+ $this->dieError( 'An array was expected, but not
found', 'not-recognized-array' );
}
if ( !is_string( $arg['site'] ) ) {
- $this->dieError( 'A string was expected, but not found'
, 'not-recognized-string' );
+ $this->dieError( 'A string was expected, but not
found', 'not-recognized-string' );
}
if ( !is_numeric( $siteCode ) ) {
if ( $siteCode !== $arg['site'] ) {
@@ -834,15 +850,15 @@
$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' );
+ $this->dieError( 'A string was expected, but not
found', 'not-recognized-string' );
}
if ( isset( $arg['badges'] ) ) {
if ( !is_array( $arg['badges'] ) ) {
- $this->dieError( 'Badges: an array was
expected, but not found' , 'not-recognized-array' );
+ $this->dieError( 'Badges: an array was
expected, but not found', 'not-recognized-array' );
} else {
foreach ( $arg['badges'] as $badge ) {
if ( !is_string( $badge ) ) {
- $this->dieError( 'Badges: a
string was expected, but not found' , 'not-recognized-string' );
+ $this->dieError( 'Badges: a
string was expected, but not found', 'not-recognized-string' );
}
}
}
--
To view, visit https://gerrit.wikimedia.org/r/186151
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: If8fdb6719e141d1569c4b788811df35deb0126e1
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
