Addshore has submitted this change and it was merged.
Change subject: Handle formatting errors gracefully
......................................................................
Handle formatting errors gracefully
Change-Id: I278acd0a4156ab78fa40341126cb05d94a664cc7
---
M lib/includes/ClaimDifferenceVisualizer.php
M repo/includes/SummaryFormatter.php
M repo/tests/phpunit/includes/SummaryFormatterTest.php
3 files changed, 87 insertions(+), 34 deletions(-)
Approvals:
Addshore: Looks good to me, approved
jenkins-bot: Verified
diff --git a/lib/includes/ClaimDifferenceVisualizer.php
b/lib/includes/ClaimDifferenceVisualizer.php
index 9b3d787..f37a0da 100644
--- a/lib/includes/ClaimDifferenceVisualizer.php
+++ b/lib/includes/ClaimDifferenceVisualizer.php
@@ -8,6 +8,7 @@
use Html;
use Diff\Diff;
use RuntimeException;
+use ValueParsers\FormattingException;
use Wikibase\Lib\EntityIdLabelFormatter;
use Wikibase\Lib\SnakFormatter;
@@ -30,7 +31,7 @@
*
* @var EntityIdLabelFormatter
*/
- private $propertyFormatter;
+ private $propertyIdFormatter;
/**
* @since 0.5
@@ -44,19 +45,19 @@
*
* @since 0.4
*
- * @param EntityIdLabelFormatter $propertyFormatter
+ * @param EntityIdLabelFormatter $propertyIdFormatter
* @param SnakFormatter $snakFormatter
*
* @throws \InvalidArgumentException
*/
- public function __construct( EntityIdLabelFormatter $propertyFormatter,
SnakFormatter $snakFormatter ) {
+ public function __construct( EntityIdLabelFormatter
$propertyIdFormatter, SnakFormatter $snakFormatter ) {
if ( $snakFormatter->getFormat() !==
SnakFormatter::FORMAT_PLAIN ) {
throw new \InvalidArgumentException(
'Expected $snakFormatter to generate plain
text, not '
. $snakFormatter->getFormat() );
}
- $this->propertyFormatter = $propertyFormatter;
+ $this->propertyIdFormatter = $propertyIdFormatter;
$this->snakFormatter = $snakFormatter;
}
@@ -155,8 +156,8 @@
$valueFormatter = new DiffOpValueFormatter(
// todo: should show specific headers for both columns
$this->getSnakHeader( $mainSnakChange->getNewValue() ),
- $this->snakFormatter->formatSnak(
$mainSnakChange->getOldValue() ),
- $this->snakFormatter->formatSnak(
$mainSnakChange->getNewValue() )
+ $this->formatSnak( $mainSnakChange->getOldValue() ),
+ $this->formatSnak( $mainSnakChange->getNewValue() )
);
return $valueFormatter->generateHtml();
@@ -211,16 +212,42 @@
$newValue = null;
if ( $oldSnak instanceof Snak ) {
- $oldValue = $this->snakFormatter->formatSnak( $oldSnak
);
+ $oldValue = $this->formatSnak( $oldSnak );
}
if ( $newSnak instanceof Snak ) {
- $newValue = $this->snakFormatter->formatSnak( $newSnak
);
+ $newValue = $this->formatSnak( $newSnak );
}
$valueFormatter = new DiffOpValueFormatter( $snakHeader,
$oldValue, $newValue );
return $valueFormatter->generateHtml();
+ }
+
+ /**
+ * @param Snak $snak
+ *
+ * @return string
+ */
+ protected function formatSnak( Snak $snak ) {
+ try {
+ return $this->snakFormatter->formatSnak( $snak );
+ } catch ( FormattingException $ex ) {
+ return '?'; // XXX: or include the error message?
+ }
+ }
+
+ /**
+ * @param EntityId
+ *
+ * @return string
+ */
+ protected function formatPropertyId( EntityId $id ) {
+ try {
+ return $this->propertyIdFormatter->format( $id );
+ } catch ( FormattingException $ex ) {
+ return '?'; // XXX: or include the error message?
+ }
}
/**
@@ -239,9 +266,9 @@
// TODO: change hardcoded ": " so something like
wfMessage( 'colon-separator' ),
// but this will require further refactoring as it
would add HTML which gets escaped
$values[] =
- $this->propertyFormatter->format(
$snak->getPropertyId() ) .
+ $this->formatPropertyId( $snak->getPropertyId()
) .
': '.
- $this->snakFormatter->formatSnak( $snak );
+ $this->formatSnak( $snak );
}
return $values;
@@ -258,7 +285,7 @@
*/
protected function getSnakHeader( Snak $snak ) {
$propertyId = $snak->getPropertyId();
- $propertyLabel = $this->propertyFormatter->format( $propertyId
);
+ $propertyLabel = $this->formatPropertyId( $propertyId );
$headerText = wfMessage( 'wikibase-entity-property' ) . ' / ' .
$propertyLabel;
return $headerText;
@@ -333,23 +360,23 @@
// but this will require further refactoring as it
would add HTML which gets escaped
if ( $change instanceof DiffOpAdd ) {
$newVal =
- $this->propertyFormatter->format(
$change->getNewValue()->getPropertyId() ) .
+ $this->formatPropertyId(
$change->getNewValue()->getPropertyId() ) .
': ' .
- $this->snakFormatter->formatSnak(
$change->getNewValue() );
+ $this->formatSnak(
$change->getNewValue() );
} else if ( $change instanceof DiffOpRemove ) {
$oldVal =
- $this->propertyFormatter->format(
$change->getOldValue()->getPropertyId() ) .
+ $this->formatPropertyId(
$change->getOldValue()->getPropertyId() ) .
': ' .
- $this->snakFormatter->formatSnak(
$change->getOldValue() );
+ $this->formatSnak(
$change->getOldValue() );
} else if ( $change instanceof DiffOpChange ) {
$oldVal =
- $this->propertyFormatter->format(
$change->getOldValue()->getPropertyId() ) .
+ $this->formatPropertyId(
$change->getOldValue()->getPropertyId() ) .
': ' .
- $this->snakFormatter->formatSnak(
$change->getOldValue() );
+ $this->formatSnak(
$change->getOldValue() );
$newVal =
- $this->propertyFormatter->format(
$change->getNewValue()->getPropertyId() ) .
+ $this->formatPropertyId(
$change->getNewValue()->getPropertyId() ) .
': ' .
- $this->snakFormatter->formatSnak(
$change->getNewValue() );
+ $this->formatSnak(
$change->getNewValue() );
} else {
throw new RuntimeException( 'Diff operation of
unknown type.' );
}
diff --git a/repo/includes/SummaryFormatter.php
b/repo/includes/SummaryFormatter.php
index f6bb942..9c8815c 100644
--- a/repo/includes/SummaryFormatter.php
+++ b/repo/includes/SummaryFormatter.php
@@ -3,6 +3,8 @@
namespace Wikibase;
use DataValues\DataValue;
+use Exception;
+use InvalidArgumentException;
use Language;
use ValueFormatters\ValueFormatter;
use Wikibase\DataModel\Entity\EntityIdValue;
@@ -50,8 +52,16 @@
* @param ValueFormatter $valueFormatter
* @param SnakFormatter $snakFormatter
* @param \Language $language
+ *
+ * @throws \InvalidArgumentException
*/
public function __construct( EntityIdFormatter $idFormatter,
ValueFormatter $valueFormatter, SnakFormatter $snakFormatter, Language
$language ) {
+ if ( $snakFormatter->getFormat() !==
SnakFormatter::FORMAT_PLAIN ) {
+ throw new InvalidArgumentException(
+ 'Expected $snakFormatter to procude text/plain
output, not '
+ . $snakFormatter->getFormat() );
+ }
+
$this->idFormatter = $idFormatter;
$this->valueFormatter = $valueFormatter;
$this->snakFormatter = $snakFormatter;
@@ -152,22 +162,33 @@
* @return string
*/
protected function formatArg( $arg ) {
- if ( $arg instanceof Snak ) {
- return $this->snakFormatter->formatSnak( $arg );
- } elseif ( $arg instanceof EntityId ) {
- return $this->idFormatter->format( $arg );
- } elseif ( $arg instanceof DataValue ) {
- return $this->valueFormatter->format( $arg );
- } elseif ( method_exists( $arg, '__toString' ) ) {
- return strval( $arg );
- } elseif ( is_object( $arg ) ) {
- return '<' . get_class( $arg ) . '>';
- } elseif ( is_array( $arg ) ) {
- $strings = $this->formatArgList( $arg );
- return $this->language->commaList( $strings );
- } else {
- return strval( $arg );
+ try {
+ if ( $arg instanceof Snak ) {
+ return $this->snakFormatter->formatSnak( $arg );
+ } elseif ( $arg instanceof EntityId ) {
+ return $this->idFormatter->format( $arg );
+ } elseif ( $arg instanceof DataValue ) {
+ return $this->valueFormatter->format( $arg );
+ } elseif ( method_exists( $arg, '__toString' ) ) {
+ return strval( $arg );
+ } elseif ( is_object( $arg ) ) {
+ return '<' . get_class( $arg ) . '>';
+ } elseif ( is_array( $arg ) ) {
+ if ( !empty( $arg ) && !isset( $arg[0] ) ) {
+ // turn assoc array into a list
+ $arg = $this->formatKeyValuePairs( $arg
);
+ }
+
+ $strings = $this->formatArgList( $arg );
+ return $this->language->commaList( $strings );
+ } else {
+ return strval( $arg );
+ }
+ } catch ( Exception $ex ) {
+ wfWarn( __METHOD__ . ': failed to render value: ' .
$ex->getMessage() );
}
+
+ return '?';
}
/**
diff --git a/repo/tests/phpunit/includes/SummaryFormatterTest.php
b/repo/tests/phpunit/includes/SummaryFormatterTest.php
index 222b418..ee761fc 100644
--- a/repo/tests/phpunit/includes/SummaryFormatterTest.php
+++ b/repo/tests/phpunit/includes/SummaryFormatterTest.php
@@ -9,6 +9,7 @@
use Wikibase\DataModel\Entity\ItemId;
use Wikibase\DataModel\Entity\PropertyId;
use Wikibase\Lib\EntityIdFormatter;
+use Wikibase\Lib\SnakFormatter;
use Wikibase\PropertyValueSnak;
use Wikibase\Snak;
use Wikibase\Summary;
@@ -92,10 +93,14 @@
$valueFormatter = $this->getMock(
'ValueFormatters\ValueFormatter' );
$valueFormatter->expects( $this->any() )->method( 'format' )
->will( $this->returnCallback( array( $this,
'formatValue' ) ) );
+ $valueFormatter->expects( $this->any() )->method( 'getFormat' )
+ ->will( $this->returnValue( SnakFormatter::FORMAT_PLAIN
) );
$snakFormatter = $this->getMock( 'Wikibase\Lib\SnakFormatter' );
$snakFormatter->expects( $this->any() )->method( 'formatSnak' )
->will( $this->returnCallback( array( $this,
'formatSnak' ) ) );
+ $snakFormatter->expects( $this->any() )->method( 'getFormat' )
+ ->will( $this->returnValue( SnakFormatter::FORMAT_PLAIN
) );
$language = Language::factory( 'en' );
--
To view, visit https://gerrit.wikimedia.org/r/84978
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I278acd0a4156ab78fa40341126cb05d94a664cc7
Gerrit-PatchSet: 12
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: Daniel Kinzler <[email protected]>
Gerrit-Reviewer: Addshore <[email protected]>
Gerrit-Reviewer: Aude <[email protected]>
Gerrit-Reviewer: Tobias Gritschacher <[email protected]>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
