From ec72a9f495000b7caf425bb901b2188000681b16 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Gerg=C5=91=20Tisza?= Date: Mon, 24 Sep 2018 16:10:56 +0000 Subject: [PATCH] SlotDiffRenderer: add utility method for parameter type checks Change-Id: I0161070fd0330d4945cec2f76f4fd8128a9793b9 --- .../diff/DifferenceEngineSlotDiffRenderer.php | 10 +-- includes/diff/SlotDiffRenderer.php | 32 ++++++++ includes/diff/TextSlotDiffRenderer.php | 14 +--- .../includes/diff/SlotDiffRendererTest.php | 77 +++++++++++++++++++ .../diff/TextSlotDiffRendererTest.php | 6 +- 5 files changed, 115 insertions(+), 24 deletions(-) create mode 100644 tests/phpunit/includes/diff/SlotDiffRendererTest.php diff --git a/includes/diff/DifferenceEngineSlotDiffRenderer.php b/includes/diff/DifferenceEngineSlotDiffRenderer.php index 0c632b97ec..c389a6fd2a 100644 --- a/includes/diff/DifferenceEngineSlotDiffRenderer.php +++ b/includes/diff/DifferenceEngineSlotDiffRenderer.php @@ -46,15 +46,7 @@ class DifferenceEngineSlotDiffRenderer extends SlotDiffRenderer { /** @inheritDoc */ public function getDiff( Content $oldContent = null, Content $newContent = null ) { - if ( !$oldContent && !$newContent ) { - throw new InvalidArgumentException( '$oldContent and $newContent cannot both be null' ); - } - if ( !$oldContent || !$newContent ) { - $someContent = $newContent ?: $oldContent; - $emptyContent = $someContent->getContentHandler()->makeEmptyContent(); - $oldContent = $oldContent ?: $emptyContent; - $newContent = $newContent ?: $emptyContent; - } + $this->normalizeContents( $oldContent, $newContent ); return $this->differenceEngine->generateContentDiffBody( $oldContent, $newContent ); } diff --git a/includes/diff/SlotDiffRenderer.php b/includes/diff/SlotDiffRenderer.php index f20b4169db..b30607f755 100644 --- a/includes/diff/SlotDiffRenderer.php +++ b/includes/diff/SlotDiffRenderer.php @@ -20,6 +20,7 @@ * @file * @ingroup DifferenceEngine */ +use Wikimedia\Assert\Assert; /** * Renders a diff for a single slot (that is, a diff between two content objects). @@ -62,4 +63,35 @@ abstract class SlotDiffRenderer { return []; } + /** + * Helper method to normalize the input of getDiff(). + * Verifies that at least one of $oldContent and $newContent is not null, verifies that + * they are instances of one of the allowed classes (if provided), and replaces null with + * empty content. + * @param Content|null &$oldContent + * @param Content|null &$newContent + * @param string|array|null $allowedClasses + */ + protected function normalizeContents( + Content &$oldContent = null, Content &$newContent = null, $allowedClasses = null + ) { + if ( !$oldContent && !$newContent ) { + throw new InvalidArgumentException( '$oldContent and $newContent cannot both be null' ); + } + + if ( $allowedClasses ) { + if ( is_array( $allowedClasses ) ) { + $allowedClasses = implode( '|', $allowedClasses ); + } + Assert::parameterType( $allowedClasses . '|null', $oldContent, '$oldContent' ); + Assert::parameterType( $allowedClasses . '|null', $newContent, '$newContent' ); + } + + if ( !$oldContent ) { + $oldContent = $newContent->getContentHandler()->makeEmptyContent(); + } elseif ( !$newContent ) { + $newContent = $oldContent->getContentHandler()->makeEmptyContent(); + } + } + } diff --git a/includes/diff/TextSlotDiffRenderer.php b/includes/diff/TextSlotDiffRenderer.php index 9c60705587..1cc6646297 100644 --- a/includes/diff/TextSlotDiffRenderer.php +++ b/includes/diff/TextSlotDiffRenderer.php @@ -116,19 +116,7 @@ class TextSlotDiffRenderer extends SlotDiffRenderer { /** @inheritDoc */ public function getDiff( Content $oldContent = null, Content $newContent = null ) { - if ( !$oldContent && !$newContent ) { - throw new InvalidArgumentException( '$oldContent and $newContent cannot both be null' ); - } elseif ( $oldContent && !( $oldContent instanceof TextContent ) ) { - throw new InvalidArgumentException( __CLASS__ . ' does not handle ' . get_class( $oldContent ) ); - } elseif ( $newContent && !( $newContent instanceof TextContent ) ) { - throw new InvalidArgumentException( __CLASS__ . ' does not handle ' . get_class( $newContent ) ); - } - - if ( !$oldContent ) { - $oldContent = $newContent->getContentHandler()->makeEmptyContent(); - } elseif ( !$newContent ) { - $newContent = $oldContent->getContentHandler()->makeEmptyContent(); - } + $this->normalizeContents( $oldContent, $newContent, TextContent::class ); $oldText = $oldContent->serialize(); $newText = $newContent->serialize(); diff --git a/tests/phpunit/includes/diff/SlotDiffRendererTest.php b/tests/phpunit/includes/diff/SlotDiffRendererTest.php new file mode 100644 index 0000000000..58b433f129 --- /dev/null +++ b/tests/phpunit/includes/diff/SlotDiffRendererTest.php @@ -0,0 +1,77 @@ +getMockBuilder( SlotDiffRenderer::class ) + ->getMock(); + try { + // __call needs help deciding which parameter to take by reference + call_user_func_array( [ TestingAccessWrapper::newFromObject( $slotDiffRenderer ), + 'normalizeContents' ], [ &$oldContent, &$newContent, $allowedClasses ] ); + $this->assertEquals( $expectedOldContent, $oldContent ); + $this->assertEquals( $expectedNewContent, $newContent ); + } catch ( Exception $e ) { + if ( !$expectedExceptionClass ) { + throw $e; + } + $this->assertInstanceOf( $expectedExceptionClass, $e ); + } + } + + public function provideNormalizeContents() { + return [ + 'both null' => [ null, null, null, null, null, InvalidArgumentException::class ], + 'left null' => [ + null, new WikitextContent( 'abc' ), null, + new WikitextContent( '' ), new WikitextContent( 'abc' ), null, + ], + 'right null' => [ + new WikitextContent( 'def' ), null, null, + new WikitextContent( 'def' ), new WikitextContent( '' ), null, + ], + 'type filter' => [ + new WikitextContent( 'abc' ), new WikitextContent( 'def' ), WikitextContent::class, + new WikitextContent( 'abc' ), new WikitextContent( 'def' ), null, + ], + 'type filter (subclass)' => [ + new WikitextContent( 'abc' ), new WikitextContent( 'def' ), TextContent::class, + new WikitextContent( 'abc' ), new WikitextContent( 'def' ), null, + ], + 'type filter (null)' => [ + new WikitextContent( 'abc' ), null, TextContent::class, + new WikitextContent( 'abc' ), new WikitextContent( '' ), null, + ], + 'type filter failure (left)' => [ + new TextContent( 'abc' ), new WikitextContent( 'def' ), WikitextContent::class, + null, null, ParameterTypeException::class, + ], + 'type filter failure (right)' => [ + new WikitextContent( 'abc' ), new TextContent( 'def' ), WikitextContent::class, + null, null, ParameterTypeException::class, + ], + 'type filter (array syntax)' => [ + new WikitextContent( 'abc' ), new JsonContent( 'def' ), + [ JsonContent::class, WikitextContent::class ], + new WikitextContent( 'abc' ), new JsonContent( 'def' ), null, + ], + 'type filter failure (array syntax)' => [ + new WikitextContent( 'abc' ), new CssContent( 'def' ), + [ JsonContent::class, WikitextContent::class ], + null, null, ParameterTypeException::class, + ], + ]; + } + +} diff --git a/tests/phpunit/includes/diff/TextSlotDiffRendererTest.php b/tests/phpunit/includes/diff/TextSlotDiffRendererTest.php index ec45e29d67..e08efacd26 100644 --- a/tests/phpunit/includes/diff/TextSlotDiffRendererTest.php +++ b/tests/phpunit/includes/diff/TextSlotDiffRendererTest.php @@ -1,5 +1,7 @@ [ $this->makeContent( '', 'testing-nontext' ), $this->makeContent( "aaa\nbbb\nccc" ), - new InvalidArgumentException( 'TextSlotDiffRenderer does not handle DummyNonTextContent' ), + new ParameterTypeException( '$oldContent', 'TextContent|null' ), ], 'non-text right content' => [ $this->makeContent( "aaa\nbbb\nccc" ), $this->makeContent( '', 'testing-nontext' ), - new InvalidArgumentException( 'TextSlotDiffRenderer does not handle DummyNonTextContent' ), + new ParameterTypeException( '$newContent', 'TextContent|null' ), ], ]; } -- 2.20.1