r66438 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r66437‎ | r66438 | r66439 >
Date:20:36, 14 May 2010
Author:mah
Status:reverted (Comments)
Tags:
Comment:
Start adding support for leaving user's messages. Begin using it in UploadFromUrl.php
In the end, we should probably use a template if its around and do something sane with the subject line if not.
Modified paths:
  • /trunk/phase3/includes/Article.php (modified) (history)
  • /trunk/phase3/includes/User.php (modified) (history)
  • /trunk/phase3/includes/upload/UploadFromUrl.php (modified) (history)
  • /trunk/phase3/languages/messages/MessagesEn.php (modified) (history)
  • /trunk/phase3/maintenance/language/messages.inc (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/language/messages.inc
@@ -1265,6 +1265,9 @@
12661266 'upload-wasdeleted',
12671267 'filename-bad-prefix',
12681268 'filename-prefix-blacklist',
 1269+ 'upload-successful-msg',
 1270+ 'upload-failure-subj',
 1271+ 'upload-failure-msg',
12691272 ),
12701273 'upload-errors' => array(
12711274 'upload-proto-error',
@@ -1696,6 +1699,10 @@
16971700 'emailsenttext',
16981701 'emailuserfooter',
16991702 ),
 1703+ 'usermessage' => array(
 1704+ 'usermessage-summary',
 1705+ 'usermessage-editor',
 1706+ ),
17001707 'watchlist' => array(
17011708 'watchlist',
17021709 'mywatchlist',
@@ -3258,6 +3265,7 @@
32593266 'newuserlog' => 'Special:Log/newusers',
32603267 'listgrouprights' => 'Special:ListGroupRights',
32613268 'emailuser' => 'E-mail user',
 3269+ 'usermessage' => 'User Messenger',
32623270 'watchlist' => 'Watchlist',
32633271 'watching' => 'Displayed when you click the "watch" button and it is in the process of watching',
32643272 'enotif' => '',
Index: trunk/phase3/includes/upload/UploadFromUrl.php
@@ -143,12 +143,12 @@
144144 }
145145
146146 $status = $this->saveTempFile( $req );
147 - $this->mRemoveTempFile = true;
148 -
149 - if( !$status->isOk() ) {
 147+ if ( !$status->isGood() ) {
150148 return $status;
151149 }
152150
 151+ $this->mRemoveTempFile = true;
 152+
153153 $v = $this->verifyUpload();
154154 if( $v['status'] !== UploadBase::OK ) {
155155 return $this->convertVerifyErrorToStatus( $v['status'], $v['details'] );
@@ -162,13 +162,18 @@
163163 // This comes from ApiBase
164164 /* $watch = $this->getWatchlistValue( $this->mParams['watchlist'], $file->getTitle() ); */
165165
166 - if ( !$status->isGood() ) {
167 - return $status;
168 - }
169 -
170166 $status = $this->getLocalFile()->upload( $this->mTempPath, $this->comment,
171167 $this->comment, File::DELETE_SOURCE, $this->mFileProps, false, $wgUser );
172168
 169+ if ( $status->isGood() ) {
 170+ $url = $this->getLocalFile()->getDescriptionUrl();
 171+ $wgUser->leaveUserMessage( wfMsg( 'successfulupload' ),
 172+ wfMsg( 'upload-success-msg', $url ) );
 173+ } else {
 174+ $wgUser->leaveUserMessage( wfMsg( 'upload-failure-subj' ),
 175+ wfMsg( 'upload-failure-msg', $status->getWikiText() ) );
 176+ }
 177+
173178 return $status;
174179 }
175180 }
Index: trunk/phase3/includes/User.php
@@ -3748,4 +3748,64 @@
37493749
37503750 return $ret;
37513751 }
 3752+
 3753+ /**
 3754+ * Leave a user a message
 3755+ * @param $text String the message to leave
 3756+ * @param $summary String the summary for this change, defaults to
 3757+ * "Leave system message."
 3758+ * @param $article Article The article to update, defaults to the
 3759+ * user's talk page.
 3760+ * @param $editor User The user leaving the message, defaults to
 3761+ * "SystemMessage"
 3762+ * @param $flags Int default edit flags
 3763+ *
 3764+ * @return boolean true if it was successful
 3765+ */
 3766+ public function leaveUserMessage( $subject, $text, $signature = "",
 3767+ $summary = null, $editor = null, $flags = 0 ) {
 3768+ if ( !isset( $summary ) ) {
 3769+ $summary = wfMsgForContent( 'usermessage-summary' );
 3770+ }
 3771+
 3772+ if ( !isset( $editor ) ) {
 3773+ $editor = User::newFromName( wfMsgForContent( 'usermessage-editor' ) );
 3774+ if ( !$editor->isLoggedIn() ) {
 3775+ $editor->addToDatabase();
 3776+ }
 3777+ }
 3778+
 3779+ $article = new Article( $this->getTalkPage() );
 3780+ wfRunHooks( 'SetupUserMessageArticle',
 3781+ array( &$article, $subject, $this, $editor ) );
 3782+
 3783+ $flags = $article->checkFlags( $flags );
 3784+
 3785+ if ( $flags & EDIT_UPDATE ) {
 3786+ $text .= $article->getContent();
 3787+ }
 3788+
 3789+ $dbw = wfGetDB( DB_MASTER );
 3790+ $dbw->begin();
 3791+
 3792+ try {
 3793+ $status = $article->doEdit( $text, $summary, $flags, false, $editor );
 3794+ } catch ( DBQueryError $e ) {
 3795+ $status = Status::newFatal("DB Error");
 3796+ }
 3797+
 3798+ if ( $status->isGood() ) {
 3799+ // Set newtalk with the right user ID
 3800+ $this->setNewtalk( true );
 3801+ wfRunHooks( 'AfterUserMessage',
 3802+ array( $this, $article, $summary, $signature, $editor, $text ) );
 3803+ $dbw->commit();
 3804+ } else {
 3805+ // The article was concurrently created
 3806+ wfDebug( __METHOD__ . ": Error ".$status->getWikiText() );
 3807+ $dbw->rollback();
 3808+ }
 3809+
 3810+ return $status->isGood();
 3811+ }
