Mwjames has uploaded a new change for review. https://gerrit.wikimedia.org/r/66537
Change subject: Use SMW\TableFormatter for the table query printer (SMWTableResultPrinter) ...................................................................... Use SMW\TableFormatter for the table query printer (SMWTableResultPrinter) SMWTableResultPrinter has a CRAP of 478 which puts this printer into a category of a potential hazardous printer that can be broken easily. Doing data gathering and doing formatting are fundamentally two different things and should not be mixed as it increases complexity and while having attributes being dependent on data they present, HTML related code has nothing to do with the data itself therefore it is the wrong place and use a formatting class instead. It doesn't solves the inherent problem of the SMWTableResultPrinter class but it allows for seperate unit testing and coverage. SMW\TableFormatter Code overage: 100% CRAP: 13 SMWTableResultPrinter Code coverage: 33.33% CRAP: before 478 after 358 ## Note SMW\TableFormatter is used for the Factbox as well. [1] http://www.semantic-mediawiki.org/wiki/CRAP Change-Id: I11c0057669e0a18c7b573ef64b21414fcb202b01 --- M SemanticMediaWiki.hooks.php M includes/Setup.php A includes/formatters/TableFormatter.php M includes/queryprinters/SMW_QP_Table.php A tests/phpunit/includes/formatters/TableFormatterTest.php 5 files changed, 430 insertions(+), 71 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/SemanticMediaWiki refs/changes/37/66537/1 diff --git a/SemanticMediaWiki.hooks.php b/SemanticMediaWiki.hooks.php index 23d9629..13bca95 100644 --- a/SemanticMediaWiki.hooks.php +++ b/SemanticMediaWiki.hooks.php @@ -257,6 +257,7 @@ 'Factbox', 'formatters/ParserParameterFormatter', + 'formatters/TableFormatter', 'handlers/CacheHandler', diff --git a/includes/Setup.php b/includes/Setup.php index 880d931..33f1edd 100644 --- a/includes/Setup.php +++ b/includes/Setup.php @@ -147,6 +147,7 @@ $wgAutoloadClasses['SMW\ArrayFormatter'] = $incDir . 'formatters/ArrayFormatter.php'; $wgAutoloadClasses['SMW\ParserParameterFormatter'] = $incDir . 'formatters/ParserParameterFormatter.php'; + $wgAutoloadClasses['SMW\TableFormatter'] = $incDir . 'formatters/TableFormatter.php'; $wgAutoloadClasses['SMW\Settings'] = $incDir . 'Settings.php'; diff --git a/includes/formatters/TableFormatter.php b/includes/formatters/TableFormatter.php new file mode 100644 index 0000000..8e69a07 --- /dev/null +++ b/includes/formatters/TableFormatter.php @@ -0,0 +1,171 @@ +<?php + +namespace SMW; + +use Html; + +/** + * Class handling Html table formatting + * + * 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 + * + * @since 1.9 + * + * @file + * @ingroup Formatter + * + * @author mwjames + */ + +/** + * Class handling Html table formatting + * + * @ingroup Formatter + */ +class TableFormatter { + + /** @var array */ + protected $headerItems = array(); + + /** @var array */ + protected $tableRows = array(); + + /** @var array */ + protected $tableHeaders = array(); + + /** + * @since 1.9 + * + * @param boolean $htmlContext + */ + public function __construct( $htmlContext = false ) { + $this->htmlContext = $htmlContext; + } + + /** + * Adds a header item to an internal array + * + * @since 1.9 + * + * @param string $element + * @param string $content + * @param array $attributes + * + * @return string + */ + public function addHeaderItem( $element, $content = '', $attributes = array() ) { + $this->headerItems[] = Html::rawElement( $element, $attributes, $content ); + } + + /** + * Returns concatenated header items + * + * @since 1.9 + * + * @return string + */ + public function getHeader() { + return implode( '', $this->headerItems ); + } + + /** + * Adds a table row to an internal array + * + * @since 1.9 + * + * @param string $content + * @param array $attributes + * + * @return string + */ + public function addTableRow( $content = '', $attributes = array() ) { + $alternate = count( $this->tableRows ) % 2 == 0 ? 'row-odd' : 'row-even'; + + if ( isset( $attributes['class'] ) ) { + $attributes['class'] = $attributes['class'] . ' ' . $alternate; + } else { + $attributes['class'] = $alternate; + } + + $this->tableRows[] = Html::rawElement( 'tr', $attributes , $content ); + } + + /** + * Adds a table header + * + * @since 1.9 + * + * @param string $content + * @param array $attributes + * + * @return string + */ + public function addTableHeader( $content = '', $attributes = array() ) { + $this->tableHeaders[] = Html::rawElement( 'th', $attributes, $content ); + } + + /** + * Returns a table cell + * + * @since 1.9 + * + * @param string $content + * @param array $attributes + * + * @return string + */ + public function addTableCell( $content = '', $attributes = array() ) { + return Html::rawElement( 'td', $attributes, $content ); + } + + /** + * Returns concatenated table headers + * + * @since 1.9 + * + * @return string + */ + public function getTableHeader() { + return $this->htmlContext ? Html::rawElement( 'thead', array(), implode( '', $this->tableHeaders ) ) : implode( '', $this->tableHeaders ); + } + + /** + * Returns concatenated table rows + * + * @since 1.9 + * + * @return string + */ + public function getTableRows() { + return $this->htmlContext ? Html::rawElement( 'tbody', array(), implode( '', $this->tableRows ) ) : implode( '', $this->tableRows ); + } + + /** + * Returns a formatted table + * + * @since 1.9 + * + * @param array $attributes + * + * @return string + */ + public function getTable( $attributes = array() ) { + return Html::rawElement( 'table', + $attributes, + $this->getTableHeader() . $this->getTableRows() + ); + } +} diff --git a/includes/queryprinters/SMW_QP_Table.php b/includes/queryprinters/SMW_QP_Table.php index 30c3ff8..444edfc 100644 --- a/includes/queryprinters/SMW_QP_Table.php +++ b/includes/queryprinters/SMW_QP_Table.php @@ -2,10 +2,10 @@ /** * Print query results in tables. - * + * * @author Markus Krötzsch * @author Jeroen De Dauw < [email protected] > - * + * * @file * @ingroup SMWQuery */ @@ -19,12 +19,15 @@ protected function getResultText( SMWQueryResult $res, $outputmode ) { $result = ''; - + + $this->isHTML = ( $outputmode === SMW_OUTPUT_HTML ); + $this->tableFormatter = new \SMW\TableFormatter( $this->isHTML ); + $columnClasses = array(); - + if ( $this->mShowHeaders != SMW_HEADERS_HIDE ) { // building headers $headers = array(); - + foreach ( $res->getPrintRequests() as /* SMWPrintRequest */ $pr ) { $attribs = array(); $columnClass = str_replace( array( ' ', '_' ), '-', strip_tags( $pr->getText( SMW_OUTPUT_WIKI ) ) ); @@ -33,71 +36,54 @@ // use in displaying each row. $columnClasses[] = $columnClass; $text = $pr->getText( $outputmode, ( $this->mShowHeaders == SMW_HEADERS_PLAIN ? null : $this->mLinker ) ); - - $headers[] = Html::rawElement( - 'th', - $attribs, - $text === '' ? ' ' : $text - ); - } - - $headers = '<tr>' . implode( "\n", $headers ) . '</tr>'; - - if ( $outputmode == SMW_OUTPUT_HTML ) { - $headers = '<thead>' . $headers . '</thead>'; - } - $headers = "\n$headers\n"; - $result .= $headers; + $this->tableFormatter->addTableHeader( ( $text === '' ? ' ' : $text ), $attribs ); + } } - + $tableRows = array(); $rowNum = 1; while ( $subject = $res->getNext() ) { - $tableRows[] = $this->getRowForSubject( $subject, $outputmode, $columnClasses, $rowNum++ ); + $this->tableFormatter->addTableRow( + $this->getRowForSubject( $subject, $outputmode, $columnClasses, $rowNum++ ) + ); } - $tableRows = implode( "\n", $tableRows ); - - if ( $outputmode == SMW_OUTPUT_HTML ) { - $tableRows = '<tbody>' . $tableRows . '</tbody>'; - } - - $result .= $tableRows; - // print further results footer if ( $this->linkFurtherResults( $res ) ) { $link = $this->getFurtherResultsLink( $res, $outputmode ); - $result .= "\t<tr class=\"smwfooter\"><td class=\"sortbottom\" colspan=\"" . $res->getColumnCount() . '"> ' . $link->getText( $outputmode, $this->mLinker ) . "</td></tr>\n"; + + $this->tableFormatter->addTableRow( + $this->tableFormatter->addTableCell( + $link->getText( $outputmode, $this->mLinker ), + array( 'class' => 'sortbottom', 'colspan' => $res->getColumnCount() ) + ), + array( 'class' => 'smwfooter' ) + ); } - - // Put the <table> tag around the whole thing + $tableAttrs = array( 'class' => $this->params['class'] ); - + if ( $this->mFormat == 'broadtable' ) { $tableAttrs['width'] = '100%'; } - - $result = Xml::tags( 'table', $tableAttrs, $result ); - $this->isHTML = ( $outputmode == SMW_OUTPUT_HTML ); // yes, our code can be viewed as HTML if requested, no more parsing needed - - return $result; + return $this->tableFormatter->getTable( $tableAttrs ); } /** * Gets a single table row for a subject, ie page. - * + * * @since 1.6.1 - * + * * @param array $subject * @param $outputmode - * + * * @return string */ protected function getRowForSubject( array /* of SMWResultArray */ $subject, $outputmode, $columnClasses, $rowNum ) { - $cells = array(); - + $cells = ''; + foreach ( $subject as $i => $field ) { // $columnClasses will be empty if "headers=hide" // was set. @@ -106,43 +92,42 @@ } else { $columnClass = null; } - $cells[] = $this->getCellForPropVals( $field, $outputmode, $columnClass ); + $cells = $cells . $this->getCellForPropVals( $field, $outputmode, $columnClass ); } - - $rowClass = ( $rowNum % 2 == 1 ) ? 'row-odd' : 'row-even'; - return "<tr class=\"$rowClass\">\n\t" . implode( "\n\t", $cells ) . "\n</tr>"; + + return $cells; } - + /** * Gets a table cell for all values of a property of a subject. - * + * * @since 1.6.1 - * + * * @param SMWResultArray $resultArray * @param $outputmode - * + * * @return string */ protected function getCellForPropVals( SMWResultArray $resultArray, $outputmode, $columnClass ) { $dataValues = array(); - + while ( ( $dv = $resultArray->getNextDataValue() ) !== false ) { $dataValues[] = $dv; } - + $attribs = array(); $content = null; - + if ( count( $dataValues ) > 0 ) { $sortkey = $dataValues[0]->getDataItem()->getSortKey(); $dataValueType = $dataValues[0]->getTypeID(); - + if ( is_numeric( $sortkey ) ) { $attribs['data-sort-value'] = $sortkey; } - + $alignment = trim( $resultArray->getPrintRequest()->getParameter( 'align' ) ); - + if ( in_array( $alignment, array( 'right', 'left', 'center' ) ) ) { $attribs['style'] = "text-align:' . $alignment . ';"; } @@ -154,33 +139,29 @@ $resultArray->getPrintRequest()->getMode() == SMWPrintRequest::PRINT_THIS ); } - - return Html::rawElement( - 'td', - $attribs, - $content - ); + + return $this->tableFormatter->addTableCell( $content, $attribs ); } - + /** * Gets the contents for a table cell for all values of a property of a subject. - * + * * @since 1.6.1 - * + * * @param array $dataValues * @param $outputmode * @param boolean $isSubject - * + * * @return string */ protected function getCellContent( array /* of SMWDataValue */ $dataValues, $outputmode, $isSubject ) { $values = array(); - + foreach ( $dataValues as $dv ) { $value = $dv->getShortText( $outputmode, $this->getLinker( $isSubject ) ); $values[] = $value; } - + return implode( '<br />', $values ); } @@ -204,5 +185,4 @@ return $params; } - } diff --git a/tests/phpunit/includes/formatters/TableFormatterTest.php b/tests/phpunit/includes/formatters/TableFormatterTest.php new file mode 100644 index 0000000..cf74033 --- /dev/null +++ b/tests/phpunit/includes/formatters/TableFormatterTest.php @@ -0,0 +1,206 @@ +<?php + +namespace SMW\Test; + +use SMW\TableFormatter; + +/** + * Tests for the TableFormatter class + * + * 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 + * + * @since 1.9 + * + * @file + * @ingroup Test + * + * @licence GNU GPL v2+ + * @author mwjames + */ + +/** + * Tests for the TableFormatter class + * @covers \SMW\TableFormatter + * + * @ingroup Test + * + * @group SMW + * @group SMWExtension + */ +class TableFormatterTest extends SemanticMediaWikiTestCase { + + /** + * Returns the name of the class to be tested + * + * @return string|false + */ + public function getClass() { + return '\SMW\TableFormatter'; + } + + /** + * Helper method that returns a TableFormatter object + * + * @since 1.9 + * + * @param array $params + * + * @return TableFormatter + */ + private function getInstance( $htmlContext = false ) { + return new TableFormatter( $htmlContext ); + } + + /** + * @test TableFormatter::__construct + * + * @since 1.9 + */ + public function testConstructor() { + $instance = $this->getInstance(); + $this->assertInstanceOf( $this->getClass(), $instance ); + } + + /** + * @test TableFormatter::addHeaderItem + * + * @since 1.9 + */ + public function testAddHeaderItem() { + $instance = $this->getInstance(); + $instance->addHeaderItem( 'span', 'lala' ); + + $this->assertContains( 'span', $instance->getHeader() ); + } + + /** + * @test TableFormatter::addTableHeader + * @test TableFormatter::getTableHeader + * + * @since 1.9 + */ + public function testAddTableHeader() { + $instance = $this->getInstance(); + $instance->addTableHeader( 'lala' ); + + $this->assertContains( 'th', $instance->getTableHeader() ); + + // HTML context + $instance = $this->getInstance( true ); + $instance->addTableHeader( 'lila' ); + + $this->assertContains( 'thead', $instance->getTableHeader() ); + } + + /** + * @test TableFormatter::addTableRow + * @test TableFormatter::addTableCell + * + * @since 1.9 + */ + public function testAddTableRow() { + $instance = $this->getInstance(); + $instance->addTableRow( $instance->addTableCell( 'lala' ), array( 'class' => 'foo' ) ); + $instance->addTableRow( $instance->addTableCell( 'lula' ) ); + + $matcher = array( + 'tag' => 'tr', + 'attributes' => array( 'class' => 'foo row-odd' ), + 'attributes' => array( 'class' => 'row-even' ), + 'descendant' => array( + 'tag' => 'td', + 'content' => 'lala' + ), + 'descendant' => array( + 'tag' => 'td', + 'content' => 'lula' + ) + ); + + $this->assertTag( $matcher, $instance->getTableRows() ); + + // HTML context + $instance = $this->getInstance( true ); + $instance->addTableRow( $instance->addTableCell( 'lila' ) ); + + $matcher = array( + 'tag' => 'tbody', + 'descendant' => array( + 'tag' => 'tr', + 'attributes' => array( 'class' => 'row-odd' ), + 'child' => array( 'tag' => 'td', 'content' => 'lila' ), + ) + ); + + $this->assertTag( $matcher, $instance->getTableRows() ); + + } + + /** + * @test TableFormatter::getTable + * + * @since 1.9 + */ + public function testGetTable() { + $instance = $this->getInstance(); + + // Row + cell + $instance->addTableRow( $instance->addTableCell( 'lala' ) ); + + $matcher = array( + 'tag' => 'table', + 'descendant' => array( + 'tag' => 'tr', + 'attributes' => array( 'class' => 'row-odd' ), + 'child' => array( 'tag' => 'td', 'content' => 'lala' ), + ) + ); + + $this->assertTag( $matcher, $instance->getTable() ); + + // Head + row + cell + $instance->addTableHeader( 'lula' ); + + $matcher = array( + 'tag' => 'table', + 'descendant' => array( + 'tag' => 'th', + 'child' => array( 'tag' => 'td', 'content' => 'lula' ), + ), + 'descendant' => array( + 'tag' => 'tr', + 'attributes' => array( 'class' => 'row-odd' ), + 'child' => array( 'tag' => 'td', 'content' => 'lala' ), + ) + ); + + $this->assertTag( $matcher, $instance->getTable() ); + + // HTML context + $instance = $this->getInstance( true ); + $instance->addTableHeader( 'lula' ); + $instance->addTableRow( $instance->addTableCell( 'lala' ) ); + + // I'm doing lazy assert here ... + $matcher = array( + 'tag' => 'table', + 'child' => array( 'tag' => 'thead', 'child' => array( 'content' => 'lula' ) ), + 'child' => array( 'tag' => 'tbody', 'child' => array( 'content' => 'lala' ) ), + ); + $this->assertTag( $matcher, $instance->getTable() ); + + } +} -- To view, visit https://gerrit.wikimedia.org/r/66537 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I11c0057669e0a18c7b573ef64b21414fcb202b01 Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/SemanticMediaWiki Gerrit-Branch: master Gerrit-Owner: Mwjames <[email protected]> _______________________________________________ MediaWiki-commits mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
