r96010 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r96009‎ | r96010 | r96011 >
Date:16:53, 1 September 2011
Author:catrope
Status:ok (Comments)
Tags:
Comment:
RL2: Refactor GadgetPageList resulting in less code duplication in GadgetHooks
* Factor getRowForTitle() out of add(), will be useful for the maintenance script I'm about to commit
* Factor the duplicated if-redirect-else logic out into updatePageStatus()
** also use it in the undelete hook, it appears that previously suffered of a bug where undeleting a newer revision on top of an older one making the page a redirect didn't cause it to be unlisted in gadgetpagelist
* Add a function isGadgetPage() that checks if a page qualifies for being in the table. Not used anywhere right now but will be used by the maintenance script
Modified paths:
  • /branches/RL2/extensions/Gadgets/GadgetHooks.php (modified) (history)
  • /branches/RL2/extensions/Gadgets/backend/GadgetPageList.php (modified) (history)

Diff [purge]

Index: branches/RL2/extensions/Gadgets/GadgetHooks.php
@@ -149,13 +149,7 @@
150150 $isWatch, $section, $flags, $revision )
151151 {
152152 $title = $article->getTitle();
153 - if ( $title->isCssOrJsPage() || $title->isCssJsSubpage() ) {
154 - if ( $title->isRedirect() ) {
155 - GadgetPageList::delete( $title );
156 - } else {
157 - GadgetPageList::add( $title );
158 - }
159 - }
 153+ GadgetPageList::updatePageStatus( $title );
160154 return true;
161155 }
162156
@@ -166,9 +160,7 @@
167161 * @param $comment String: Undeletion summary
168162 */
169163 public static function cssOrJsPageUndelete( $title, $created, $comment ) {
170 - if ( ( $title->isCssOrJsPage() || $title->isCssJsSubpage() ) && !$title->isRedirect() ) {
171 - GadgetPageList::add( $title );
172 - }
 164+ GadgetPageList::updatePageStatus( $title );
173165 return true;
174166 }
175167
@@ -177,13 +169,7 @@
178170 // it'll be a redirect and we don't want those in there
179171 GadgetPageList::delete( $oldTitle );
180172
181 - if ( $newTitle->isCssOrJsPage() || $newTitle->isCssJsSubpage() ) {
182 - if ( $title->isRedirect() ) {
183 - GadgetPageList::delete( $newTitle );
184 - } else {
185 - GadgetPageList::add( $newTitle );
186 - }
187 - }
 173+ GadgetPageList::updatePageStatus( $newTitle );
188174 return true;
189175 }
190176
Index: branches/RL2/extensions/Gadgets/backend/GadgetPageList.php
@@ -28,16 +28,54 @@
2929 }
3030
3131 /**
 32+ * Check whether a given title is a gadget page
 33+ * @param $title Title object
 34+ * @return bool True if $title is a CSS/JS page and isn't a redirect, false otherwise
 35+ */
 36+ public static function isGadgetPage( $title ) {
 37+ return ( $title->isCssOrJsPage() || $title->isCssJsSubpage() ) && !$title->isRedirect();
 38+ }
 39+
 40+ /**
 41+ * Get a row for the gadgetpagelist table
 42+ * @param $title Title object
 43+ * @return array Database row
 44+ */
 45+ public static function getRowForTitle( $title ) {
 46+ return array(
 47+ 'gpl_extension' => self::determineExtension( $title ),
 48+ 'gpl_namespace' => $title->getNamespace(),
 49+ 'gpl_title' => $title->getDBKey()
 50+ );
 51+ }
 52+
 53+ /**
 54+ * Update the status of a title, typically called when a title has been
 55+ * edited or created.
 56+ *
 57+ * If $title is a CSS/JS page and not a redirect, it is added to the table.
 58+ * If it is a CSS/JS page but is a redirect, it is removed from the table.
 59+ * If it's not a CSS/JS page, it's assumed never to have been added to begin with, so nothing happens/
 60+ * @param $title Title object
 61+ */
 62+ public static function updatePageStatus( $title ) {
 63+ if ( $title->isCssOrJsPage() || $title->isCssJsSubpage() ) {
 64+ if ( $title->isRedirect() ) {
 65+ self::delete( $title );
 66+ } else {
 67+ self::add( $title );
 68+ }
 69+ }
 70+ }
 71+
 72+ /**
3273 * Add a title to the gadgetpagelist table
3374 * @param $title Title object
3475 */
3576 public static function add( $title ) {
3677 $dbw = wfGetDB( DB_MASTER );
37 - $dbw->insert( 'gadgetpagelist', array(
38 - 'gpl_extension' => self::determineExtension( $title ),
39 - 'gpl_namespace' => $title->getNamespace(),
40 - 'gpl_title' => $title->getDBKey()
41 - ), __METHOD__, array( 'IGNORE' )
 78+ $dbw->insert( 'gadgetpagelist', self::getRowForTitle( $title ),
 79+ __METHOD__, array( 'IGNORE' )
4280 );
4381 }
4482

Sign-offs

UserFlagDate
Krinkleinspected15:11, 3 September 2011

Comments

#Comment by Krinkle (talk | contribs)   15:11, 3 September 2011

isGadgetPage is a bit confusing, as it doesn't mean a page in NS_GADGET. Perhaps a different name ? Something more generic like resource wiki pages thingies ? (since Gadget modules can only contain scripts/styles from NS_GADGET)

#Comment by Catrope (talk | contribs)   18:35, 12 September 2011

It's meant to be in line with the "gadgetpagelist" table name. It's not too late to change that, though, so if you have an idea for a better name I'm happy to hear it :)

Status & tagging log