From 288fb8cafaa14e5bacda9316536f36fe4425b8a4 Mon Sep 17 00:00:00 2001
From: addshore <addshorewiki@gmail.com>
Date: Wed, 11 Oct 2017 13:40:03 +0100
Subject: [PATCH] Revision: test and fix __construct exceptions

This adds tests for each exception that can be thrown
in the Revision constructor.
It also fixes where the exception for the content part of a row
not containing a content object is thrown.
Prior to this ->getModel() could be called on the content row
element before the check had actually occoured.

Change-Id: Ia2d2cfdca01871fc6dbb96707d781db33d7d0a40
---
 includes/Revision.php                   |  8 ++---
 tests/phpunit/includes/RevisionTest.php | 41 ++++++++++++++++++++++---
 2 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/includes/Revision.php b/includes/Revision.php
index bcfbe638b4..8855441310 100644
--- a/includes/Revision.php
+++ b/includes/Revision.php
@@ -645,6 +645,10 @@ class Revision implements IDBAccessObject {
 
 			# if we have a content object, use it to set the model and type
 			if ( !empty( $row['content'] ) ) {
+				if ( !( $row['content'] instanceof Content ) ) {
+					throw new MWException( '`content` field must contain a Content object.' );
+				}
+
 				// @todo when is that set? test with external store setup! check out insertOn() [dk]
 				if ( !empty( $row['text_id'] ) ) {
 					throw new MWException( "Text already stored in external store (id {$row['text_id']}), " .
@@ -685,10 +689,6 @@ class Revision implements IDBAccessObject {
 
 			// if we have a Content object, override mText and mContentModel
 			if ( !empty( $row['content'] ) ) {
-				if ( !( $row['content'] instanceof Content ) ) {
-					throw new MWException( '`content` field must contain a Content object.' );
-				}
-
 				$handler = $this->getContentHandler();
 				$this->mContent = $row['content'];
 
diff --git a/tests/phpunit/includes/RevisionTest.php b/tests/phpunit/includes/RevisionTest.php
index 80a690fbe3..39f7e5c304 100644
--- a/tests/phpunit/includes/RevisionTest.php
+++ b/tests/phpunit/includes/RevisionTest.php
@@ -66,11 +66,7 @@ class RevisionTest extends MediaWikiTestCase {
 		];
 		yield 'with content' => [
 			[
-				'content' => ContentHandler::makeContent(
-					'hello world.',
-					Title::newFromText( 'RevisionTest_testConstructWithContent' ),
-					CONTENT_MODEL_JAVASCRIPT
-				),
+				'content' => new JavaScriptContent( 'hellow world.' )
 			],
 		];
 	}
@@ -85,6 +81,41 @@ class RevisionTest extends MediaWikiTestCase {
 		$this->assertEquals( CONTENT_MODEL_JAVASCRIPT, $rev->getContentModel() );
 	}
 
+	public function provideConstructThrowsExceptions() {
+		yield 'content and text_id both not empty' => [
+			[
+				'content' => new WikitextContent( 'GOAT' ),
+				'text_id' => 'someid',
+				],
+			new MWException( "Text already stored in external store (id someid), " .
+				"can't serialize content object" )
+		];
+		yield 'with bad content object (class)' => [
+			[ 'content' => new stdClass() ],
+			new MWException( '`content` field must contain a Content object.' )
+		];
+		yield 'with bad content object (string)' => [
+			[ 'content' => 'ImAGoat' ],
+			new MWException( '`content` field must contain a Content object.' )
+		];
+		yield 'bad row format' => [
+			'imastring, not a row',
+			new MWException( 'Revision constructor passed invalid row format.' )
+		];
+	}
+
+	/**
+	 * @dataProvider provideConstructThrowsExceptions
+	 */
+	public function testConstructThrowsExceptions( $rowArray, Exception $expectedException ) {
+		$this->setExpectedException(
+			get_class( $expectedException ),
+			$expectedException->getMessage(),
+			$expectedException->getCode()
+		);
+		new Revision( $rowArray );
+	}
+
 	public function provideGetRevisionText() {
 		yield 'Generic test' => [
 			'This is a goat of revision text.',
-- 
2.20.1