From e4f84af980c0e1054b7cf101c00b8c858dd6c562 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Wed, 3 Dec 2014 02:10:50 +0000 Subject: [PATCH] content: Refactor and fix various bugs in JsonContent Follows-up d2a82fcb60. These issues weren't previously exposed as nothing uses JsonContent by default in core, and the extensions using it (e.g. EventLogging) lock it down very early. As are most of these methods were never really put to use (they were called after the extension does its superset of checking, or too early and WikiPage ignores it). Bug fixes * Empty JSON object was converted to an array by PST conversion. The beautifyJSON method is intended for prettify purposes but actually modified the content stored in the database and made it no longer roundtrip ({} != []). We can't change getJsonData to return an object since it's a public method and people use it as an array. So we can't cast it to a PHP object as that would break back-compat. Turns out the class doesn't even support non-objects anyway (a primitive in JSON can trivially cause a fatal as it wasn't consistently considered invalid, though it didn't actually fatal due to some lucky spaghetti code in WikiPage). * Fix beautifyJSON by checking for empty objects to prevent implicit {} to [] conversion. * Add isValid() check to fillParserOutput() as it's called early on. Otherwise it throws a warning that 'foreach' (in objectTable) iterates over null. In practice it doesn't matter since the entire parser output is rejected when WikiPage eventually checks isValid (through Content::prepareSave). * Consider all non- (PHP) array values invalid instead of just non-null values. Enhancements * Display message "Empty object" instead of a completely blank page for an empty object. * Display message "Empty object" or "Empty array" instead of an empty table cell. * Render arrays as a list of values (without indices). * Remove italics from table cells for values. The monospace font should be enough. It also offsets it from the "Empty" placeholders (which are italicised). Refactoring and clean up * Use FormatJson::parse so that we can use Status to distinguish between null parse result and thus reliably cache it. Ideally we wouldn't need to cache it, but right now this code is pulled apart and called in so many strange ways that we end up calling this several times. * Improve fairly meaningless test (testBeautifyJson) that was calling FormatJson in its data provider, exactly what the method being tested did. It also provided the test with data that could never end up in normal usage (a PHP-style associated array with implied numerical indices). * Document that this class rejects non-array values. * Document the problem with WikiPage assumming PST can run on any content. WikiPage fundamentally still assumes wikitext, in that there's no concept of invalid content. * Fix incorrect documentation for getJsonData's return value (It may return null.) * Fix incorrect documentation for beautifyJSON's return value. (It never returned boolean.) Bug: T76553 Change-Id: Ifed379ba4674a8289b554a95953951886bf2cbfd --- includes/content/JsonContent.php | 155 ++++++++++++++---- languages/i18n/en.json | 3 + languages/i18n/qqq.json | 3 + .../src/mediawiki/mediawiki.content.json.css | 11 +- .../includes/content/JsonContentTest.php | 76 ++++++--- 5 files changed, 187 insertions(+), 61 deletions(-) diff --git a/includes/content/JsonContent.php b/includes/content/JsonContent.php index 1ce25c2dba..ff3b25b8b2 100644 --- a/includes/content/JsonContent.php +++ b/includes/content/JsonContent.php @@ -2,6 +2,8 @@ /** * JSON Content Model * + * This class requires the root structure to be an object (not primitives or arrays). + * * @file * * @author Ori Livneh @@ -14,53 +16,80 @@ */ class JsonContent extends TextContent { - public function __construct( $text, $modelId = CONTENT_MODEL_JSON ) { - parent::__construct( $text, $modelId ); + /** + * @since 1.25 + * @var Status + */ + protected $jsonParse; + + /** + * @param string $text JSON + */ + public function __construct( $text ) { + parent::__construct( $text, CONTENT_MODEL_JSON ); } /** * Decodes the JSON into a PHP associative array. - * @return array + * + * @deprecated since 1.25 Use getData instead. + * @return array|null */ public function getJsonData() { + wfDeprecated( __METHOD__, '1.25' ); return FormatJson::decode( $this->getNativeData(), true ); } /** - * @return bool Whether content is valid JSON. + * Decodes the JSON string into a PHP object. + * + * @return Status + */ + public function getData() { + if ( $this->jsonParse === null ) { + $this->jsonParse = FormatJson::parse( $this->getNativeData() ); + } + return $this->jsonParse; + } + + /** + * @return bool Whether content is valid. */ public function isValid() { - return $this->getJsonData() !== null; + return $this->getData()->isGood() && is_object( $this->getData()->getValue() ); } /** - * Pretty-print JSON + * Pretty-print JSON. + * + * If called before validation, it may return JSON "null". * - * @return bool|null|string + * @return string */ public function beautifyJSON() { - $decoded = $this->getJsonData(); - if ( !is_array( $decoded ) ) { - return null; - } - return FormatJson::encode( $decoded, true ); - + return FormatJson::encode( $this->getData()->getValue(), true ); } /** * Beautifies JSON prior to save. + * * @param Title $title Title * @param User $user User * @param ParserOptions $popts * @return JsonContent */ public function preSaveTransform( Title $title, User $user, ParserOptions $popts ) { + // FIXME: WikiPage::doEditContent invokes PST before validation. As such, native data + // may be invalid (though PST result is discarded later in that case). + if ( !$this->isValid() ) { + return $this; + } + return new static( $this->beautifyJSON() ); } /** - * Set the HTML and add the appropriate styles - * + * Set the HTML and add the appropriate styles. * * @param Title $title * @param int $revId @@ -71,50 +100,112 @@ class JsonContent extends TextContent { protected function fillParserOutput( Title $title, $revId, ParserOptions $options, $generateHtml, ParserOutput &$output ) { - if ( $generateHtml ) { - $output->setText( $this->objectTable( $this->getJsonData() ) ); + // FIXME: WikiPage::doEditContent generates parser output before validation. + // As such, native data may be invalid (though output is discarded later in that case). + if ( $generateHtml && $this->isValid() ) { + $output->setText( $this->objectTable( $this->getData()->getValue() ) ); $output->addModuleStyles( 'mediawiki.content.json' ); } else { $output->setText( '' ); } } + /** - * Constructs an HTML representation of a JSON object. - * @param array $mapping + * Construct an HTML representation of a JSON object. + * + * Called recursively via valueCell(). + * + * @param stdClass $mapping * @return string HTML */ protected function objectTable( $mapping ) { $rows = array(); + $empty = true; foreach ( $mapping as $key => $val ) { $rows[] = $this->objectRow( $key, $val ); + $empty = false; } - return Xml::tags( 'table', array( 'class' => 'mw-json' ), - Xml::tags( 'tbody', array(), join( "\n", $rows ) ) + if ( $empty ) { + $rows[] = Html::rawElement( 'tr', array(), + Html::element( 'td', array( 'class' => 'mw-json-empty' ), + wfMessage( 'content-json-empty-object' )->text() + ) + ); + } + return Html::rawElement( 'table', array( 'class' => 'mw-json' ), + Html::rawElement( 'tbody', array(), join( "\n", $rows ) ) ); } /** - * Constructs HTML representation of a single key-value pair. + * Construct HTML representation of a single key-value pair. * @param string $key * @param mixed $val * @return string HTML. */ protected function objectRow( $key, $val ) { $th = Xml::elementClean( 'th', array(), $key ); - if ( is_array( $val ) ) { - $td = Xml::tags( 'td', array(), self::objectTable( $val ) ); - } else { - if ( is_string( $val ) ) { - $val = '"' . $val . '"'; - } else { - $val = FormatJson::encode( $val ); - } + $td = self::valueCell( $val ); + return Html::rawElement( 'tr', array(), $th . $td ); + } + + /** + * Constructs an HTML representation of a JSON array. + * + * Called recursively via valueCell(). + * + * @param array $mapping + * @return string HTML + */ + protected function arrayTable( $mapping ) { + $rows = array(); + $empty = true; - $td = Xml::elementClean( 'td', array( 'class' => 'value' ), $val ); + foreach ( $mapping as $val ) { + $rows[] = $this->arrayRow( $val ); + $empty = false; } + if ( $empty ) { + $rows[] = Html::rawElement( 'tr', array(), + Html::element( 'td', array( 'class' => 'mw-json-empty' ), + wfMessage( 'content-json-empty-array' )->text() + ) + ); + } + return Html::rawElement( 'table', array( 'class' => 'mw-json' ), + Html::rawElement( 'tbody', array(), join( "\n", $rows ) ) + ); + } - return Xml::tags( 'tr', array(), $th . $td ); + /** + * Construct HTML representation of a single array value. + * @param mixed $val + * @return string HTML. + */ + protected function arrayRow( $val ) { + $td = self::valueCell( $val ); + return Html::rawElement( 'tr', array(), $td ); } + /** + * Construct HTML representation of a single value. + * @param mixed $val + * @return string HTML. + */ + protected function valueCell( $val ) { + if ( is_object( $val ) ) { + return Html::rawElement( 'td', array(), self::objectTable( $val ) ); + } + if ( is_array( $val ) ) { + return Html::rawElement( 'td', array(), self::arrayTable( $val ) ); + } + if ( is_string( $val ) ) { + $val = '"' . $val . '"'; + } else { + $val = FormatJson::encode( $val ); + } + + return Xml::elementClean( 'td', array( 'class' => 'value' ), $val ); + } } diff --git a/languages/i18n/en.json b/languages/i18n/en.json index 2ee3df2586..da49fe90a9 100644 --- a/languages/i18n/en.json +++ b/languages/i18n/en.json @@ -698,6 +698,9 @@ "content-model-text": "plain text", "content-model-javascript": "JavaScript", "content-model-css": "CSS", + "content-model-json": "JSON", + "content-json-empty-object": "Empty object", + "content-json-empty-array": "Empty array", "duplicate-args-category": "Pages using duplicate arguments in template calls", "duplicate-args-category-desc": "The page contains template calls that use duplicates of arguments, such as {{foo|bar=1|bar=2}} or {{foo|bar|1=baz}}.", "expensive-parserfunction-warning": "Warning: This page contains too many expensive parser function calls.\n\nIt should have less than $2 {{PLURAL:$2|call|calls}}, there {{PLURAL:$1|is now $1 call|are now $1 calls}}.", diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json index 813064d799..42ae44c97b 100644 --- a/languages/i18n/qqq.json +++ b/languages/i18n/qqq.json @@ -862,6 +862,9 @@ "content-model-text": "Name for the plain text content model, used when decribing what type of content a page contains.\n\nThis message is substituted in:\n*{{msg-mw|Bad-target-model}}\n*{{msg-mw|Content-not-allowed-here}}\n{{Identical|Plain text}}", "content-model-javascript": "Name for the JavaScript content model, used when decribing what type of content a page contains.\n\nThis message is substituted in:\n*{{msg-mw|Bad-target-model}}\n*{{msg-mw|Content-not-allowed-here}}", "content-model-css": "Name for the CSS content model, used when decribing what type of content a page contains.\n\nThis message is substituted in:\n*{{msg-mw|Bad-target-model}}\n*{{msg-mw|Content-not-allowed-here}}", + "content-model-json": "Name for the JSON content model, used when decribing what type of content a page contains.\n\nThis message is substituted in:\n*{{msg-mw|Bad-target-model}}\n*{{msg-mw|Content-not-allowed-here}}", + "content-json-empty-object": "Used to represent an object with no properties on a JSON content model page.", + "content-json-empty-array": "Used to represent an array with no values on a JSON content model page.", "duplicate-args-category": "This message is used as a category name for a [[mw:Special:MyLanguage/Help:Tracking categories|tracking category]] where pages are placed automatically if they contain template calls that use duplicates of arguments, such as {{foo|bar=1|bar=2}} or {{foo|bar|1=baz}}.", "duplicate-args-category-desc": "Duplicate arguments category description. Shown on [[Special:TrackingCategories]].\n\nSee also:\n* {{msg-mw|Duplicate-args-category}}", "expensive-parserfunction-warning": "On some (expensive) [[MetaWikipedia:Help:ParserFunctions|parser functions]] (e.g. {{#ifexist:}}) there is a limit of how many times it may be used. This is an error message shown when the limit is exceeded.\n\nParameters:\n* $1 - the current number of parser function calls\n* $2 - the allowed number of parser function calls\nSee also [[:mw:Manual:$wgExpensiveParserFunctionLimit|$wgExpensiveParserFunctionLimit in the MediaWiki manual]].\n\nSee also:\n* {{msg-mw|Expensive-parserfunction-category}}", diff --git a/resources/src/mediawiki/mediawiki.content.json.css b/resources/src/mediawiki/mediawiki.content.json.css index d93e291ecc..4afccda3b5 100644 --- a/resources/src/mediawiki/mediawiki.content.json.css +++ b/resources/src/mediawiki/mediawiki.content.json.css @@ -18,19 +18,20 @@ padding: 0.5em 1em; } -.mw-json td { - background-color: #eee; - font-style: italic; -} - .mw-json .value { background-color: #dcfae3; font-family: monospace, monospace; white-space: pre-wrap; } +.mw-json-empty { + background-color: #fff; + font-style: italic; +} + .mw-json tr { margin-bottom: 0.5em; + background-color: #eee; } .mw-json th { diff --git a/tests/phpunit/includes/content/JsonContentTest.php b/tests/phpunit/includes/content/JsonContentTest.php index d4151a5c24..0ad8ecc803 100644 --- a/tests/phpunit/includes/content/JsonContentTest.php +++ b/tests/phpunit/includes/content/JsonContentTest.php @@ -6,48 +6,76 @@ */ class JsonContentTest extends MediaWikiLangTestCase { + protected function setUp() { + parent::setUp(); + + $this->setMwGlobals( 'wgWellFormedXml', true ); + } + public static function provideValidConstruction() { return array( - array( 'foo', CONTENT_MODEL_JSON, false, null ), - array( FormatJson::encode( array() ), CONTENT_MODEL_JSON, true, array() ), - array( FormatJson::encode( array( 'foo' ) ), CONTENT_MODEL_JSON, true, array( 'foo' ) ), + array( 'foo', false, null ), + array( '{}', true, (object)array() ), + array( '{ "0": "bar" }', true, (object)array( 'bar' ) ), ); } /** * @dataProvider provideValidConstruction */ - public function testValidConstruct( $text, $modelId, $isValid, $expected ) { - $obj = new JsonContent( $text, $modelId ); + public function testIsValid( $text, $isValid, $expected ) { + $obj = new JsonContent( $text, CONTENT_MODEL_JSON ); $this->assertEquals( $isValid, $obj->isValid() ); - $this->assertEquals( $expected, $obj->getJsonData() ); + $this->assertEquals( $expected, $obj->getData()->getValue() ); } public static function provideDataToEncode() { return array( - array( array() ), - array( array( 'foo' ) ), - array( array( 'foo', 'bar' ) ), - array( array( 'baz' => 'foo', 'bar' ) ), - array( array( 'baz' => 1000, 'bar' ) ), + array( + // Round-trip empty array + '[]', + '[]', + ), + array( + // Round-trip empty object + '{}', + '{}', + ), + array( + // Round-trip empty array/object (nested) + '{ "foo": {}, "bar": [] }', + "{\n \"foo\": {},\n \"bar\": []\n}", + ), + array( + '{ "foo": "bar" }', + "{\n \"foo\": \"bar\"\n}", + ), + array( + '{ "foo": 1000 }', + "{\n \"foo\": 1000\n}", + ), + array( + '{ "foo": 1000, "0": "bar" }', + "{\n \"foo\": 1000,\n \"0\": \"bar\"\n}", + ), ); } /** * @dataProvider provideDataToEncode */ - public function testBeautifyUsesFormatJson( $data ) { - $obj = new JsonContent( FormatJson::encode( $data ) ); - $this->assertEquals( FormatJson::encode( $data, true ), $obj->beautifyJSON() ); + public function testBeautifyJson( $input, $beautified ) { + $obj = new JsonContent( $input ); + $this->assertEquals( $beautified, $obj->beautifyJSON() ); } /** * @dataProvider provideDataToEncode */ - public function testPreSaveTransform( $data ) { - $obj = new JsonContent( FormatJson::encode( $data ) ); + public function testPreSaveTransform( $input, $transformed ) { + $obj = new JsonContent( $input ); $newObj = $obj->preSaveTransform( $this->getMockTitle(), $this->getMockUser(), $this->getMockParserOptions() ); - $this->assertTrue( $newObj->equals( new JsonContent( FormatJson::encode( $data, true ) ) ) ); + $this->assertTrue( $newObj->equals( new JsonContent( $transformed ) ) ); } private function getMockTitle() { @@ -70,33 +98,33 @@ class JsonContentTest extends MediaWikiLangTestCase { public static function provideDataAndParserText() { return array( array( - array(), - '
' + (object)array(), + '
Empty object
' ), array( - array( 'foo' ), + (object)array( 'foo' ), '
0"foo"
' ), array( - array( 'foo', 'bar' ), + (object)array( 'foo', 'bar' ), '' . "\n" . '
0"foo"
1"bar"
' ), array( - array( 'baz' => 'foo', 'bar' ), + (object)array( 'baz' => 'foo', 'bar' ), '' . "\n" . '
baz"foo"
0"bar"
' ), array( - array( 'baz' => 1000, 'bar' ), + (object)array( 'baz' => 1000, 'bar' ), '' . "\n" . '
baz1000
0"bar"
' ), array( - array( ''), + (object)array( ''), '
0"<script>alert("evil!")</script>"
', ), ); -- 2.20.1