37523812 }
Index: trunk/phase3/includes/Article.php
@@ -1871,6 +1871,22 @@
18721872 }
18731873
18741874 /**
 1875+ * Check flags and add EDIT_NEW or EDIT_UPDATE to them as needed.
 1876+ * @param $flags Int
 1877+ * @return Int updated $flags
 1878+ */
 1879+ function checkFlags( $flags ) {
 1880+ if ( !( $flags & EDIT_NEW ) && !( $flags & EDIT_UPDATE ) ) {
 1881+ if ( $this->mTitle->getArticleID() ) {
 1882+ $flags |= EDIT_UPDATE;
 1883+ } else {
 1884+ $flags |= EDIT_NEW;
 1885+ }
 1886+ }
 1887+
 1888+ return $flags;
 1889+ }
 1890+ /**
18751891 * Article::doEdit()
18761892 *
18771893 * Change an existing article or create a new article. Updates RC and all necessary caches,
@@ -1936,14 +1952,7 @@
19371953 # Load $this->mTitle->getArticleID() and $this->mLatest if it's not already
19381954 $this->loadPageData();
19391955
1940 - if ( !( $flags & EDIT_NEW ) && !( $flags & EDIT_UPDATE ) ) {
1941 - $aid = $this->mTitle->getArticleID();
1942 - if ( $aid ) {
1943 - $flags |= EDIT_UPDATE;
1944 - } else {
1945 - $flags |= EDIT_NEW;
1946 - }
1947 - }
 1956+ $flags = $this->checkFlags( $flags );
19481957
19491958 if ( !wfRunHooks( 'ArticleSave', array( &$this, &$user, &$text, &$summary,
19501959 $flags & EDIT_MINOR, null, null, &$flags, &$status ) ) )
Index: trunk/phase3/languages/messages/MessagesEn.php
@@ -2149,6 +2149,9 @@
21502150 MGP # Pentax
21512151 PICT # misc.
21522152 #</pre> <!-- leave this line exactly as it is -->', # only translate this message to other languages if you have to change it
 2153+'upload-successful-msg' => 'Your upload is available here: $1',
 2154+'upload-failure-subj' => 'Upload Problem',
 2155+'upload-failure-msg' => 'There was a problem with your upload:\n $1',
21532156
21542157 'upload-proto-error' => 'Incorrect protocol',
21552158 'upload-proto-error-text' => 'Remote upload requires URLs beginning with <code>http://</code> or <code>ftp://</code>.',
@@ -2602,6 +2605,10 @@
26032606 'emailsenttext' => 'Your e-mail message has been sent.',
26042607 'emailuserfooter' => 'This e-mail was sent by $1 to $2 by the "E-mail user" function at {{SITENAME}}.',
26052608
 2609+# User Message
 2610+'usermessage-summary' => 'Leave system message.',
 2611+'usermessage-editor' => 'System messenger',
 2612+
26062613 # Watchlist
26072614 'watchlist' => 'My watchlist',
26082615 'mywatchlist' => 'My watchlist',

Follow-up revisions

RevisionCommit summaryAuthorDate
r66595re: r66438 Fix doc string, add usermessage-editor to $wgReservedUsernamesmah00:14, 18 May 2010
r81612Per CR r66438 and IRC, revert User::leaveNewMessage for now. Copied the funct...btongminh22:44, 6 February 2011

Comments

#Comment by Mormegil (talk | contribs)   22:32, 17 May 2010
  1. The doccomment is wrong: $editor does not default to "SystemMessage", but to {{MediaWiki:usermessage-editor}} (which is "System messenger" by default).
  2. msg:usermessage-editor should be added to $wgReservedUsernames.
#Comment by MarkAHershberger (talk | contribs)   00:15, 18 May 2010

Thanks for the help. see r66595

#Comment by Simetrical (talk | contribs)   20:38, 9 June 2010

Why are we leaving a talk page message here, instead of just displaying a message directly? If it's asynchronous for some reason, couldn't we just give a loading page? Or am I misunderstanding? If this actually posts a talk page message for each upload by URL, that'll be a lot of messages for prolific uploaders . . .

#Comment by Bryan (talk | contribs)   13:55, 29 July 2010

Follow up in r70137. This does not address Simetrical comments though.

#Comment by Hashar (talk | contribs)   11:10, 25 October 2010

Follow up in r66450 . \n in single quoted string.

#Comment by 😂 (talk | contribs)   21:45, 6 February 2011

I really don't like this, per Aryeh's concerns above. For feedback like this, I think we should find another way thank a talkpage message to provide feedback to the user for async upload information.

From an architectural standpoint, performing edits through User seems wrong. Edits are performed through Articles, and I don't like the idea of opening up a new method through User objects. If it's public, extensions *will* use it. It also adds to the complexity with having to deal with more $wgReservedUsernames.

#Comment by Brion VIBBER (talk | contribs)   22:00, 6 February 2011

The idea of an abstract way to say "hey send a notification from the software to this user" is actually something I really like -- BUT:

  • it should not be tied to public talk pages -- in future it should send to a sensible system-to-user messaging system
  • it should not offer editing-related options such as the user to edit as, signature, summary, etc

It should definitely not let you shove it at an arbitrary page, but that seems to have been just a stray doc comment that's been removed (yay).

I'd say drop the $signature, $summary, $editor, and $flags parameters. Note that Extension:NewUserMessage (ab)uses all those parameters, as predicted.

#Comment by RobLa-WMF (talk | contribs)   22:10, 6 February 2011

Can we fix this in such a way that it can be ready in ~33 hours?

#Comment by Bryan (talk | contribs)   22:15, 6 February 2011

This does not block deployment, as it is not used unless async upload by url is enabled.

#Comment by RobLa-WMF (talk | contribs)   22:17, 6 February 2011

Thanks for pointing that out! Marking as "nodeploy" then.

#Comment by Brion VIBBER (talk | contribs)   22:19, 6 February 2011

Extension:NewUserMessage is used on Wikimedia sites and uses this function. Removing nodeploy tag.

#Comment by MarkAHershberger (talk | contribs)   15:48, 7 February 2011

Along with NewUserMessage, LQT uses (or at least, it used, have to check now) the hooks this series of changes introduces.

IIRC, this was my solution to getting LQT to work together with NewUserMessage. Prior to this, there was some ugly inter-dependent code in NewUserMessage and LQT.

#Comment by Bryan (talk | contribs)   15:55, 7 February 2011

Guess we need to check if stuff really breaks without the hooks. I don't have time to do that before deployment unfortunately.

#Comment by MarkAHershberger (talk | contribs)   18:06, 7 February 2011

Grepping extensions shows that only NewUserMessage and LQT use the hooks. It may be possible to revert to what was there before this change was introduced but it was (even more?) uglier than this.

#Comment by MarkAHershberger (talk | contribs)   17:59, 7 February 2011

r66441 is what integrates this with LQT

#Comment by Bryan (talk | contribs)   22:47, 6 February 2011

Reverted in r81612, time is out.

#Comment by Bryan (talk | contribs)   23:04, 6 February 2011

As noted on IRC, there are a lot of unreviewed revisions for NewUserMessage, which either need reversion or review in the coming few hours. I think that reverting to 1.16wmf4 for now and review the revisions the coming week is the best thing to do.

Status & tagging log