r111796 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r111795‎ | r111796 | r111797 >
Date:22:13, 17 February 2012
Author:bsitu
Status:ok (Comments)
Tags:
Comment:
fix for bug34431 - Disable the old toolbar if the new toolbar is enabled
Modified paths:
  • /trunk/extensions/WikiEditor/WikiEditor.hooks.php (modified) (history)
  • /trunk/extensions/WikiEditor/WikiEditor.php (modified) (history)

Diff [purge]

Index: trunk/extensions/WikiEditor/WikiEditor.php
——@@ -62,6 +62,7 @@
6363 $wgHooks['GetPreferences'][] = 'WikiEditorHooks::getPreferences';
6464 $wgHooks['ResourceLoaderGetConfigVars'][] = 'WikiEditorHooks::resourceLoaderGetConfigVars';
6565 $wgHooks['MakeGlobalVariablesScript'][] = 'WikiEditorHooks::makeGlobalVariablesScript';
 66+$wgHooks['EditPageBeforeEditToolbar'][] = 'WikiEditorHooks::EditPageBeforeEditToolbar';
6667
6768 $wikiEditorTpl = array(
6869 'localBasePath' => dirname( __FILE__ ) . '/modules',
Index: trunk/extensions/WikiEditor/WikiEditor.hooks.php
——@@ -214,6 +214,22 @@
215215 }
216216
217217 /**
 218+ * EditPageBeforeEditToolbar hook
 219+ *
 220+ * Disable the old toolbar if the new one is enabled
 221+ *
 222+ * @param $toolbar html
 223+ * @return bool
 224+ */
 225+ public static function EditPageBeforeEditToolbar( &$toolbar ) {
 226+ if ( self::isEnabled( 'toolbar' ) ) {
 227+ $toolbar = '';
 228+ return false;
 229+ }
 230+ return true;
 231+ }
 232+
 233+ /**
218234 * GetPreferences hook
219235 *
220236 * Adds WikiEditor-releated items to the preferences

Follow-up revisions

RevisionCommit summaryAuthorDate
r112160Followup r111796, always return truereedy23:07, 22 February 2012
r112164MFT r111604, r111796, r112153, r112160reedy23:31, 22 February 2012

Comments

#Comment by Aaron Schulz (talk | contribs)   23:41, 17 February 2012

If the new toolbar is loaded via JS and the user has JS off, what happens?

#Comment by Bsitu (talk | contribs)   00:09, 18 February 2012

Both toolbars are loaded via js, users would see neither of them with js turned off. Do you mean the hook shouldn't remove the following tag so html code can be injected into it for no-js version? 

<div id="toolbar"></div>
#Comment by Hashar (talk | contribs)   14:59, 20 February 2012

Looks like we need it to be deployed. Tagging 1.19wmf1

#Comment by Reedy (talk | contribs)   17:22, 21 February 2012

I'm not sure about using return false which will not allow any hooked functions after it to not be called...

#Comment by Bsitu (talk | contribs)   21:31, 21 February 2012

I was thinking that some other hook functions may add the toolbar back. If we disable it we may just stop any further hook functions running against it. I agree with you that there may be some functions registered to this hook and not related to toolbar, it's safer to make it return true.

Status & tagging log