r91159 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r91158‎ | r91159 | r91160 >
Date:07:05, 30 June 2011
Author:aaron
Status:resolved (Comments)
Tags:
Comment:
Expand on r91123 Article refactoring:
* Added $user param to relevant WikiPage functions (with b/c)
* Cleaned up editUpdates() signature and tweaked three other WikiPage functions signatures (with b/c)
* Added fixme to prepareTextForEdit()
Modified paths:
  • /trunk/phase3/includes/WikiPage.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/WikiPage.php
——@@ -10,8 +10,6 @@
1111 * Some fields are public only for backwards-compatibility. Use accessors.
1212 * In the past, this class was part of Article.php and everything was public.
1313 *
14 - * @TODO: dependency inject $wgUser as an argument to functions
15 - *
1614 * @internal documentation reviewed 15 Mar 2010
1715 */
1816 class WikiPage extends Page {
——@@ -48,6 +46,9 @@
4947
5048 /**
5149 * Constructor from a page id
 50+ *
 51+ * Always override this for all subclasses (until we use PHP with LSB)
 52+ *
5253 * @param $id Int article ID to load
5354 */
5455 public static function newFromID( $id ) {
——@@ -643,13 +644,14 @@
644645 /**
645646 * Should the parser cache be used?
646647 *
 648+ * @param $user User The relevant user
647649 * @return boolean
648650 */
649 - public function useParserCache( $oldid ) {
650 - global $wgUser, $wgEnableParserCache;
 651+ public function isParserCacheUsed( User $user, $oldid ) {
 652+ global $wgEnableParserCache;
651653
652654 return $wgEnableParserCache
653 - && $wgUser->getStubThreshold() == 0
 655+ && $user->getStubThreshold() == 0
654656 && $this->exists()
655657 && empty( $oldid )
656658 && !$this->mTitle->isCssOrJsPage()
——@@ -923,8 +925,6 @@
924926 * Change an existing article or create a new article. Updates RC and all necessary caches,
925927 * optionally via the deferred update array.
926928 *
927 - * $wgUser must be set before calling this function.
928 - *
929929 * @param $text String: new text
930930 * @param $summary String: edit summary
931931 * @param $flags Integer bitfield:
——@@ -950,7 +950,7 @@
951951 * auto-detection due to MediaWiki's performance-optimised locking strategy.
952952 *
953953 * @param $baseRevId the revision ID this edit was based off, if any
954 - * @param $user User (optional), $wgUser will be used if not passed
 954+ * @param $user User the user doing the edit
955955 *
956956 * @return Status object. Possible errors:
957957 * edit-hook-aborted: The ArticleSave hook aborted the edit but didn't set the fatal flag of $status
——@@ -1119,7 +1119,7 @@
11201120 # as a template. Partly deferred.
11211121 self::onArticleEdit( $this->mTitle );
11221122 # Update links tables, site stats, etc.
1123 - $this->editUpdates( $text, $summary, $isminor, $now, $revisionId, $changed, $user );
 1123+ $this->doEditUpdates( $text, $user, $summary, $isminor, $revisionId, $changed );
11241124 } else {
11251125 # Create new article
11261126 $status->value['new'] = true;
——@@ -1180,7 +1180,7 @@
11811181 $dbw->commit();
11821182
11831183 # Update links, etc.
1184 - $this->editUpdates( $text, $summary, $isminor, $now, $revisionId, true, $user, true );
 1184+ $this->doEditUpdates( $text, $user, $summary, $isminor, $revisionId, true, true );
11851185
11861186 # Clear caches
11871187 self::onArticleCreate( $this->mTitle );
——@@ -1214,10 +1214,14 @@
12151215 * @param $reason String
12161216 * @param &$cascade Integer. Set to false if cascading protection isn't allowed.
12171217 * @param $expiry Array: per restriction type expiration
 1218+ * @param $user User The user updating the restrictions
12181219 * @return bool true on success
12191220 */
1220 - public function updateRestrictions( $limit = array(), $reason = '', &$cascade = 0, $expiry = array() ) {
 1221+ public function updateRestrictions(
 1222+ $limit = array(), $reason = '', &$cascade = 0, $expiry = array(), User $user = null
 1223+ ) {
12211224 global $wgUser, $wgContLang;
 1225+ $user = is_null( $user ) ? $wgUser : $user;
12221226
12231227 $restrictionTypes = $this->mTitle->getRestrictionTypes();
12241228
——@@ -1276,7 +1280,7 @@
12771281
12781282 # If nothing's changed, do nothing
12791283 if ( $changed ) {
1280 - if ( wfRunHooks( 'ArticleProtect', array( &$this, &$wgUser, $limit, $reason ) ) ) {
 1284+ if ( wfRunHooks( 'ArticleProtect', array( &$this, &$user, $limit, $reason ) ) ) {
12811285 $dbw = wfGetDB( DB_MASTER );
12821286
12831287 # Prepare a null revision to be added to the history
——@@ -1375,8 +1379,8 @@
13761380 ), __METHOD__
13771381 );
13781382
1379 - wfRunHooks( 'NewRevisionFromEditComplete', array( $this, $nullRevision, $latest, $wgUser ) );
1380 - wfRunHooks( 'ArticleProtectComplete', array( &$this, &$wgUser, $limit, $reason ) );
 1383+ wfRunHooks( 'NewRevisionFromEditComplete', array( $this, $nullRevision, $latest, $user ) );
 1384+ wfRunHooks( 'ArticleProtectComplete', array( &$this, &$user, $limit, $reason ) );
13811385
13821386 # Update the protection log
13831387 $log = new LogPage( 'protect' );
——@@ -1507,15 +1511,19 @@
15081512 * Revision::DELETED_RESTRICTED
15091513 * @param $id int article ID
15101514 * @param $commit boolean defaults to true, triggers transaction end
 1515+ * @param &$errors Array of errors to append to
 1516+ * @param $user User The relevant user
15111517 * @return boolean true if successful
15121518 */
1513 - public function doDeleteArticle( $reason, $suppress = false, $id = 0, $commit = true, &$error = '' ) {
1514 - global $wgDeferredUpdateList, $wgUseTrackbacks;
1515 - global $wgUser;
 1519+ public function doDeleteArticle(
 1520+ $reason, $suppress = false, $id = 0, $commit = true, &$error = '', User $user = null
 1521+ ) {
 1522+ global $wgDeferredUpdateList, $wgUseTrackbacks, $wgUser;
 1523+ $user = is_null( $user ) ? $wgUser : $user;
15161524
15171525 wfDebug( __METHOD__ . "\n" );
15181526
1519 - if ( ! wfRunHooks( 'ArticleDelete', array( &$this, &$wgUser, &$reason, &$error ) ) ) {
 1527+ if ( ! wfRunHooks( 'ArticleDelete', array( &$this, &$user, &$reason, &$error ) ) ) {
15201528 return false;
15211529 }
15221530 $dbw = wfGetDB( DB_MASTER );
——@@ -1644,7 +1652,7 @@
16451653 $dbw->commit();
16461654 }
16471655
1648 - wfRunHooks( 'ArticleDeleteComplete', array( &$this, &$wgUser, $reason, $id ) );
 1656+ wfRunHooks( 'ArticleDeleteComplete', array( &$this, &$user, $reason, $id ) );
16491657 return true;
16501658 }
16511659
——@@ -1652,7 +1660,7 @@
16531661 * Roll back the most recent consecutive set of edits to a page
16541662 * from the same user; fails if there are no eligible edits to
16551663 * roll back to, e.g. user is the sole contributor. This function
1656 - * performs permissions checks on $wgUser, then calls commitRollback()
 1664+ * performs permissions checks on $user, then calls commitRollback()
16571665 * to do the dirty work
16581666 *
16591667 * @param $fromP String: Name of the user whose edits to rollback.
——@@ -1664,26 +1672,30 @@
16651673 * 'alreadyrolled' : 'current' (rev)
16661674 * success : 'summary' (str), 'current' (rev), 'target' (rev)
16671675 *
 1676+ * @param $user User The user performing the rollback
16681677 * @return array of errors, each error formatted as
16691678 * array(messagekey, param1, param2, ...).
16701679 * On success, the array is empty. This array can also be passed to
16711680 * OutputPage::showPermissionsErrorPage().
16721681 */
1673 - public function doRollback( $fromP, $summary, $token, $bot, &$resultDetails ) {
 1682+ public function doRollback(
 1683+ $fromP, $summary, $token, $bot, &$resultDetails, User $user = null
 1684+ ) {
16741685 global $wgUser;
 1686+ $user = is_null( $user ) ? $wgUser : $user;
16751687
16761688 $resultDetails = null;
16771689
16781690 # Check permissions
1679 - $editErrors = $this->mTitle->getUserPermissionsErrors( 'edit', $wgUser );
1680 - $rollbackErrors = $this->mTitle->getUserPermissionsErrors( 'rollback', $wgUser );
 1691+ $editErrors = $this->mTitle->getUserPermissionsErrors( 'edit', $user );
 1692+ $rollbackErrors = $this->mTitle->getUserPermissionsErrors( 'rollback', $user );
16811693 $errors = array_merge( $editErrors, wfArrayDiff2( $rollbackErrors, $editErrors ) );
16821694
1683 - if ( !$wgUser->matchEditToken( $token, array( $this->mTitle->getPrefixedText(), $fromP ) ) ) {
 1695+ if ( !$user->matchEditToken( $token, array( $this->mTitle->getPrefixedText(), $fromP ) ) ) {
16841696 $errors[] = array( 'sessionfailure' );
16851697 }
16861698
1687 - if ( $wgUser->pingLimiter( 'rollback' ) || $wgUser->pingLimiter() ) {
 1699+ if ( $user->pingLimiter( 'rollback' ) || $user->pingLimiter() ) {
16881700 $errors[] = array( 'actionthrottledtext' );
16891701 }
16901702
——@@ -1692,7 +1704,7 @@
16931705 return $errors;
16941706 }
16951707
1696 - return $this->commitRollback( $fromP, $summary, $bot, $resultDetails );
 1708+ return $this->commitRollback( $user, $fromP, $summary, $bot, $resultDetails );
16971709 }
16981710
16991711 /**
——@@ -1703,9 +1715,16 @@
17041716 * rollback to the DB Therefore, you should only call this function direct-
17051717 * ly if you want to use custom permissions checks. If you don't, use
17061718 * doRollback() instead.
 1719+ * @param $fromP String: Name of the user whose edits to rollback.
 1720+ * @param $summary String: Custom summary. Set to default summary if empty.
 1721+ * @param $bot Boolean: If true, mark all reverted edits as bot.
 1722+ *
 1723+ * @param $resultDetails Array: contains result-specific array of additional values
 1724+ * @param $guser User The user performing the rollback
17071725 */
1708 - public function commitRollback( $fromP, $summary, $bot, &$resultDetails ) {
 1726+ public function commitRollback( $fromP, $summary, $bot, &$resultDetails, User $guser = null ) {
17091727 global $wgUseRCPatrol, $wgUser, $wgContLang;
 1728+ $guser = is_null( $guser ) ? $wgUser : $guser;
17101729
17111730 $dbw = wfGetDB( DB_MASTER );
17121731
——@@ -1753,7 +1772,7 @@
17541773 }
17551774
17561775 $set = array();
1757 - if ( $bot && $wgUser->isAllowed( 'markbotedits' ) ) {
 1776+ if ( $bot && $guser->isAllowed( 'markbotedits' ) ) {
17581777 # Mark all reverted edits as bot
17591778 $set['rc_bot'] = 1;
17601779 }
——@@ -1794,11 +1813,11 @@
17951814 # Save
17961815 $flags = EDIT_UPDATE;
17971816
1798 - if ( $wgUser->isAllowed( 'minoredit' ) ) {
 1817+ if ( $guser->isAllowed( 'minoredit' ) ) {
17991818 $flags |= EDIT_MINOR;
18001819 }
18011820
1802 - if ( $bot && ( $wgUser->isAllowedAny( 'markbotedits', 'bot' ) ) ) {
 1821+ if ( $bot && ( $guser->isAllowedAny( 'markbotedits', 'bot' ) ) ) {
18031822 $flags |= EDIT_FORCE_BOT;
18041823 }
18051824
——@@ -1810,7 +1829,7 @@
18111830 $revId = false;
18121831 }
18131832
1814 - wfRunHooks( 'ArticleRollbackComplete', array( $this, $wgUser, $target, $current ) );
 1833+ wfRunHooks( 'ArticleRollbackComplete', array( $this, $guser, $target, $current ) );
18151834
18161835 $resultDetails = array(
18171836 'summary' => $summary,
——@@ -1824,21 +1843,22 @@
18251844
18261845 /**
18271846 * Do standard deferred updates after page view
 1847+ * @param $user User The relevant user
18281848 */
1829 - public function viewUpdates() {
1830 - global $wgDeferredUpdateList, $wgDisableCounters, $wgUser;
 1849+ public function doViewUpdates( User $user ) {
 1850+ global $wgDeferredUpdateList, $wgDisableCounters;
18311851 if ( wfReadOnly() ) {
18321852 return;
18331853 }
18341854
18351855 # Don't update page view counters on views from bot users (bug 14044)
1836 - if ( !$wgDisableCounters && !$wgUser->isAllowed( 'bot' ) && $this->getID() ) {
 1856+ if ( !$wgDisableCounters && !$user->isAllowed( 'bot' ) && $this->getID() ) {
18371857 $wgDeferredUpdateList[] = new ViewCountUpdate( $this->getID() );
18381858 $wgDeferredUpdateList[] = new SiteStatsUpdate( 1, 0, 0 );
18391859 }
18401860
18411861 # Update newtalk / watchlist notification status
1842 - $wgUser->clearNotification( $this->mTitle );
 1862+ $user->clearNotification( $this->mTitle );
18431863 }
18441864
18451865 /**
——@@ -1846,17 +1866,17 @@
18471867 * Returns a stdclass with source, pst and output members
18481868 */
18491869 public function prepareTextForEdit( $text, $revid = null, User $user = null ) {
1850 - if ( $this->mPreparedEdit && $this->mPreparedEdit->newText == $text && $this->mPreparedEdit->revid == $revid ) {
 1870+ global $wgParser, $wgUser;
 1871+ $user = is_null( $user ) ? $wgUser : $user;
 1872+ // @TODO fixme: check $user->getId() here???
 1873+ if ( $this->mPreparedEdit
 1874+ && $this->mPreparedEdit->newText == $text
 1875+ && $this->mPreparedEdit->revid == $revid
 1876+ ) {
18511877 // Already prepared
18521878 return $this->mPreparedEdit;
18531879 }
18541880
1855 - global $wgParser;
1856 -
1857 - if( $user === null ) {
1858 - global $wgUser;
1859 - $user = $wgUser;
1860 - }
18611881 $popts = ParserOptions::newFromUser( $user );
18621882 wfRunHooks( 'ArticlePrepareTextForEdit', array( $this, $popts ) );
18631883
——@@ -1881,18 +1901,17 @@
18821902 *
18831903 * @private
18841904 * @param $text String: New text of the article
 1905+ * @param $user User object: User doing the edit
18851906 * @param $summary String: Edit summary
18861907 * @param $minoredit Boolean: Minor edit
1887 - * @param $timestamp_of_pagechange String timestamp associated with the page change
18881908 * @param $newid Integer: rev_id value of the new revision
18891909 * @param $changed Boolean: Whether or not the content actually changed
1890 - * @param $user User object: User doing the edit
18911910 * @param $created Boolean: Whether the edit created the page
18921911 */
1893 - public function editUpdates( $text, $summary, $minoredit, $timestamp_of_pagechange, $newid,
1894 - $changed = true, User $user = null, $created = false )
1895 - {
1896 - global $wgDeferredUpdateList, $wgUser, $wgEnableParserCache;
 1912+ public function doEditUpdates(
 1913+ $text, $user, $summary, $minoredit, $newid, $changed = true, $created = false
 1914+ ) {
 1915+ global $wgDeferredUpdateList, $wgEnableParserCache;
18971916
18981917 wfProfileIn( __METHOD__ );
18991918
——@@ -1961,8 +1980,8 @@
19621981 # Don't do this if $changed = false otherwise some idiot can null-edit a
19631982 # load of user talk pages and piss people off, nor if it's a minor edit
19641983 # by a properly-flagged bot.
1965 - if ( $this->mTitle->getNamespace() == NS_USER_TALK && $shortTitle != $wgUser->getTitleKey() && $changed
1966 - && !( $minoredit && $wgUser->isAllowed( 'nominornewtalk' ) )
 1984+ if ( $this->mTitle->getNamespace() == NS_USER_TALK && $shortTitle != $user->getTitleKey() && $changed
 1985+ && !( $minoredit && $user->isAllowed( 'nominornewtalk' ) )
19671986 ) {
19681987 if ( wfRunHooks( 'ArticleEditUpdateNewTalk', array( &$this ) ) ) {
19691988 $other = User::newFromName( $shortTitle, false );
——@@ -1992,12 +2011,14 @@
19932012 * @param $rev Revision object
19942013 *
19952014 * @todo This is a shitty interface function. Kill it and replace the
1996 - * other shitty functions like editUpdates and such so it's not needed
 2015+ * other shitty functions like doEditUpdates and such so it's not needed
19972016 * anymore.
 2017+ * @deprecated since 1.19, use doEditUpdates()
19982018 */
19992019 public function createUpdates( $rev ) {
2000 - $this->editUpdates( $rev->getText(), $rev->getComment(),
2001 - $rev->isMinor(), wfTimestamp(), $rev->getId(), true, null, true );
 2020+ global $wgUser;
 2021+ $this->doEditUpdates( $rev->getText(), $wgUser, $rev->getComment(),
 2022+ $rev->isMinor(), $rev->getId(), true, true );
20022023 }
20032024
20042025 /**
——@@ -2005,21 +2026,16 @@
20062027 * so we can do things like signatures and links-in-context.
20072028 *
20082029 * @param $text String article contents
2009 - * @param $user User object: user doing the edit, $wgUser will be used if
2010 - * null is given
 2030+ * @param $user User object: user doing the edit
20112031 * @param $popts ParserOptions object: parser options, default options for
20122032 * the user loaded if null given
20132033 * @return string article contents with altered wikitext markup (signatures
20142034 * converted, {{subst:}}, templates, etc.)
20152035 */
20162036 public function preSaveTransform( $text, User $user = null, ParserOptions $popts = null ) {
2017 - global $wgParser;
 2037+ global $wgParser, $wgUser;
 2038+ $user = is_null( $user ) ? $wgUser : $user;
20182039
2019 - if ( $user === null ) {
2020 - global $wgUser;
2021 - $user = $wgUser;
2022 - }
2023 -
20242040 if ( $popts === null ) {
20252041 $popts = ParserOptions::newFromUser( $user );
20262042 }
——@@ -2069,10 +2085,11 @@
20702086 * are not updated, caches are not flushed.
20712087 *
20722088 * @param $text String: text submitted
 2089+ * @param $user User The relevant user
20732090 * @param $comment String: comment submitted
20742091 * @param $minor Boolean: whereas it's a minor modification
20752092 */
2076 - public function quickEdit( $text, $comment = '', $minor = 0 ) {
 2093+ public function doQuickEdit( $text, User $user, $comment = '', $minor = 0 ) {
20772094 wfProfileIn( __METHOD__ );
20782095
20792096 $dbw = wfGetDB( DB_MASTER );
——@@ -2081,12 +2098,11 @@
20822099 'text' => $text,
20832100 'comment' => $comment,
20842101 'minor_edit' => $minor ? 1 : 0,
2085 - ) );
 2102+ ) );
20862103 $revision->insertOn( $dbw );
20872104 $this->updateRevisionOn( $dbw, $revision );
20882105
2089 - global $wgUser;
2090 - wfRunHooks( 'NewRevisionFromEditComplete', array( $this, $revision, false, $wgUser ) );
 2106+ wfRunHooks( 'NewRevisionFromEditComplete', array( $this, $revision, false, $user ) );
20912107
20922108 wfProfileOut( __METHOD__ );
20932109 }
——@@ -2400,20 +2416,22 @@
24012417 * @since 1.16 (r52326) for LiquidThreads
24022418 *
24032419 * @param $oldid mixed integer Revision ID or null
 2420+ * @param $user User The relevant user
24042421 * @return ParserOutput or false if the given revsion ID is not found
24052422 */
2406 - public function getParserOutput( $oldid = null ) {
 2423+ public function getParserOutput( $oldid = null, User $user = null ) {
24072424 global $wgEnableParserCache, $wgUser;
 2425+ $user = is_null( $user ) ? $wgUser : $user;
24082426
24092427 // Should the parser cache be used?
24102428 $useParserCache = $wgEnableParserCache &&
2411 - $wgUser->getStubThreshold() == 0 &&
 2429+ $user->getStubThreshold() == 0 &&
24122430 $this->exists() &&
24132431 $oldid === null;
24142432
24152433 wfDebug( __METHOD__ . ': using parser cache: ' . ( $useParserCache ? 'yes' : 'no' ) . "\n" );
24162434
2417 - if ( $wgUser->getStubThreshold() ) {
 2435+ if ( $user->getStubThreshold() ) {
24182436 wfIncrStats( 'pcache_miss_stub' );
24192437 }
24202438
——@@ -2437,4 +2455,39 @@
24382456
24392457 return $this->getOutputFromWikitext( $text, $useParserCache );
24402458 }
 2459+
 2460+ /*
 2461+ * @deprecated since 1.19
 2462+ */
 2463+ public function quickEdit( $text, $comment = '', $minor = 0 ) {
 2464+ global $wgUser;
 2465+ return $this->doQuickEdit( $text, $wgUser, $comment, $minor );
 2466+ }
 2467+
 2468+ /*
 2469+ * @deprecated since 1.19
 2470+ */
 2471+ public function editUpdates(
 2472+ $text, $summary, $minoredit, $timestamp_of_pagechange, $newid,
 2473+ $changed = true, User $user = null, $created = false
 2474+ ) {
 2475+ global $wgUser;
 2476+ return $this->doEditUpdates( $text, $wgUser, $summary, $minoredit, $newid, $changed, $created );
 2477+ }
 2478+
 2479+ /*
 2480+ * @deprecated since 1.19
 2481+ */
 2482+ public function viewUpdates() {
 2483+ global $wgUser;
 2484+ return $this->doViewUpdates( $wgUser );
 2485+ }
 2486+
 2487+ /*
 2488+ * @deprecated since 1.19
 2489+ */
 2490+ public function useParserCache( $oldid ) {
 2491+ global $wgUser;
 2492+ return $this->isParserCacheUsed( $wgUser, $oldid );
 2493+ }
24412494 }

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r91123* Split off WikiPage class from Article, WikiFilePage class from ImagePage, a...aaron22:09, 29 June 2011

Comments

#Comment by Happy-melon (talk | contribs)   08:42, 30 June 2011

(empty post for now to join the CC list)

#Comment by Nikerabbit (talk | contribs)   12:57, 30 June 2011 
+       public function commitRollback( $fromP, $summary, $bot, &$resultDetails, User $guser = null ) {
               global $wgUseRCPatrol, $wgUser, $wgContLang;
+               $guser = is_null( $guser ) ? $wgUser : $guser;

I don't think adding automatic use of global $wgUser is good idea for new params.

#Comment by Nikerabbit (talk | contribs)   12:58, 30 June 2011

How stupid of me, it is needed to keep BC with callers.

#Comment by Aaron Schulz (talk | contribs)   17:06, 30 June 2011

Yeah, kind of stuck with it. I avoided mentioning the default in the docs to discourage reliance on it. Next version it should give warnings when it's not given.

#Comment by Reedy (talk | contribs)   23:40, 30 June 2011

Not sure if you exactly did it in this revision, but I think you caused bug 29662

#Comment by Aaron Schulz (talk | contribs)   00:13, 1 July 2011

Probably was r91123. Change in r91238.

#Comment by Catrope (talk | contribs)   12:30, 7 September 2011 
+	public function editUpdates(
+		$text, $summary, $minoredit, $timestamp_of_pagechange, $newid,
+		$changed = true, User $user = null, $created = false
+	) {
+		global $wgUser;
+		return $this->doEditUpdates( $text, $wgUser, $summary, $minoredit, $newid, $changed, $created );

Shouldn't this use $user if not null?

OK otherwise.

#Comment by Aaron Schulz (talk | contribs)   18:05, 7 September 2011

This function was already killed in r91180.

Status & tagging log