r86786 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r86785‎ | r86786 | r86787 >
Date:18:58, 23 April 2011
Author:mah
Status:resolved (Comments)
Tags:needs-js-test 
Comment:
* Fixes Bug #27860 - Minor edit after clicking 'new section' tab

Patch from Jan Paul Posma of which he writes:

This patch hides the minor edit checkbox when editing a new page or new
section, and makes sure it cannot be enabled by injecting the form value in
HTML.

* Add Jan Paul Posma to CREDITs re Reedy's earlier comments.
* Add note about isNew to other possible places that it can be used.
Modified paths:
  • /trunk/phase3/CREDITS (modified) (history)
  • /trunk/phase3/includes/EditPage.php (modified) (history)

Diff [purge]

Index: trunk/phase3/CREDITS
——@@ -30,6 +30,7 @@
3131 * Ilmari Karonen
3232 * Jack D. Pond
3333 * Jack Phoenix
 34+* Jan Paul Posma
3435 * Jason Richey
3536 * Jon Harald Søby
3637 * Juliano F. Ravasi
Index: trunk/phase3/includes/EditPage.php
——@@ -57,6 +57,7 @@
5858 var $isCssJsSubpage = false;
5959 var $isCssSubpage = false;
6060 var $isJsSubpage = false;
 61+ var $isNew = false; // new page or new section
6162 var $deletedSinceEdit;
6263 var $formtype;
6364 var $firsttime;
——@@ -165,7 +166,7 @@
166167
167168 $preload = $wgRequest->getVal( 'preload',
168169 // Custom preload text for new sections
169 - $section === 'new' ? 'MediaWiki:addsection-preload' : '' );
 170+ $section === 'new' ? 'MediaWiki:addsection-preload' : '' ); /* use $this->isNew here? */
