r85951 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r85950‎ | r85951 | r85952 >
Date:14:30, 13 April 2011
Author:happy-melon
Status:resolved (Comments)
Tags:
Comment:
Implement user-is-blocked and wiki-is-read-only as exceptions.
Modified paths:
  • /trunk/phase3/includes/AutoLoader.php (modified) (history)
  • /trunk/phase3/includes/Exception.php (modified) (history)
  • /trunk/phase3/includes/OutputPage.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/OutputPage.php
@@ -1933,53 +1933,10 @@
19341934
19351935 /**
19361936 * Produce a "user is blocked" page.
1937 - *
1938 - * @param $return Boolean: whether to have a "return to $wgTitle" message or not.
1939 - * @return nothing
 1937+ * @deprecated since 1.18
19401938 */
1941 - function blockedPage( $return = true ) {
1942 - global $wgContLang;
1943 -
1944 - $this->setPageTitle( wfMsg( 'blockedtitle' ) );
1945 - $this->setRobotPolicy( 'noindex,nofollow' );
1946 - $this->setArticleRelated( false );
1947 -
1948 - $name = $this->getUser()->blockedBy();
1949 - $reason = $this->getUser()->blockedFor();
1950 - if( $reason == '' ) {
1951 - $reason = wfMsg( 'blockednoreason' );
1952 - }
1953 - $blockTimestamp = $this->getContext()->getLang()->timeanddate(
1954 - wfTimestamp( TS_MW, $this->getUser()->mBlock->mTimestamp ), true
1955 - );
1956 - $ip = wfGetIP();
1957 -
1958 - $link = '[[' . $wgContLang->getNsText( NS_USER ) . ":{$name}|{$name}]]";
1959 -
1960 - $blockid = $this->getUser()->mBlock->getId();
1961 -
1962 - $blockExpiry = $this->getContext()->getLang()->formatExpiry( $this->getUser()->mBlock->mExpiry );
1963 -
1964 - if ( $this->getUser()->mBlock->mAuto ) {
1965 - $msg = 'autoblockedtext';
1966 - } else {
1967 - $msg = 'blockedtext';
1968 - }
1969 -
1970 - /* $ip returns who *is* being blocked, $intended contains who was meant to be blocked.
1971 - * This could be a username, an IP range, or a single IP. */
1972 - $intended = $this->getUser()->mBlock->getTarget();
1973 -
1974 - $this->addWikiMsg(
1975 - $msg, $link, $reason, $ip, $name, $blockid, $blockExpiry,
1976 - $intended, $blockTimestamp
1977 - );
1978 -
1979 - # Don't auto-return to special pages
1980 - if( $return ) {
1981 - $return = $this->getTitle()->getNamespace() > -1 ? $this->getTitle() : null;
1982 - $this->returnToMain( null, $return );
1983 - }
 1939+ function blockedPage() {
 1940+ throw new UserBlockedError( $this->getUser()->mBlock );
19841941 }
19851942
19861943 /**
@@ -2163,9 +2120,7 @@
21642121 $this->addWikiText( $this->formatPermissionsErrorMessage( $reasons, $action ) );
21652122 } else {
21662123 // Wiki is read only
2167 - $this->setPageTitle( wfMsg( 'readonly' ) );
2168 - $reason = wfReadOnlyReason();
2169 - $this->wrapWikiMsg( "<div class='mw-readonly-error'>\n$1\n</div>", array( 'readonlytext', $reason ) );
 2124+ throw new ReadOnlyError;
21702125 }
21712126
21722127 // Show source, if supplied
Index: trunk/phase3/includes/AutoLoader.php
@@ -196,6 +196,7 @@
197197 'RawPage' => 'includes/RawPage.php',
198198 'RCCacheEntry' => 'includes/ChangesList.php',
199199 'RdfMetaData' => 'includes/Metadata.php',
 200+ 'ReadOnlyError' => 'includes/Exception.php',
200201 'RecentChange' => 'includes/RecentChange.php',
201202 'RegexlikeReplacer' => 'includes/StringUtils.php',
202203 'ReplacementArray' => 'includes/StringUtils.php',
@@ -249,6 +250,7 @@
250251 'User' => 'includes/User.php',
251252 'UserArray' => 'includes/UserArray.php',
252253 'UserArrayFromResult' => 'includes/UserArray.php',
 254+ 'UserBlockedError' => 'includes/Exception.php',
253255 'UserMailer' => 'includes/UserMailer.php',
254256 'UserRightsProxy' => 'includes/UserRightsProxy.php',
255257 'ViewCountUpdate' => 'includes/ViewCountUpdate.php',
Index: trunk/phase3/includes/Exception.php
@@ -371,6 +371,56 @@
372372 }
373373
374374 /**
 375+ * Show an error when the wiki is locked/read-only and the user tries to do
 376+ * something that requires write access
 377+ */
 378+class ReadOnlyError extends ErrorPageError {
 379+ public function __construct(){
 380+ parent::__construct(
 381+ 'readonly',
 382+ 'readonlytext',
 383+ wfReadOnlyReason()
 384+ );
 385+ }
 386+}
 387+
 388+/**
 389+ * Show an error when the user tries to do something whilst blocked
 390+ */
 391+class UserBlockedError extends ErrorPageError {
 392+ public function __construct( Block $block ){
 393+ global $wgLang;
 394+
 395+ $blockerUserpage = $block->getBlocker()->getUserPage();
 396+ $link = "[[{$blockerUserpage->getPrefixedText()}|{$blockerUserpage->getText()}]]";
 397+
 398+ $reason = $block->mReason;
 399+ if( $reason == '' ) {
 400+ $reason = wfMsg( 'blockednoreason' );
 401+ }
 402+
 403+ /* $ip returns who *is* being blocked, $intended contains who was meant to be blocked.
 404+ * This could be a username, an IP range, or a single IP. */
 405+ $intended = $block->getTarget();
 406+
 407+ parent::__construct(
 408+ 'blockedtitle',
 409+ $block->mAuto ? 'autoblocketext' : 'blockedtext',
 410+ array(
 411+ $link,
 412+ $reason,
 413+ wfGetIP(),
 414+ $block->getBlocker()->getName(),
 415+ $block->getId(),
 416+ $wgLang->formatExpiry( $block->mExpiry ),
 417+ $intended,
 418+ $wgLang->timeanddate( wfTimestamp( TS_MW, $block->mTimestamp ), true )
 419+ )
 420+ );
 421+ }
 422+}
 423+
 424+/**
375425 * Install an exception handler for MediaWiki exception types.
376426 */
377427 function wfInstallExceptionHandler() {

Follow-up revisions

RevisionCommit summaryAuthorDate
r93237Follow-up r93234: use User::getBlock() accessor rather than accessing $mBlock...happy-melon19:58, 26 July 2011
r93303Typo in message autoblockedtext...hashar19:46, 27 July 2011

Comments

#Comment by Brion VIBBER (talk | contribs)   22:55, 21 June 2011

Accessing user->$mBlock from the outside is a code smell; while it's pretty likely that it's already been filled out before we get to OutputPage::blockedPage(), it's meant to be lazy-initialized and should really be marked either private or protected.

Either User needs to be extended with an accessor that returns the actual Block object and that should be used, or the multiple pieces of code calling '$block = $user->mBlock; throw new UserBlockedError( $block )' should be changed to pass in the user (and the exception changed to take the user), or the exception should always operate on the global $wgUser -- because who else could it be outputting to?

#Comment by Happy-melon (talk | contribs)   23:46, 26 June 2011

All of User.php smells, the bits associated with blocking particularly so... :D

#Comment by Catrope (talk | contribs)   10:00, 25 August 2011

$user->mBlock was already being accessed by the same function under the same circumstances. This revision does not change this at all. You're right that it sucks and should be rewritten to use an accessor (which was attempted in r93237 and reverted due to breaking tests) but it's unrelated to the merits of this revision.

Marking resolved because this is fine other than that UserBlockedError makes Chad feel icky, Brion raised a good but unrelated point and there was a typo in a message key that was fixed in r93303.

#Comment by Aaron Schulz (talk | contribs)   23:13, 21 June 2011

What is the advantage of making these exceptions btw?

#Comment by Happy-melon (talk | contribs)   23:47, 26 June 2011

One step closer to the global-less code nirvana ;)

#Comment by 😂 (talk | contribs)   00:58, 21 July 2011

Suggest reverting UserBlockedError for now per Brion's comments and the fact that it makes me feel icky. I do kind of like ReadOnlyError as an exception though.

#Comment by Happy-melon (talk | contribs)   20:00, 26 July 2011

I implemented an accessor for $user->mBlock in r93237. The change I made to Title.php is a good demonstration of the value of having it as an exception: just because the object subclasses Exception doesn't mean it has to always be thrown ;)

Status & tagging log