Daniel Kinzler has uploaded a new change for review.
https://gerrit.wikimedia.org/r/107842
Change subject: Refactor SpecialCategories
......................................................................
Refactor SpecialCategories
This refactors SpecialCategories to achieve better testability.
This demonstrates how mocking can be used in tests to make
validation easier. It also shows how consistent dependency
injection reduces the number of services that need to be passed
explicitly.
Change-Id: Iaa22e9e0f34e563fd83a85d8b028c668840dda1c
---
M includes/AutoLoader.php
A includes/specialpage/ResultRowFormatter.php
M includes/specials/SpecialCategories.php
A tests/phpunit/includes/specials/CategoryPagerRowFormatterTest.php
A tests/phpunit/includes/specials/CategoryPagerTest.php
A tests/phpunit/includes/specials/SpecialCategoriesTest.php
6 files changed, 389 insertions(+), 51 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core
refs/changes/42/107842/1
diff --git a/includes/AutoLoader.php b/includes/AutoLoader.php
index ced7b1d..0322521 100644
--- a/includes/AutoLoader.php
+++ b/includes/AutoLoader.php
@@ -178,6 +178,7 @@
'ReadOnlyError' => 'includes/Exception.php',
'RedirectSpecialArticle' =>
'includes/specialpage/RedirectSpecialPage.php',
'RedirectSpecialPage' => 'includes/specialpage/RedirectSpecialPage.php',
+ 'ResultRowFormatter' => 'includes/specialpage/ResultRowFormatter.php',
'ReverseChronologicalPager' => 'includes/Pager.php',
'RevisionItem' => 'includes/RevisionList.php',
'RevisionItemBase' => 'includes/RevisionList.php',
@@ -915,6 +916,7 @@
'BlockListPager' => 'includes/specials/SpecialBlockList.php',
'BrokenRedirectsPage' => 'includes/specials/SpecialBrokenRedirects.php',
'CategoryPager' => 'includes/specials/SpecialCategories.php',
+ 'CategoryPagerRowFormatter' =>
'includes/specials/SpecialCategories.php',
'ContribsPager' => 'includes/specials/SpecialContributions.php',
'DeadendPagesPage' => 'includes/specials/SpecialDeadendpages.php',
'DeletedContribsPager' =>
'includes/specials/SpecialDeletedContributions.php',
diff --git a/includes/specialpage/ResultRowFormatter.php
b/includes/specialpage/ResultRowFormatter.php
new file mode 100644
index 0000000..9d9abee
--- /dev/null
+++ b/includes/specialpage/ResultRowFormatter.php
@@ -0,0 +1,38 @@
+<?php
+/**
+ * A service for formatting rows from a database query result.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ * http://www.gnu.org/copyleft/gpl.html
+ *
+ * @file
+ * @license GPL 2+
+ * @author Daniel Kinzler
+ */
+
+/**
+ * A service for formatting rows from a database query result as HTML.
+ */
+interface ResultRowFormatter {
+
+ /**
+ * @param object $row the result row to format, as an stdclass object
+ *
+ * @return string HTML
+ */
+ function formatRow( $row );
+
+}
+
\ No newline at end of file
diff --git a/includes/specials/SpecialCategories.php
b/includes/specials/SpecialCategories.php
index 8dd93fd..bc1523d 100644
--- a/includes/specials/SpecialCategories.php
+++ b/includes/specials/SpecialCategories.php
@@ -27,14 +27,9 @@
class SpecialCategories extends SpecialPage {
/**
- * @var TitleFormatter
+ * @var callable
*/
- protected $titleFormatter = null;
-
- /**
- * @var PageLinkRenderer
- */
- protected $linkRenderer = null;
+ protected $pagerBuilder = null;
function __construct() {
parent::__construct( 'Categories' );
@@ -45,48 +40,66 @@
}
/**
- * Initialize or override the services SpecialCategories collaborates
with.
- * Useful mainly for testing.
+ * Initialize or override the pagerBuilder
SpecialCategories::newCategoryPager() will
+ * use to create a pager that will execute the database query and
generate the HTML output.
*
- * @todo: the pager should also be injected, and de-coupled from the
rendering logic.
+ * @param callable $pagerBuilder A callable that accepts one parameter,
+ * the "from" page title as a string, and returns an IndexPager for
listing
+ * results.
*
- * @param TitleFormatter $titleFormatter
- * @param PageLinkRenderer $linkRenderer
+ * @throws InvalidArgumentException
*/
- public function setServices(
- TitleFormatter $titleFormatter,
- PageLinkRenderer $linkRenderer
- ) {
- $this->titleFormatter = $titleFormatter;
- $this->linkRenderer = $linkRenderer;
+ public function setPagerBuilder( $pagerBuilder ) {
+ if ( !is_callable( $pagerBuilder ) ) {
+ throw new InvalidArgumentException( '$pagerBuilder must
be callable' );
+ }
+
+ $this->pagerBuilder = $pagerBuilder;
}
/**
- * Initialize any services we'll need. Use initServices() to override.
- * This allows for dependency injection even though we don't control
object creation.
+ * Default builder for the IndexPager to be used by this special page.
+ * Will be called from newCategoryPager() unless overridden by
setPagerBuilder().
+ *
+ * @param string $from The title the pager should start at.
+ *
+ * @return CategoryPager
*/
- private function initServices() {
- $lang = $this->getContext()->getLanguage();
+ protected function buildCategoryPager( $from ) {
+ $titleFormatter = new WikitextTitleFormatter(
$this->getLanguage(), false );
+ $linkRenderer = new HtmlPageLinkRenderer( new
UrlTitleFormatter( $this->getLanguage() ) );
- if ( !$this->titleFormatter ) {
- $this->titleFormatter = new WikitextTitleFormatter(
$lang, false );
+ $resultRowFormatter = new CategoryPagerRowFormatter(
$this->getContext(), $titleFormatter, $linkRenderer );
+ return new CategoryPager( $this->getContext(), $from,
$resultRowFormatter );
+ }
+
+ /**
+ * Returns a new IndexPager suitable for listing categories starting
from
+ * the title given by $from.
+ *
+ * The IndexPager is created using the builder set via
setPagerBuilder(),
+ * defaulting to buildCategoryPager() if setPagerBuilder() wasn't used.
+ *
+ * @param string $from The title the pager should start at.
+ *
+ * @return CategoryPager
+ */
+ protected function newCategoryPager( $from ) {
+ if ( !$this->pagerBuilder ) {
+ $this->pagerBuilder = array( $this,
'buildCategoryPager' );
}
- if ( !$this->linkRenderer ) {
- $this->linkRenderer = new HtmlPageLinkRenderer( new
UrlTitleFormatter( $lang ) );
- }
+ return call_user_func( $this->pagerBuilder, $from );
}
function execute( $par ) {
- $this->initServices();
-
$this->setHeaders();
$this->outputHeader();
$this->getOutput()->allowClickjacking();
$from = $this->getRequest()->getText( 'from', $par );
- $cap = new CategoryPager( $this->getContext(), $from,
$this->titleFormatter, $this->linkRenderer );
+ $cap = $this->newCategoryPager( $from );
$cap->doQuery();
$this->getOutput()->addHTML(
@@ -114,24 +127,16 @@
class CategoryPager extends AlphabeticPager {
/**
- * @var TitleFormatter
+ * @var ResultRowFormatter
*/
- protected $titleFormatter;
-
- /**
- * @var PageLinkRenderer
- */
- protected $linkRenderer;
+ protected $rowFormatter;
/**
* @param IContextSource $context
* @param string $from
- * @param TitleFormatter $titleFormatter
- * @param PageLinkRenderer $linkRenderer
+ * @param ResultRowFormatter $rowFormatter
*/
- function __construct( IContextSource $context, $from,
- TitleFormatter $titleFormatter, PageLinkRenderer $linkRenderer
- ) {
+ function __construct( IContextSource $context, $from,
ResultRowFormatter $rowFormatter ) {
parent::__construct( $context );
$from = str_replace( ' ', '_', $from );
if ( $from !== '' ) {
@@ -140,8 +145,7 @@
$this->setIncludeOffset( true );
}
- $this->titleFormatter = $titleFormatter;
- $this->linkRenderer = $linkRenderer;
+ $this->rowFormatter = $rowFormatter;
}
function getQueryInfo() {
@@ -182,7 +186,7 @@
$this->mResult->rewind();
foreach ( $this->mResult as $row ) {
- $batch->addObj( Title::makeTitleSafe( NS_CATEGORY,
$row->cat_title ) );
+ $batch->add( NS_CATEGORY, $row->cat_title );
}
$batch->execute();
$this->mResult->rewind();
@@ -191,12 +195,7 @@
}
function formatRow( $result ) {
- $title = new TitleValue( NS_CATEGORY, $result->cat_title );
- $text = $this->titleFormatter->format( $title );
- $link = $this->linkRenderer->renderLink( $title,
htmlspecialchars( $text ) );
-
- $count = $this->msg( 'nmembers' )->numParams(
$result->cat_pages )->escaped();
- return Html::rawElement( 'li', null,
$this->getLanguage()->specialList( $link, $count ) ) . "\n";
+ return $this->rowFormatter->formatRow( $result );
}
public function getStartForm( $from ) {
@@ -218,3 +217,54 @@
);
}
}
+
+/**
+ * Formatter for result rows on SpecialCategories
+ *
+ * @license GPL 2+
+ * @author Daniel Kinzler
+ *
+ * @ingroup SpecialPage Formatter
+ */
+class CategoryPagerRowFormatter extends ContextSource implements
ResultRowFormatter {
+
+ /**
+ * @var TitleFormatter
+ */
+ protected $titleFormatter;
+
+ /**
+ * @var PageLinkRenderer
+ */
+ protected $linkRenderer;
+
+ /**
+ * @param IContextSource $context
+ * @param TitleFormatter $titleFormatter
+ * @param PageLinkRenderer $linkRenderer
+ */
+ function __construct(
+ IContextSource $context,
+ TitleFormatter $titleFormatter,
+ PageLinkRenderer $linkRenderer
+ ) {
+ parent::setContext( $context );
+
+ $this->titleFormatter = $titleFormatter;
+ $this->linkRenderer = $linkRenderer;
+ }
+
+ /**
+ * @param object $row the result row to format, as an stdclass object
+ *
+ * @return string HTML
+ */
+ function formatRow( $row ) {
+ $title = new TitleValue( NS_CATEGORY, $row->cat_title );
+ $text = $this->titleFormatter->format( $title );
+ $link = $this->linkRenderer->renderLink( $title,
htmlspecialchars( $text ) );
+
+ $count = $this->msg( 'nmembers' )->numParams( $row->cat_pages
)->escaped();
+ return Html::rawElement( 'li', null,
$this->getLanguage()->specialList( $link, $count ) ) . "\n";
+ }
+}
diff --git a/tests/phpunit/includes/specials/CategoryPagerRowFormatterTest.php
b/tests/phpunit/includes/specials/CategoryPagerRowFormatterTest.php
new file mode 100644
index 0000000..835de55
--- /dev/null
+++ b/tests/phpunit/includes/specials/CategoryPagerRowFormatterTest.php
@@ -0,0 +1,55 @@
+<?php
+
+/**
+ * @covers CategoryPagerRowFormatter
+ *
+ * @author Daniel Kinzler
+ */
+class CategoryPagerRowFormatterTest extends MediaWikiTestCase {
+
+ /**
+ * Creates a CategoryPagerRowFormatter based on mock services,
+ * providing easy-to-verify output.
+ *
+ * @return CategoryPagerRowFormatter
+ */
+ protected function newCategoryPagerRowFormatter() {
+ // make a canonical context
+ $context = new RequestContext( new FauxRequest() );
+ $context->setLanguage( 'en' );
+
+ // mock formatter providing an easy to validate title format
+ $titleFormatter = $this->getMock( 'TitleFormatter' );
+ $titleFormatter->expects( $this->any() )
+ ->method( 'format' )
+ ->will( $this->returnCallback( function( TitleValue
$title ) {
+ return 'Category:' . $title->getText() . '#' .
$title->getSection();
+ }) );
+
+ // mock link renderer providing an easy to validate output
+ $linkRenderer = $this->getMock( 'PageLinkRenderer' );
+ $linkRenderer->expects( $this->any() )
+ ->method( 'renderLink' )
+ ->will( $this->returnCallback( function( TitleValue
$title ) use ( $titleFormatter ) {
+ return "Link:" . $titleFormatter->format(
$title );
+ }) );
+
+ $formatter = new CategoryPagerRowFormatter( $context,
$titleFormatter, $linkRenderer );
+ return $formatter;
+ }
+
+ /**
+ * Test Formatting.
+ */
+ public function testFormatRow( ) {
+ $row = new stdClass();
+ $row->cat_title = 'Test';
+ $row->cat_pages = 25;
+
+ $formatter = $this->newCategoryPagerRowFormatter();
+ $html = $formatter->formatRow( $row );
+
+ //$this->assertValidHtml( $html ); //TODO: use this once
Idb98af785ca is merged
+ $this->assertRegExp( '!<li.*Link:Category:Test.*25
members.*</li>!s', $html );
+ }
+}
diff --git a/tests/phpunit/includes/specials/CategoryPagerTest.php
b/tests/phpunit/includes/specials/CategoryPagerTest.php
new file mode 100644
index 0000000..c5c0374
--- /dev/null
+++ b/tests/phpunit/includes/specials/CategoryPagerTest.php
@@ -0,0 +1,124 @@
+<?php
+
+/**
+ * @covers CategoryPager
+ *
+ * @author Daniel Kinzler
+ *
+ * @group Database
+ */
+class CategoryPagerTest extends MediaWikiTestCase {
+
+ public function setUp() {
+ parent::setUp();
+
+ $this->tablesUsed[] = 'category';
+
+ $this->insertRows(
+ 'category',
+ array( 'cat_title', 'cat_pages' ),
+ array(
+ array( 'Foo', 3 ),
+ array( 'Bar', 7 ),
+ array( 'Xyzzy', 11 ),
+ array( 'Quux', 2 ),
+ ) );
+
+ }
+
+ protected function insertRows( $table, $fields, $rows ) {
+ $dbw = wfGetDB( DB_MASTER );
+
+ foreach ( $rows as $row ) {
+ $dbw->insert( $table, array_combine( $fields, $row ),
__METHOD__ );
+ }
+ }
+
+ /**
+ * Make a mock pager
+ *
+ * @param $from
+ *
+ * @return PHPUnit_Framework_MockObject_MockObject
+ */
+ public function buildPager( $from ) {
+ $pager = $this->getMockBuilder( 'CategoryPager'
)->disableOriginalConstructor()->getMock();
+
+ return $pager;
+ }
+
+ /**
+ * Make a CategoryPager page for the given request, using a mock pager
+ * to cut out any database access.
+ *
+ * @param array $params request parameters
+ *
+ * @return CategoryPager
+ */
+ protected function newCategoryPager( array $params ) {
+ // make a canonical context
+ $context = new RequestContext( new FauxRequest( $params ) );
+ $context->setLanguage( 'en' );
+
+ // mock formatter providing an easy to validate format
+ $rowFormatter = $this->getMockBuilder(
'CategoryPagerRowFormatter' )
+ ->disableOriginalConstructor()
+ ->getMock();
+
+ $rowFormatter->expects( $this->any() )
+ ->method( 'formatRow' )
+ ->will( $this->returnCallback( function( $row ) {
+ return "\n%%" . $row->cat_title . '|' .
$row->cat_pages . "%%;\n";
+ }) );
+
+ $from = isset( $params['from'] ) ? $params['from'] : '';
+
+ $pager = new CategoryPager( $context, $from, $rowFormatter );
+ return $pager;
+ }
+
+ public function provideExpectedRows() {
+ //See setUp() for the rows present in the database
+
+ return array(
+ 'all' => array(
+ array(),
+ array(
+ 'Bar|7',
+ 'Foo|3',
+ 'Quux|2',
+ 'Xyzzy|11',
+ )
+ ),
+ 'from' => array(
+ array( 'from' => 'Quux' ),
+ array(
+ 'Quux|2',
+ 'Xyzzy|11',
+ )
+ ),
+ );
+ }
+
+ /**
+ * Test pager query execution.
+ *
+ * @dataProvider provideExpectedRows
+ */
+ public function testDoQuery( $params, $expectedRows ) {
+ $pager = $this->newCategoryPager( $params );
+
+ $pager->doQuery();
+
+ $html = $pager->getBody();
+ //$this->assertValidHtml( $html ); //TODO: use this once
Idb98af785ca is merged
+ //TODO: validate any top or bottom structures, like forms
+
+ // Since we are using a mock row formatter, the HTML will be
trivial,
+ // and can easily be split.
+ preg_match_all( '/%%(.*?)%%/', $html, $matches,
PREG_PATTERN_ORDER );
+ $actualRows = $matches[1];
+
+ $this->assertArrayEquals( $expectedRows, $actualRows );
+ }
+}
diff --git a/tests/phpunit/includes/specials/SpecialCategoriesTest.php
b/tests/phpunit/includes/specials/SpecialCategoriesTest.php
new file mode 100644
index 0000000..1a7e51c
--- /dev/null
+++ b/tests/phpunit/includes/specials/SpecialCategoriesTest.php
@@ -0,0 +1,69 @@
+<?php
+
+/**
+ * @covers SpecialCategories
+ *
+ * @author Daniel Kinzler
+ *
+ * @group Database
+ * ^--- even though we try to cut out any database access for this test,
+ * we can't be totally sure yet that *no* database access will be
done,
+ * mainly due to Title objects being used somewhere deep down.
+ */
+class SpecialCategoriesTest extends MediaWikiTestCase {
+
+ /**
+ * Make a mock pager
+ *
+ * @param $from
+ *
+ * @return PHPUnit_Framework_MockObject_MockObject
+ */
+ public function buildPager( $from ) {
+ $pager = $this->getMockBuilder( 'CategoryPager'
)->disableOriginalConstructor()->getMock();
+
+ return $pager;
+ }
+
+ /**
+ * Make a SpecialCategories page for the given request, using a mock
pager
+ * to cut out any database access.
+ *
+ * @param WebRequest $request
+ *
+ * @return SpecialCategories
+ */
+ protected function newSpecialCategoriesPage( WebRequest $request = null
) {
+ if ( $request === null ) {
+ $request = new FauxRequest();
+ }
+
+ // make a canonical context
+ $context = new RequestContext( $request );
+ $context->setLanguage( 'en' );
+
+ $page = new SpecialCategories();
+ $page->setContext( $context );
+
+ $page->setPagerBuilder( array( $this, 'buildPager' ) );
+ return $page;
+ }
+
+ /**
+ * Test special page execution. Note that this does not do much, since
we
+ * use a mock pager. If a full stack integration test is desired, that
should
+ * be done in a separate function and/or class.
+ */
+ public function testExecute() {
+ $page = $this->newSpecialCategoriesPage();
+
+ $page->execute( null );
+
+ // Just check the HTML structure
+ // Since we mocked otu the pager, execute() isn't doing
anything interesting really.
+
+ $html = $page->getOutput()->getHTML();
+ //$this->assertValidHtml( $html ); //TODO: use this once
Idb98af785ca is merged
+ $this->assertRegExp( '!<div.*</div>!s', $html );
+ }
+}
--
To view, visit https://gerrit.wikimedia.org/r/107842
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iaa22e9e0f34e563fd83a85d8b028c668840dda1c
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Daniel Kinzler <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
