Thiemo Mättig (WMDE) has uploaded a new change for review.
https://gerrit.wikimedia.org/r/133523
Change subject: Attempt to clean-up and refactor ChangeOp and Validators code
......................................................................
Attempt to clean-up and refactor ChangeOp and Validators code
This is not really fixing a bug. The code works. But it's hard to
read and to maintain. Unfortunatelly the best I could do is to use
clone. I tried to avoid it. It gets a mess very fast (try to
reconstruct a Fingerprint object from an other Fingerprint object).
This just doesn't feel right. I think we really need to discuss this.
This is a direct follow up to some (but not all) of the comments in
Ibb6ee8439ba3ab0252403eb811aa48d1274d1d58.
Change-Id: I7434d5c40cd601976e72f104ce5699e59fef184e
---
M repo/includes/ChangeOp/ChangeOpAliases.php
M repo/includes/ChangeOp/ChangeOpDescription.php
M repo/includes/ChangeOp/ChangeOpLabel.php
M repo/includes/Validators/CompositeFingerprintValidator.php
M repo/includes/Validators/FingerprintValidator.php
M repo/includes/Validators/LabelDescriptionUniquenessValidator.php
M repo/includes/Validators/LabelUniquenessValidator.php
7 files changed, 118 insertions(+), 105 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase
refs/changes/23/133523/1
diff --git a/repo/includes/ChangeOp/ChangeOpAliases.php
b/repo/includes/ChangeOp/ChangeOpAliases.php
index 4d38a4a..e77afa1 100644
--- a/repo/includes/ChangeOp/ChangeOpAliases.php
+++ b/repo/includes/ChangeOp/ChangeOpAliases.php
@@ -25,86 +25,87 @@
*
* @var string
*/
- protected $language;
+ private $languageCode;
/**
* @since 0.4
*
* @var string[]
*/
- protected $aliases;
+ private $aliases;
/**
* @since 0.4
*
* @var array
*/
- protected $action;
+ private $action;
/**
* @since 0.5
*
* @var TermValidatorFactory
*/
- protected $termValidatorFactory;
+ private $termValidatorFactory;
/**
* @since 0.5
*
- * @param string $language
+ * @param string $languageCode
* @param string[] $aliases
* @param string $action should be set|add|remove
- *
* @param TermValidatorFactory $termValidatorFactory
*
* @throws InvalidArgumentException
*/
public function __construct(
- $language,
+ $languageCode,
array $aliases,
$action,
TermValidatorFactory $termValidatorFactory
) {
- if ( !is_string( $language ) ) {
- throw new InvalidArgumentException( '$language needs to
be a string' );
+ if ( !is_string( $languageCode ) ) {
+ throw new InvalidArgumentException( 'Language code
needs to be a string.' );
}
if ( !is_string( $action ) ) {
- throw new InvalidArgumentException( '$action needs to
be a string' );
+ throw new InvalidArgumentException( 'Action needs to be
a string.' );
}
- $this->language = $language;
+ $this->languageCode = $languageCode;
$this->aliases = $aliases;
$this->action = $action;
-
$this->termValidatorFactory = $termValidatorFactory;
}
/**
- * Applies the change to the fingerprint
- *
* @param Fingerprint $fingerprint
*
* @throws ChangeOpException
+ * @returns Fingerprint
*/
- private function updateFingerprint( Fingerprint $fingerprint ) {
- try {
- $current = $fingerprint->getAliasGroup( $this->language
)->getAliases();
- } catch ( \OutOfBoundsException $ex ) {
- $current = array();
+ private function getUpdatedFingerprint( Fingerprint $fingerprint ) {
+ if ( $fingerprint->getAliasGroups()->hasGroupForLanguage(
$this->languageCode ) ) {
+ $oldAliases = $fingerprint->getAliasGroup(
$this->languageCode )->getAliases();
+ } else {
+ $oldAliases = array();
}
- if ( $this->action === "remove" ) {
- $updated = array_diff( $current, $this->aliases );
- } elseif ( $this->action === "add" ) {
- $updated = array_merge( $current, $this->aliases );
- } elseif ( $this->action === "set" || $this->action === "" ) {
- $updated = $this->aliases;
+ $fingerprint = clone $fingerprint;
+
+ if ( $this->action === 'set' || $this->action === '' ) {
+ $newAliases = $this->aliases;
+ } elseif ( $this->action === 'add' ) {
+ $newAliases = array_merge( $oldAliases, $this->aliases
);
+ } elseif ( $this->action === 'remove' ) {
+ $newAliases = array_diff( $oldAliases, $this->aliases );
} else {
throw new ChangeOpException( 'Bad action: ' .
$this->action );
}
- $fingerprint->setAliasGroup( new AliasGroup( $this->language,
$updated ) );
+ $fingerprint->setAliasGroup( new AliasGroup(
$this->languageCode, $newAliases ) );
+
+ return $fingerprint;
}
/**
@@ -113,10 +114,11 @@
public function apply( Entity $entity, Summary $summary = null ) {
$fingerprint = $entity->getFingerprint();
- $this->updateFingerprint( $fingerprint );
- $this->updateSummary( $summary, $this->action, $this->language,
$this->aliases );
+ $this->updateSummary( $summary, $this->action,
$this->languageCode, $this->aliases );
+ $fingerprint = $this->getUpdatedFingerprint( $fingerprint );
$entity->setFingerprint( $fingerprint );
+
return true;
}
@@ -129,7 +131,6 @@
*
* @param Entity $entity
*
- * @throws ChangeOpException
* @return Result
*/
public function validate( Entity $entity ) {
@@ -137,13 +138,14 @@
$termValidator =
$this->termValidatorFactory->getLabelValidator( $entity->getType() );
// check that the language is valid
- $result = $languageValidator->validate( $this->language );
+ $result = $languageValidator->validate( $this->languageCode );
if ( !$result->isValid() ) {
return $result;
}
- if ( $this->action === 'set' || $this->action === '' ||
$this->action === 'add' ) {
+ // It should be possible to remove invalid aliases, but not to
add/set new invalid ones
+ if ( $this->action !== 'remove' ) {
// Check that the new aliases are valid
foreach ( $this->aliases as $alias ) {
$result = $termValidator->validate( $alias );
@@ -152,11 +154,10 @@
return $result;
}
}
- } elseif ( $this->action !== 'remove' ) {
- throw new ChangeOpException( 'Bad action: ' .
$this->action );
}
//XXX: Do we want to check the updated fingerprint, as we do
for labels and descriptions?
- return Result::newSuccess();
+ return $result;
}
+
}
diff --git a/repo/includes/ChangeOp/ChangeOpDescription.php
b/repo/includes/ChangeOp/ChangeOpDescription.php
index e99d13d..1e5d57f 100644
--- a/repo/includes/ChangeOp/ChangeOpDescription.php
+++ b/repo/includes/ChangeOp/ChangeOpDescription.php
@@ -7,6 +7,7 @@
use Wikibase\DataModel\Entity\Entity;
use Wikibase\DataModel\Term\Fingerprint;
use Wikibase\DataModel\Term\Term;
+use Wikibase\DataModel\Term\TermList;
use Wikibase\Summary;
use Wikibase\Validators\TermValidatorFactory;
@@ -25,58 +26,60 @@
*
* @var string
*/
- protected $language;
+ private $languageCode;
/**
* @since 0.4
*
* @var string|null
*/
- protected $description;
+ private $description;
/**
* @since 0.5
*
* @var TermValidatorFactory
*/
- protected $termValidatorFactory;
+ private $termValidatorFactory;
/**
* @since 0.4
*
- * @param string $language
+ * @param string $languageCode
* @param string|null $description
- *
* @param TermValidatorFactory $termValidatorFactory
*
* @throws InvalidArgumentException
*/
public function __construct(
- $language,
+ $languageCode,
$description,
TermValidatorFactory $termValidatorFactory
) {
- if ( !is_string( $language ) ) {
- throw new InvalidArgumentException( '$language needs to
be a string' );
+ if ( !is_string( $languageCode ) ) {
+ throw new InvalidArgumentException( 'Language code
needs to be a string.' );
}
- $this->language = $language;
+ $this->languageCode = $languageCode;
$this->description = $description;
-
$this->termValidatorFactory = $termValidatorFactory;
}
/**
- * Applies the change to the fingerprint
- *
* @param Fingerprint $fingerprint
+ *
+ * @returns Fingerprint
*/
- private function updateFingerprint( Fingerprint $fingerprint ) {
+ private function getUpdatedFingerprint( Fingerprint $fingerprint ) {
+ $fingerprint = clone $fingerprint;
+
if ( $this->description === null ) {
- $fingerprint->removeDescription( $this->language );
+ $fingerprint->removeDescription( $this->languageCode );
} else {
- $fingerprint->setDescription( new Term(
$this->language, $this->description ) );
+ $fingerprint->setDescription( new Term(
$this->languageCode, $this->description ) );
}
+
+ return $fingerprint;
}
/**
@@ -84,23 +87,19 @@
*/
public function apply( Entity $entity, Summary $summary = null ) {
$fingerprint = $entity->getFingerprint();
- $exists = $fingerprint->getDescriptions()->hasTermForLanguage(
$this->language );
- if ( $this->description === null ) {
- if ( $exists ) {
- $old = $fingerprint->getDescription(
$this->language )->getText();
- $this->updateSummary( $summary, 'remove',
$this->language, $old );
+ if ( $fingerprint->getDescriptions()->hasTermForLanguage(
$this->languageCode ) ) {
+ if ( $this->description === null ) {
+ $removedDescription =
$fingerprint->getDescription( $this->languageCode )->getText();
+ $this->updateSummary( $summary, 'remove',
$this->languageCode, $removedDescription );
+ } else {
+ $this->updateSummary( $summary, 'set',
$this->languageCode, $this->description );
}
} else {
- if ( $exists ) {
- $fingerprint->getDescription( $this->language );
- $this->updateSummary( $summary, 'set',
$this->language, $this->description );
- } else {
- $this->updateSummary( $summary, 'add',
$this->language, $this->description );
- }
+ $this->updateSummary( $summary, 'add',
$this->languageCode, $this->description );
}
- $this->updateFingerprint( $fingerprint );
+ $fingerprint = $this->getUpdatedFingerprint( $fingerprint );
$entity->setFingerprint( $fingerprint );
return true;
@@ -123,7 +122,7 @@
$fingerprintValidator =
$this->termValidatorFactory->getFingerprintValidator( $entity->getType() );
// check that the language is valid
- $result = $languageValidator->validate( $this->language );
+ $result = $languageValidator->validate( $this->languageCode );
if ( $result->isValid() && $this->description !== null ) {
// Check that the new description is valid
@@ -137,12 +136,12 @@
// Check if the new fingerprint of the entity is valid (e.g. if
the combination
// of label and description is still unique)
$fingerprint = $entity->getFingerprint();
- $this->updateFingerprint( $fingerprint );
+ $fingerprint = $this->getUpdatedFingerprint( $fingerprint );
$result = $fingerprintValidator->validateFingerprint(
$fingerprint,
$entity->getId(),
- array( $this->language )
+ array( $this->languageCode )
);
return $result;
diff --git a/repo/includes/ChangeOp/ChangeOpLabel.php
b/repo/includes/ChangeOp/ChangeOpLabel.php
index e3578cc..c2870cd 100644
--- a/repo/includes/ChangeOp/ChangeOpLabel.php
+++ b/repo/includes/ChangeOp/ChangeOpLabel.php
@@ -3,7 +3,6 @@
namespace Wikibase\ChangeOp;
use InvalidArgumentException;
-use OutOfBoundsException;
use ValueValidators\Result;
use Wikibase\DataModel\Entity\Entity;
use Wikibase\DataModel\Term\Fingerprint;
@@ -26,58 +25,60 @@
*
* @var string
*/
- protected $language;
+ private $languageCode;
/**
* @since 0.4
*
* @var string|null
*/
- protected $label;
+ private $label;
/**
* @since 0.5
*
* @var TermValidatorFactory
*/
- protected $termValidatorFactory;
+ private $termValidatorFactory;
/**
* @since 0.5
*
- * @param string $language
+ * @param string $languageCode
* @param string|null $label
- *
* @param TermValidatorFactory $termValidatorFactory
*
* @throws InvalidArgumentException
*/
public function __construct(
- $language,
+ $languageCode,
$label,
TermValidatorFactory $termValidatorFactory
) {
- if ( !is_string( $language ) ) {
- throw new InvalidArgumentException( '$language needs to
be a string' );
+ if ( !is_string( $languageCode ) ) {
+ throw new InvalidArgumentException( 'Language code
needs to be a string.' );
}
- $this->language = $language;
+ $this->languageCode = $languageCode;
$this->label = $label;
-
$this->termValidatorFactory = $termValidatorFactory;
}
/**
- * Applies the change to the fingerprint
- *
* @param Fingerprint $fingerprint
+ *
+ * @return Fingerprint
*/
- private function updateFingerprint( Fingerprint $fingerprint ) {
+ private function getUpdatedFingerprint( Fingerprint $fingerprint ) {
+ $fingerprint = clone $fingerprint;
+
if ( $this->label === null ) {
- $fingerprint->removeLabel( $this->language );
+ $fingerprint->removeLabel( $this->languageCode );
} else {
- $fingerprint->setLabel( new Term( $this->language,
$this->label ) );
+ $fingerprint->setLabel( new Term( $this->languageCode,
$this->label ) );
}
+
+ return $fingerprint;
}
/**
@@ -86,28 +87,23 @@
* @param Entity $entity
* @param Summary $summary
*
- * @throws ChangeOpException
* @return bool
*/
public function apply( Entity $entity, Summary $summary = null ) {
$fingerprint = $entity->getFingerprint();
- $exists = $fingerprint->getLabels()->hasTermForLanguage(
$this->language );
- if ( $this->label === null ) {
- if ( $exists ) {
- $old = $fingerprint->getLabel( $this->language
)->getText();
- $this->updateSummary( $summary, 'remove',
$this->language, $old );
+ if ( $fingerprint->getLabels()->hasTermForLanguage(
$this->languageCode ) ) {
+ if ( $this->label === null ) {
+ $oldLabel = $fingerprint->getLabel(
$this->languageCode )->getText();
+ $this->updateSummary( $summary, 'remove',
$this->languageCode, $oldLabel );
+ } else {
+ $this->updateSummary( $summary, 'set',
$this->languageCode, $this->label );
}
} else {
- if ( $exists ) {
- $fingerprint->getLabel( $this->language );
- $this->updateSummary( $summary, 'set',
$this->language, $this->label );
- } else {
- $this->updateSummary( $summary, 'add',
$this->language, $this->label );
- }
+ $this->updateSummary( $summary, 'add',
$this->languageCode, $this->label );
}
- $this->updateFingerprint( $fingerprint );
+ $fingerprint = $this->getUpdatedFingerprint( $fingerprint );
$entity->setFingerprint( $fingerprint );
return true;
@@ -130,7 +126,7 @@
$fingerprintValidator =
$this->termValidatorFactory->getFingerprintValidator( $entity->getType() );
// check that the language is valid
- $result = $languageValidator->validate( $this->language );
+ $result = $languageValidator->validate( $this->languageCode );
if ( $result->isValid() && $this->label !== null ) {
// Check that the new label is valid
@@ -143,12 +139,12 @@
// Check if the new fingerprint of the entity is valid (e.g. if
the label is unique)
$fingerprint = $entity->getFingerprint();
- $this->updateFingerprint( $fingerprint );
+ $fingerprint = $this->getUpdatedFingerprint( $fingerprint );
$result = $fingerprintValidator->validateFingerprint(
$fingerprint,
$entity->getId(),
- array( $this->language )
+ array( $this->languageCode )
);
return $result;
diff --git a/repo/includes/Validators/CompositeFingerprintValidator.php
b/repo/includes/Validators/CompositeFingerprintValidator.php
index 084051f..177dd1f 100644
--- a/repo/includes/Validators/CompositeFingerprintValidator.php
+++ b/repo/includes/Validators/CompositeFingerprintValidator.php
@@ -41,7 +41,11 @@
*
* @return Result
*/
- public function validateFingerprint( Fingerprint $fingerprint, EntityId
$entityId = null, $languageCodes = null ) {
+ public function validateFingerprint(
+ Fingerprint $fingerprint,
+ EntityId $entityId = null,
+ array $languageCodes = null
+ ) {
foreach ( $this->validators as $validator ) {
$result = $validator->validateFingerprint(
$fingerprint, $entityId, $languageCodes );
diff --git a/repo/includes/Validators/FingerprintValidator.php
b/repo/includes/Validators/FingerprintValidator.php
index cad460d..fd499bb 100644
--- a/repo/includes/Validators/FingerprintValidator.php
+++ b/repo/includes/Validators/FingerprintValidator.php
@@ -26,11 +26,16 @@
* @param Fingerprint $fingerprint
* @param EntityId|null $entityId Context for uniqueness checks:
conflicts with this entity
* are ignored.
- * @param string[]|null $languageCodes If given, the validation may be
limited to the given languages;
- * This is intended for optimization for the common case of only
a single language changing.
+ * @param string[]|null $languageCodes If given, the validation may be
limited to the given
+ * languages. This is intended for optimization for the common
case of only a single
+ * language changing.
*
* @return Result
*/
- public function validateFingerprint( Fingerprint $fingerprint, EntityId
$entityId = null, $languageCodes = null );
+ public function validateFingerprint(
+ Fingerprint $fingerprint,
+ EntityId $entityId = null,
+ array $languageCodes = null
+ );
-}
\ No newline at end of file
+}
diff --git a/repo/includes/Validators/LabelDescriptionUniquenessValidator.php
b/repo/includes/Validators/LabelDescriptionUniquenessValidator.php
index ec651e1..66531e4 100644
--- a/repo/includes/Validators/LabelDescriptionUniquenessValidator.php
+++ b/repo/includes/Validators/LabelDescriptionUniquenessValidator.php
@@ -51,11 +51,15 @@
*
* @param Fingerprint $fingerprint
* @param EntityId|null $entityId
- * @param array|null $languageCodes
+ * @param string[]|null $languageCodes
*
* @return Result
*/
- public function validateFingerprint( Fingerprint $fingerprint, EntityId
$entityId = null, $languageCodes = null ) {
+ public function validateFingerprint(
+ Fingerprint $fingerprint,
+ EntityId $entityId = null,
+ array $languageCodes = null
+ ) {
$labels = array_map(
function( Term $term ) {
return $term->getText();
diff --git a/repo/includes/Validators/LabelUniquenessValidator.php
b/repo/includes/Validators/LabelUniquenessValidator.php
index db6426a..dcb8b46 100644
--- a/repo/includes/Validators/LabelUniquenessValidator.php
+++ b/repo/includes/Validators/LabelUniquenessValidator.php
@@ -51,11 +51,15 @@
*
* @param Fingerprint $fingerprint
* @param EntityId|null $entityId
- * @param array|null $languageCodes
+ * @param string[]|null $languageCodes
*
* @return Result
*/
- public function validateFingerprint( Fingerprint $fingerprint, EntityId
$entityId = null, $languageCodes = null ) {
+ public function validateFingerprint(
+ Fingerprint $fingerprint,
+ EntityId $entityId = null,
+ array $languageCodes = null
+ ) {
$labels = array_map(
function( Term $term ) {
return $term->getText();
--
To view, visit https://gerrit.wikimedia.org/r/133523
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7434d5c40cd601976e72f104ce5699e59fef184e
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
