r110276 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r110275‎ | r110276 | r110277 >
Date:11:08, 30 January 2012
Author:hashar
Status:reverted (Comments)
Tags:chad-flip-a-coin-please 
Comment:
drop default parameters from r110252

Html::nmaespaceSelector() generate a select using 'namespace' value
for both 'id' and 'name'. Hence passing:

array( 'name' => 'namespace', 'id' => 'namespace' )

is not needed.
Modified paths:
  • /trunk/phase3/includes/specials/SpecialAllpages.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialRecentchanges.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/specials/SpecialAllpages.php
——@@ -128,8 +128,7 @@
129129 " </td>
130130 <td class='mw-input'>" .
131131 Html::namespaceSelector(
132 - array( 'selected' => $namespace ),
133 - array( 'name' => 'namespace', 'id' => 'namespace' )
 132+ array( 'selected' => $namespace )
134133 ) . ' ' .
135134 Xml::submitButton( $this->msg( 'allpagessubmit' )->text() ) .
136135 " </td>
Index: trunk/phase3/includes/specials/SpecialRecentchanges.php
——@@ -656,8 +656,7 @@
657657 */
658658 protected function namespaceFilterForm( FormOptions $opts ) {
659659 $nsSelect = Html::namespaceSelector(
660 - array( 'selected' => $opts['namespace'], 'all' => '' ),
661 - array( 'name' => 'namespace', 'id' => 'namespace' )
 660+ array( 'selected' => $opts['namespace'], 'all' => '' )
662661 );
663662 $nsLabel = Xml::label( wfMsg( 'namespace' ), 'namespace' );
664663 $invert = Xml::checkLabel(

Follow-up revisions

RevisionCommit summaryAuthorDate
r110629Revert r110276...reedy22:57, 2 February 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r110252[specials] Xml::namespaceSelector > Html::namespaceSelector...krinkle19:05, 29 January 2012

Comments

#Comment by Krinkle (talk | contribs)   18:26, 30 January 2012

Please don't remove the 'name' from the callee. I specifically added that because SpecialRecentChanges and SpecialAllpages both use the query param "namespace" in their classes to retrieve the value, so the submission point should name this name. Not assume that Html::namespaceSelector will use the same name.

Also aside from the query param reading for the name "namespace". There is also css/javascript specific for this special page that targets the id "namespace", again, those are pairs that belong together.

This code should not assume anything about Html::namespaceSelector, it also makes debugging much easier and code more readable if for example SpecialRecentchanges::getDefaultOptions for output and the form generation for input both use the same name in their code.

I'd prefer to revert this.

#Comment by Hashar (talk | contribs)   15:39, 31 January 2012

As a summary, you seem to be afraid that someone change the 'namespace' default in Html::namespaceSelector(). So either we could:

  1. change the default name/id to something else such as wpNamespace and reinstate the array( 'name' => 'namespace', 'id' => 'namespace' ) above
  2. add a comment in Html::namespaceSelector() to keep the 'namespace' default since it is expected by various code paths / javascripts ...
#Comment by Krinkle (talk | contribs)   17:57, 31 January 2012
3. Or we could keep the default in Html::namespaceSelector for backwards compatibility and recommend that callers pass this
4. Or we could remove the defaults entirely and require that callers pass this.

I think 4 is the best option here, which is save since this method was added just last week. No compatibility issues.

#Comment by Reedy (talk | contribs)   22:57, 2 February 2012

Reverted. Something to discuss post 1.19 branching

Status & tagging log