r92382 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r92381‎ | r92382 | r92383 >
Date:20:59, 16 July 2011
Author:jeroendedauw
Status:ok (Comments)
Tags:
Comment:
added special page which will serve as admin interface for campaign config
Modified paths:
  • /trunk/extensions/UploadWizard (modified) (history)
  • /trunk/extensions/UploadWizard/SpecialUploadCampaigns.php (added) (history)
  • /trunk/extensions/UploadWizard/UploadWizard.alias.php (modified) (history)
  • /trunk/extensions/UploadWizard/UploadWizard.i18n.php (modified) (history)
  • /trunk/extensions/UploadWizard/UploadWizard.php (modified) (history)

Diff [purge]

Index: trunk/extensions/UploadWizard/UploadWizard.alias.php
@@ -9,6 +9,7 @@
1010 /** English (English) */
1111 $specialPageAliases['en'] = array(
1212 'UploadWizard' => array( 'UploadWizard' ),
 13+ 'UploadCampaigns' => array( 'UploadCampaigns' ),
1314 );
1415
1516 /** Arabic (العربية) */
Index: trunk/extensions/UploadWizard/UploadWizard.i18n.php
@@ -14,6 +14,7 @@
1515 $messages['en'] = array(
1616 'uploadwizard' => 'Upload wizard',
1717 'uploadwizard-desc' => 'Upload wizard, developed for the Multimedia Usability grant',
 18+ 'uploadcampaigns' => 'Upload campaigns',
1819 'mwe-upwiz-js-off' => 'UploadWizard uses JavaScript for an improved interface. Your browser either does not support JavaScript or has JavaScript turned off, so we are showing you a simple upload form.',
1920 'mwe-upwiz-extension-disabled' => 'This page has been disabled due to temporary technical problems. In the meantime try the standard upload form.',
2021 'mwe-upwiz-code-unknown' => 'Unknown language',
Index: trunk/extensions/UploadWizard/UploadWizard.php
@@ -36,12 +36,14 @@
3737 $wgExtensionAliasesFiles['UploadWizard'] = $wgUpwizDir . '/UploadWizard.alias.php';
3838
3939 # Require modules, including the special page
40 -foreach ( array( 'SpecialUploadWizard',
41 - 'UploadWizardMessages',
42 - 'UploadWizardHooks',
43 - 'UploadWizardTutorial'
44 - ) as $module ) {
45 - $wgAutoloadLocalClasses[$module] = $wgUpwizDir . "/" . $module . ".php";
 40+foreach ( array(
 41+ 'SpecialUploadWizard',
 42+ 'SpecialUploadCampaigns',
 43+ 'UploadWizardMessages',
 44+ 'UploadWizardHooks',
 45+ 'UploadWizardTutorial'
 46+ ) as $module ) {
 47+ $wgAutoloadLocalClasses[$module] = $wgUpwizDir . '/' . $module . '.php';
4648 }
4749
4850 // $wgAPIModules['titlecheck'] = 'ApiTitleCheck';
@@ -51,6 +53,9 @@
5254 $wgSpecialPages['UploadWizard'] = 'SpecialUploadWizard';
5355 $wgSpecialPageGroups['UploadWizard'] = 'media';
5456
 57+$wgSpecialPages['UploadCampaigns'] = 'SpecialUploadCampaigns';
 58+$wgSpecialPageGroups['UploadCampaigns'] = 'media';
 59+
5560 $wgResourceLoaderNamedPaths[ 'UploadWizardPage' ] = 'extensions/UploadWizard/UploadWizardPage.js';
5661
5762 // Set up the javascript path for the loader and localization file.
Index: trunk/extensions/UploadWizard/SpecialUploadCampaigns.php
@@ -0,0 +1,42 @@
 2+<?php
 3+/**
 4+ * Special:UploadCampaigns
 5+ *
 6+ * Configuration interface for upload wizard campaigns.
 7+ *
 8+ * @file
 9+ * @ingroup SpecialPage
 10+ * @ingroup Upload
 11+ */
 12+
 13+class SpecialUploadCampaigns extends SpecialPage {
 14+
 15+ /**
 16+ * Constructor.
 17+ *
 18+ * @param $request is the request (usually wgRequest)
 19+ * @param $par is everything in the URL after Special:UploadWizard. Not sure what we can use it for
 20+ */
 21+ public function __construct( $request = null, $par = null ) {
 22+ global $wgRequest;
 23+
 24+ parent::__construct( 'UploadCampaigns', 'delete' );
 25+
 26+ // TODO
 27+ }
 28+
 29+ /**
 30+ * Main method.
 31+ *
 32+ * @param string $subPage, e.g. the "foo" in Special:UploadWizard/foo.
 33+ */
 34+ public function execute( $subPage ) {
 35+ // If the user is authorized, display the page, if not, show an error.
 36+ if ( $this->userCanExecute( $GLOBALS['wgUser'] ) ) {
 37+ // TODO
 38+ } else {
 39+ $this->displayRestrictionError();
 40+ }
 41+ }
 42+
 43+}
\ No newline at end of file
Property changes on: trunk/extensions/UploadWizard/SpecialUploadCampaigns.php
___________________________________________________________________
Added: svn:eol-style
144 + native
Property changes on: trunk/extensions/UploadWizard
___________________________________________________________________
Modified: svn:ignore
245 - .project
346 + .project
.git

Follow-up revisions

RevisionCommit summaryAuthorDate
r92749fu r92382, don't usejeroendedauw16:10, 21 July 2011

Comments

#Comment by NeilK (talk | contribs)   23:13, 19 July 2011

I'm marking this ok, but just a few fixes for consistency:

  • change 'uploadcampaigns' to use the 'mwe-upwiz-' prefix
  • thanks for adding docs! just please add types to your @params consistently.
  • don't use $GLOBALS['wgUser'], we use $wgUser everywhere else.
#Comment by Jeroen De Dauw (talk | contribs)   23:32, 19 July 2011

> don't use $GLOBALS['wgUser'], we use $wgUser everywhere else.

Why introduce a global into your whole local scope when only using it once? Acceding it directly via $GLOBALS seems cleaner.

> just please add types to your @params consistently.

I normally do, unless it's docs copied from somewhere, or code where I don't know what the type is supposed to be.

> change 'uploadcampaigns' to use the 'mwe-upwiz-' prefix

Done in r92602 :)

#Comment by Jeroen De Dauw (talk | contribs)   23:32, 19 July 2011

Accessing - spellcheck fail...

#Comment by NeilK (talk | contribs)   00:06, 20 July 2011

We're doing an exercise in code review today at the WMF, so I happened to have Brion Vibber right next to me... we reviewed it together. We know that your code is right, but it's just a little bit extra mental effort to connect $wgUser and $GLOBALS['wgUser']. Plus, people grep for globals with "$wg" sometimes.

I don't quite see your point that $GLOBALS['wgUser'] is much safer. We have a convention and no other variables should be named $wg-anything.

As for @param types, I know that some of my code does this inconsistently too, but we're trying to get better. ;)

#Comment by Jeroen De Dauw (talk | contribs)   11:44, 20 July 2011

> I don't quite see your point that $GLOBALS['wgUser'] is much safer.

Not safer, cleaner. In any case, if it's the convention to not use this, I'll replace the stuff before my next commit :)

Status & tagging log