170171 $undoafter = $wgRequest->getVal( 'undoafter' );
171172 $undo = $wgRequest->getVal( 'undo' );
172173
——@@ -232,7 +233,7 @@
233234 $this->editFormPageTop .= $wgOut->parse( '<div class="error mw-undo-norev">' . wfMsgNoTrans( 'undo-norev' ) . '</div>' );
234235 }
235236 } else if ( $section != '' ) {
236 - if ( $section == 'new' ) {
 237+ if ( $section == 'new' ) { /* use $this->isNew here? */
237238 $text = $this->getPreloadedText( $preload );
238239 } else {
239240 // Get section edit text (returns $def_text for invalid sections)
——@@ -415,10 +416,11 @@
416417
417418 $this->isConflict = false;
418419 // css / js subpages of user pages get a special treatment
419 - $this->isCssJsSubpage = $this->mTitle->isCssJsSubpage();
420 - $this->isCssSubpage = $this->mTitle->isCssSubpage();
421 - $this->isJsSubpage = $this->mTitle->isJsSubpage();
 420+ $this->isCssJsSubpage = $this->mTitle->isCssJsSubpage();
 421+ $this->isCssSubpage = $this->mTitle->isCssSubpage();
 422+ $this->isJsSubpage = $this->mTitle->isJsSubpage();
422423 $this->isWrongCaseCssJsPage = $this->isWrongCaseCssJsPage();
 424+ $this->isNew = !$this->mTitle->exists() || $this->section == 'new';
423425
424426 # Show applicable editing introductions
425427 if ( $this->formtype == 'initial' || $this->firsttime )
——@@ -530,7 +532,7 @@
531533 } elseif ( $wgRequest->getVal( 'preview' ) == 'no' ) {
532534 // Explicit override from request
533535 return false;
534 - } elseif ( $this->section == 'new' ) {
 536+ } elseif ( $this->section == 'new' ) { /* use $this->isNew here? */
535537 // Nothing *to* preview for new sections
536538 return false;
537539 } elseif ( ( $wgRequest->getVal( 'preload' ) !== null || $this->mTitle->exists() ) && $wgUser->getOption( 'previewonfirst' ) ) {
——@@ -688,10 +690,10 @@
689691 $this->watchthis = $request->getBool( 'watchthis', false ); // Watch may be overriden by request parameters
690692 $this->recreate = false;
691693
692 - if ( $this->section == 'new' && $request->getVal( 'preloadtitle' ) ) {
 694+ if ( $this->section == 'new' && $request->getVal( 'preloadtitle' ) ) { /* use $this->isNew here? */
693695 $this->summary = $request->getVal( 'preloadtitle' );
694696 }
695 - elseif ( $this->section != 'new' && $request->getVal( 'summary' ) ) {
 697+ elseif ( $this->section != 'new' && $request->getVal( 'summary' ) ) { /* use $this->isNew here? */
696698 $this->summary = $request->getText( 'summary' );
697699 }
698700
——@@ -709,7 +711,7 @@
710712 $this->live = $request->getCheck( 'live' );
711713 $this->editintro = $request->getText( 'editintro',
712714 // Custom edit intro for new sections
713 - $this->section === 'new' ? 'MediaWiki:addsection-editintro' : '' );
 715+ $this->section === 'new' ? 'MediaWiki:addsection-editintro' : '' ); /* use $this->isNew here? */
714716
715717 // Allow extensions to modify form data
716718 wfRunHooks( 'EditPage::importFormData', array( $this, $request ) );
——@@ -976,7 +978,7 @@
977979 }
978980
979981 $text = $this->textbox1;
980 - if ( $this->section == 'new' && $this->summary != '' ) {
 982+ if ( $this->section == 'new' && $this->summary != '' ) { /* use $this->isNew here? */
981983 $text = wfMsgForContent( 'newsectionheaderdefaultlevel', $this->summary ) . "\n\n" . $text;
982984 }
983985
——@@ -992,7 +994,7 @@
993995
994996 if ( $this->mArticle->getTimestamp() != $this->edittime ) {
995997 $this->isConflict = true;
996 - if ( $this->section == 'new' ) {
 998+ if ( $this->section == 'new' ) { /* use $this->isNew here? */
997999 if ( $this->mArticle->getUserText() == $wgUser->getName() &&
9981000 $this->mArticle->getComment() == $this->summary ) {
9991001 // Probably a duplicate submission of a new comment.
——@@ -1058,7 +1060,7 @@
10591061 }
10601062
10611063 # Handle the user preference to force summaries here, but not for null edits
1062 - if ( $this->section != 'new' && !$this->allowBlankSummary && 0 != strcmp( $oldtext, $text )
 1064+ if ( $this->section != 'new' && !$this->allowBlankSummary && 0 != strcmp( $oldtext, $text ) /* use $this->isNew here? */
10631065 && !Title::newFromRedirect( $text ) ) # check if it's not a redirect
10641066 {
10651067 if ( md5( $this->summary ) == $this->autoSumm ) {
——@@ -1069,7 +1071,7 @@
10701072 }
10711073
10721074 # And a similar thing for new sections
1073 - if ( $this->section == 'new' && !$this->allowBlankSummary ) {
 1075+ if ( $this->section == 'new' && !$this->allowBlankSummary ) { /* use $this->isNew here? */
10741076 if ( trim( $this->summary ) == '' ) {
10751077 $this->missingSummary = true;
10761078 wfProfileOut( __METHOD__ );
——@@ -1080,7 +1082,7 @@
10811083 # All's well
10821084 wfProfileIn( __METHOD__ . '-sectionanchor' );
10831085 $sectionanchor = '';
1084 - if ( $this->section == 'new' ) {
 1086+ if ( $this->section == 'new' ) { /* use $this->isNew here? */
10851087 if ( $this->textbox1 == '' ) {
10861088 $this->missingComment = true;
10871089 wfProfileOut( __METHOD__ . '-sectionanchor' );
——@@ -1128,7 +1130,7 @@
11291131
11301132 $flags = EDIT_DEFER_UPDATES | EDIT_AUTOSUMMARY |
11311133 ( $new ? EDIT_NEW : EDIT_UPDATE ) |
1132 - ( $this->minoredit ? EDIT_MINOR : 0 ) |
 1134+ ( ( $this->minoredit && !$this->isNew ) ? EDIT_MINOR : 0 ) |
11331135 ( $bot ? EDIT_FORCE_BOT : 0 );
11341136
11351137 $status = $this->mArticle->doEdit( $text, $this->summary, $flags );
——@@ -1237,7 +1239,7 @@
12381240 # Already watched
12391241 $this->watchthis = true;
12401242 }
1241 - if ( $wgUser->getOption( 'minordefault' ) ) $this->minoredit = true;
 1243+ if ( $wgUser->getOption( 'minordefault' ) && !$this->isNew ) $this->minoredit = true;
12421244 if ( $this->textbox1 === false ) return false;
12431245 wfProxyCheck();
12441246 return true;
——@@ -1252,7 +1254,7 @@
12531255 if ( $this->isConflict ) {
12541256 $wgOut->setPageTitle( wfMsg( 'editconflict', $this->getContextTitle()->getPrefixedText() ) );
12551257 } elseif ( $this->section != '' ) {
1256 - $msg = $this->section == 'new' ? 'editingcomment' : 'editingsection';
 1258+ $msg = $this->section == 'new' ? 'editingcomment' : 'editingsection'; /* use $this->isNew here? */
12571259 $wgOut->setPageTitle( wfMsg( $msg, $this->getContextTitle()->getPrefixedText() ) );
12581260 } else {
12591261 # Use the title defined by DISPLAYTITLE magic word when present
——@@ -1375,14 +1377,14 @@
13761378 # For a bit more sophisticated detection of blank summaries, hash the
13771379 # automatic one and pass that in the hidden field wpAutoSummary.
13781380 if ( $this->missingSummary ||
1379 - ( $this->section == 'new' && $this->nosummary ) )
 1381+ ( $this->section == 'new' && $this->nosummary ) ) /* use $this->isNew here? */
13801382 $wgOut->addHTML( Html::hidden( 'wpIgnoreBlankSummary', true ) );
13811383 $autosumm = $this->autoSumm ? $this->autoSumm : md5( $this->summary );
13821384 $wgOut->addHTML( Html::hidden( 'wpAutoSummary', $autosumm ) );
13831385
13841386 $wgOut->addHTML( Html::hidden( 'oldid', $this->mArticle->getOldID() ) );
13851387
1386 - if ( $this->section == 'new' ) {
 1388+ if ( $this->section == 'new' ) { /* use $this->isNew here? */
13871389 $this->showSummaryInput( true, $this->summary );
13881390 $wgOut->addHTML( $this->getSummaryPreview( true, $this->summary ) );
13891391 }
——@@ -1449,7 +1451,7 @@
14501452 return false;
14511453 }
14521454
1453 - if ( $this->section != '' && $this->section != 'new' ) {
 1455+ if ( $this->section != '' && $this->section != 'new' ) { /* use $this->isNew here? */
14541456 $matches = array();
14551457 if ( !$this->summary && !$this->preview && !$this->diff ) {
14561458 preg_match( "/^(=+)(.+)\\1/mi", $this->textbox1, $matches );
——@@ -1466,11 +1468,11 @@
14671469 $wgOut->wrapWikiMsg( "<div id='mw-missingcommenttext'>\n$1\n</div>", 'missingcommenttext' );
14681470 }
14691471
1470 - if ( $this->missingSummary && $this->section != 'new' ) {
 1472+ if ( $this->missingSummary && $this->section != 'new' ) { /* use $this->isNew here? */
14711473 $wgOut->wrapWikiMsg( "<div id='mw-missingsummary'>\n$1\n</div>", 'missingsummary' );
14721474 }
14731475
1474 - if ( $this->missingSummary && $this->section == 'new' ) {
 1476+ if ( $this->missingSummary && $this->section == 'new' ) { /* use $this->isNew here? */
14751477 $wgOut->wrapWikiMsg( "<div id='mw-missingcommentheader'>\n$1\n</div>", 'missingcommentheader' );
14761478 }
14771479
——@@ -1854,7 +1856,7 @@
18551857 global $wgOut, $wgUser;
18561858 $wgOut->addHTML( "<div class='editOptions'>\n" );
18571859
1858 - if ( $this->section != 'new' ) {
 1860+ if ( $this->section != 'new' ) { /* use $this->isNew here? */
18591861 $this->showSummaryInput( false, $this->summary );
18601862 $wgOut->addHTML( $this->getSummaryPreview( false, $this->summary ) );
18611863 }
——@@ -1997,7 +1999,7 @@
19982000
19992001 # If we're adding a comment, we need to show the
20002002 # summary as the headline
2001 - if ( $this->section == "new" && $this->summary != "" ) {
 2003+ if ( $this->section == "new" && $this->summary != "" ) { /* use $this->isNew here? */
20022004 $toparse = "== {$this->summary} ==\n\n" . $toparse;
20032005 }
20042006
——@@ -2439,19 +2441,22 @@
24402442
24412443 $checkboxes = array();
24422444
2443 - $checkboxes['minor'] = '';
2444 - $minorLabel = wfMsgExt( 'minoredit', array( 'parseinline' ) );
2445 - if ( $wgUser->isAllowed( 'minoredit' ) ) {
2446 - $attribs = array(
2447 - 'tabindex' => ++$tabindex,
2448 - 'accesskey' => wfMsg( 'accesskey-minoredit' ),
2449 - 'id' => 'wpMinoredit',
2450 - );
2451 - $checkboxes['minor'] =
2452 - Xml::check( 'wpMinoredit', $checked['minor'], $attribs ) .
2453 - "&#160;<label for='wpMinoredit' id='mw-editpage-minoredit'" .
2454 - Xml::expandAttributes( array( 'title' => $skin->titleAttrib( 'minoredit', 'withaccess' ) ) ) .
2455 - ">{$minorLabel}</label>";
 2445+ // don't show the minor edit checkbox if it's a new page or section
 2446+ if ( !$this->isNew ) {
 2447+ $checkboxes['minor'] = '';
 2448+ $minorLabel = wfMsgExt( 'minoredit', array( 'parseinline' ) );
 2449+ if ( $wgUser->isAllowed( 'minoredit' ) ) {
 2450+ $attribs = array(
 2451+ 'tabindex' => ++$tabindex,
 2452+ 'accesskey' => wfMsg( 'accesskey-minoredit' ),
 2453+ 'id' => 'wpMinoredit',
 2454+ );
 2455+ $checkboxes['minor'] =
 2456+ Xml::check( 'wpMinoredit', $checked['minor'], $attribs ) .
 2457+ "&#160;<label for='wpMinoredit' id='mw-editpage-minoredit'" .
 2458+ Xml::expandAttributes( array( 'title' => $skin->titleAttrib( 'minoredit', 'withaccess' ) ) ) .
 2459+ ">{$minorLabel}</label>";
 2460+ }
24562461 }
24572462
24582463 $watchLabel = wfMsgExt( 'watchthis', array( 'parseinline' ) );

Follow-up revisions

RevisionCommit summaryAuthorDate
r86834Remove hints since $this->isNew was not possible anywhere else.mah03:20, 25 April 2011
r99924Re: r86786 … add a RELEASE-NOTES entrymah22:51, 15 October 2011

Comments

#Comment by JanPaul123 (talk | contribs)   20:59, 23 April 2011

$this->isNew doesn't apply to most suggestions you make, as it's set for new sections but also new pages! Most of the suggestions should only be triggered on new sections. I'll look into this later to verify.

#Comment by JanPaul123 (talk | contribs)   17:40, 24 April 2011

Can you revert this patch and apply the original patch again? (I don't have core access yet.) I've looked at it, and so far we cannot use $this->isNew at the other locations you suggested, unfortunately.

#Comment by MarkAHershberger (talk | contribs)   03:22, 25 April 2011

See r86834 where I just removed the hints. Easier than reverting and re-applying :)

#Comment by Krinkle (talk | contribs)   21:07, 23 April 2011

Does this diff actually change anything ?

I see many whitespace changes, code style changes and suggestions in comments...

#Comment by MarkAHershberger (talk | contribs)   22:24, 23 April 2011

See http://bug-attachment.wikimedia.org/attachment.cgi?id=8452 for the core diff

#Comment by Brion VIBBER (talk | contribs)   22:08, 14 June 2011

Seems to work ok. Extra comments cleaned up in r86834.

We'll want JS test cases for this once we have infrastructure to run 'live' wiki texts via qunit, to confirm that the checkbox consistent does/doesn't show and that injection doesn't work. Not letting that block anything as we don't have that yet.

#Comment by Liangent (talk | contribs)   12:39, 8 October 2011

This change is not that trivial. Some Wikipedia users are asking about this. I guess this worth a line in RELEASE-NOTE.

Status & tagging log