From 5c65525c957b74f0ca42be21328d6a50df726ef4 Mon Sep 17 00:00:00 2001 From: Thiemo Kreuz Date: Mon, 16 Dec 2019 17:11:42 +0100 Subject: [PATCH] Introduce ReferenceStack::appendText I feel this is a little better than before. It looks like we never need to *replace* a text that existed before. This depends on I4a156aa which fixes one of the last remaining trimming issues. Outside of , a with no other content but some whitespace was already forbidden. But not inside of . This is relevant for appendText(). It should not be called with null, but was because of the inconsistent behavior. Change-Id: I38c9929f2fa6e69482e45919e2f8dbf823cb1c8b --- src/Cite.php | 32 +++++++++++------------ src/ReferenceStack.php | 21 +++++++++------ tests/phpunit/unit/CiteUnitTest.php | 10 +++---- tests/phpunit/unit/ReferenceStackTest.php | 31 +++++++++++++--------- 4 files changed, 51 insertions(+), 43 deletions(-) diff --git a/src/Cite.php b/src/Cite.php index ae012cb07..1a3e073e3 100644 --- a/src/Cite.php +++ b/src/Cite.php @@ -326,23 +326,21 @@ class Cite { } else { $groupRefs = $this->referenceStack->getGroupRefs( $group ); if ( !isset( $groupRefs[$name]['text'] ) ) { - $this->referenceStack->setRefText( $group, $name, $text ); - } else { - if ( $groupRefs[$name]['text'] !== $text ) { - // two refs with same key and different content - // adds error message to the original ref - // TODO: report these errors the same way as the others, rather than a - // special case to append to the second one's content. - $this->referenceStack->setRefText( - $group, - $name, - $groupRefs[$name]['text'] . ' ' . $this->errorReporter->plain( - $parser, - 'cite_error_references_duplicate_key', - $name - ) - ); - } + $this->referenceStack->appendText( $group, $name, $text ); + } elseif ( $groupRefs[$name]['text'] !== $text ) { + // two refs with same key and different content + // adds error message to the original ref + // TODO: report these errors the same way as the others, rather than a + // special case to append to the second one's content. + $this->referenceStack->appendText( + $group, + $name, + ' ' . $this->errorReporter->plain( + $parser, + 'cite_error_references_duplicate_key', + $name + ) + ); } } return ''; diff --git a/src/ReferenceStack.php b/src/ReferenceStack.php index 21ba15369..0b868a005 100644 --- a/src/ReferenceStack.php +++ b/src/ReferenceStack.php @@ -2,6 +2,7 @@ namespace Cite; +use LogicException; use Parser; use StripState; @@ -137,7 +138,7 @@ class ReferenceStack { if ( $this->refs[$group][$follow] ?? false ) { // We know the parent note already, so just perform the "follow" and bail out // TODO: Separate `pushRef` from these side-effects. - $this->refs[$group][$follow]['text'] .= ' ' . $text; + $this->appendText( $group, $follow, ' ' . $text ); return null; } @@ -401,16 +402,20 @@ class ReferenceStack { } /** - * Interface to set reference text from external code. Ideally we can take over - * responsibility for this logic. - * @deprecated - * * @param string $group * @param string $name - * @param ?string $text + * @param string $text */ - public function setRefText( string $group, string $name, ?string $text ) { - $this->refs[$group][$name]['text'] = $text; + public function appendText( string $group, string $name, string $text ) { + if ( !isset( $this->refs[$group][$name] ) ) { + throw new LogicException( "Unknown ref \"$name\" in group \"$group\"" ); + } + + if ( isset( $this->refs[$group][$name]['text'] ) ) { + $this->refs[$group][$name]['text'] .= $text; + } else { + $this->refs[$group][$name]['text'] = $text; + } } } diff --git a/tests/phpunit/unit/CiteUnitTest.php b/tests/phpunit/unit/CiteUnitTest.php index 0090a0f9a..018c3d226 100644 --- a/tests/phpunit/unit/CiteUnitTest.php +++ b/tests/phpunit/unit/CiteUnitTest.php @@ -469,9 +469,9 @@ class CiteUnitTest extends \MediaWikiUnitTestCase { ]; } ); - $spy->referenceStack->method( 'setRefText' )->willReturnCallback( + $spy->referenceStack->method( 'appendText' )->willReturnCallback( function ( $group, $name, $text ) use ( &$pushedRefs ) { - $pushedRefs[] = [ 'setRefText', $group, $name, $text ]; + $pushedRefs[] = [ 'appendText', $group, $name, $text ]; } ); @@ -519,7 +519,7 @@ class CiteUnitTest extends \MediaWikiUnitTestCase { '', [], [ - [ 'setRefText', 'foo', 'a', 'text' ] + [ 'appendText', 'foo', 'a', 'text' ] ] ], 'Successful ref' => [ @@ -561,7 +561,7 @@ class CiteUnitTest extends \MediaWikiUnitTestCase { '', [], [ - [ 'setRefText', '', 'a', 'text' ] + [ 'appendText', '', 'a', 'text' ] ] ], 'Mismatched text in references' => [ @@ -580,7 +580,7 @@ class CiteUnitTest extends \MediaWikiUnitTestCase { '', [], [ - [ 'setRefText', '', 'a', 'text-1 (cite_error_references_duplicate_key|a)' ] + [ 'appendText', '', 'a', ' (cite_error_references_duplicate_key|a)' ] ] ], ]; diff --git a/tests/phpunit/unit/ReferenceStackTest.php b/tests/phpunit/unit/ReferenceStackTest.php index 910084a01..11595ecce 100644 --- a/tests/phpunit/unit/ReferenceStackTest.php +++ b/tests/phpunit/unit/ReferenceStackTest.php @@ -4,6 +4,7 @@ namespace Cite\Tests\Unit; use Cite\ErrorReporter; use Cite\ReferenceStack; +use LogicException; use Parser; use StripState; use Wikimedia\TestingAccessWrapper; @@ -825,12 +826,6 @@ class ReferenceStackTest extends \MediaWikiUnitTestCase { * @covers ::rollbackRefs * @covers ::rollbackRef * @dataProvider provideRollbackRefs - * - * @param array $initialCallStack - * @param array $initialRefs - * @param int $count - * @param array $expectedRedo - * @param array $expectedRefs */ public function testRollbackRefs( array $initialCallStack, @@ -1057,17 +1052,27 @@ class ReferenceStackTest extends \MediaWikiUnitTestCase { } /** - * @covers ::setRefText + * @covers ::appendText */ - public function testSetRefText() { + public function testAppendText_failure() { $stack = $this->newStack(); - $stack->setRefText( 'group', 'name', 'the-text' ); + $this->expectException( LogicException::class ); + $stack->appendText( 'group', 'name', 'the-text' ); + } - $this->assertSame( - [ 'group' => [ 'name' => [ 'text' => 'the-text' ] ] ], - $stack->refs - ); + /** + * @covers ::appendText + */ + public function testAppendText() { + $stack = $this->newStack(); + $stack->refs = [ 'group' => [ 'name' => [] ] ]; + + $stack->appendText( 'group', 'name', 'set' ); + $this->assertSame( [ 'text' => 'set' ], $stack->refs['group']['name'] ); + + $stack->appendText( 'group', 'name', ' and append' ); + $this->assertSame( [ 'text' => 'set and append' ], $stack->refs['group']['name'] ); } /**