Krinkle has uploaded a new change for review.
https://gerrit.wikimedia.org/r/177171
Change subject: content: Refactor and fix various bugs in JsonContent
......................................................................
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.
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
* Fix incorrect documentation for beautifyJSON's return value.
Bug: T76553
Change-Id: Ifed379ba4674a8289b554a95953951886bf2cbfd
---
M includes/content/JsonContent.php
M tests/phpunit/includes/content/JsonContentTest.php
2 files changed, 67 insertions(+), 22 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core
refs/changes/71/177171/1
diff --git a/includes/content/JsonContent.php b/includes/content/JsonContent.php
index b36827c..738dd12 100644
--- a/includes/content/JsonContent.php
+++ b/includes/content/JsonContent.php
@@ -2,6 +2,8 @@
/**
* JSON Content Model
*
+ * This class considers primitives and arrays invalid.
+ *
* @file
*
* @author Ori Livneh <[email protected]>
@@ -13,6 +15,7 @@
* @since 1.24
*/
class JsonContent extends TextContent {
+ protected $parsedJson;
public function __construct( $text, $modelId = CONTENT_MODEL_JSON ) {
parent::__construct( $text, $modelId );
@@ -20,35 +23,51 @@
/**
* Decodes the JSON into a PHP associative array.
- * @return array
+ * @return array|null
*/
- public function getJsonData() {
- return FormatJson::decode( $this->getNativeData(), true );
+ protected function getJsonData() {
+ if ( $this->parsedJson === null ) {
+ // Use parse() and its Status instead of decode() so
that we can
+ // distinguish its null return value from uninitialised.
+ $this->parsedJson = FormatJson::parse(
$this->getNativeData(), FormatJson::FORCE_ASSOC );
+ }
+ return $this->parsedJson->getValue();
}
/**
* @return bool Whether content is valid JSON.
*/
public function isValid() {
- return $this->getJsonData() !== null;
+ return is_array( $this->getJsonData() );
}
/**
- * Pretty-print JSON
+ * Pretty-print JSON.
*
- * @return bool|null|string
+ * If called before validation, it may return false to invidate the
+ * content is invalid.
+ *
+ * @return null|string
*/
public function beautifyJSON() {
- $decoded = FormatJson::decode( $this->getNativeData(), true );
- if ( !is_array( $decoded ) ) {
+ if ( !$this->isValid() ) {
return null;
}
- return FormatJson::encode( $decoded, true );
-
+ if ( !count( $this->getJsonData() ) ) {
+ // Don't transform {} to array. These are supposed to
be JSON objects, we don't
+ // even support arrays.
+ return FormatJson::encode( new stdClass(), true );
+ }
+ return FormatJson::encode( $this->getJsonData(), true );
}
/**
* Beautifies JSON prior to save.
+ *
+ * WikiPage::doEditContent invokes chain
WikiPage::prepareContentForEdit ->
+ * Content::preSaveTransform, before validation. As such, the native
data here
+ * may be invalid.
+ *
* @param Title $title Title
* @param User $user User
* @param ParserOptions $popts
@@ -61,6 +80,9 @@
/**
* Set the HTML and add the appropriate styles
*
+ * WikiPage::doEditContent invokes chain
WikiPage::prepareContentForEdit ->
+ * Content::getParserOutput -> Content::fillParserOutput, before
validation.
+ * As such, the native data here may be invalid.
*
* @param Title $title
* @param int $revId
@@ -71,7 +93,7 @@
protected function fillParserOutput( Title $title, $revId,
ParserOptions $options, $generateHtml, ParserOutput &$output
) {
- if ( $generateHtml ) {
+ if ( $this->isValid() && $generateHtml ) {
$output->setText( $this->objectTable(
$this->getJsonData() ) );
$output->addModuleStyles( 'mediawiki.content.json' );
} else {
diff --git a/tests/phpunit/includes/content/JsonContentTest.php
b/tests/phpunit/includes/content/JsonContentTest.php
index d4151a5..766b47b 100644
--- a/tests/phpunit/includes/content/JsonContentTest.php
+++ b/tests/phpunit/includes/content/JsonContentTest.php
@@ -25,29 +25,52 @@
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(
+ '[]',
+ '[]',
+ ),
+ array(
+ '{}',
+ '{}',
+ ),
+ array(
+ '["foo"]',
+ "[\n \"foo\"\n]",
+ ),
+ array(
+ '["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() {
--
To view, visit https://gerrit.wikimedia.org/r/177171
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifed379ba4674a8289b554a95953951886bf2cbfd
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Krinkle <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
