MediaWiki r109562 - Code Review

Jump to: navigation, search
Repository:MediaWiki
Revision:r109561‎ | r109562 (on ViewVC)‎ | r109563 >
Date:19:16, 19 January 2012
Author:reedy
Status:reverted (Comments)
Tags:
Comment:
* (bug 32341) Add upload by URL domain limitation.
Modified paths:

Diff [purge]

Loading diff…

Follow-up revisions

Rev.Commit summaryAuthorDate
r109564Followup r109562...reedy20:26, 19 January 2012
r109570r109562: Register new message key for maintenance scriptraymond21:12, 19 January 2012
r109741Revert feature out of r109562, r109564, r109570...reedy17:33, 22 January 2012
r111120* (bug 32341) Add upload by URL domain limitation....reedy23:22, 9 February 2012

Comments

#Comment by Platonides (talk | contribs)   19:31, 19 January 2012

strpos() on the full url? Really? That won't really work.

Plus, I'd add a hook there, for extensions which may want to perform further checks (eg. flickr domain is allowed, and the extension additionally checks that the license is free).

#Comment by Reedy (talk | contribs)   19:38, 19 January 2012

Well, yes, if you then do something fancy like putting it in some other part, you can fool it. But for the Wikipedia use case, that won't work anyway

How should it be done? Unless we include protocols in the listed urls, and then check if it's at the start of the string? Or go to the extent of extracting the host from the given url (do we have a nice way in MW to do this? Feels like it'd be re-inventing the wheel)... And then becomes protocol irrelevant...

We currently have in WikiMap, and is similarily basic...

	/**
	 * @return string
	 * @throws MWException
	 */
	public function getHostname() {
		$prefixes = array( '[http://', http://',] '[https://' https://'] );
		foreach ( $prefixes as $prefix ) {
			if ( substr( $this->mCanonicalServer, 0, strlen( $prefix ) ) ) {
				return substr( $this->mCanonicalServer, strlen( $prefix ) );
			}
		}
		throw new MWException( "Invalid hostname for wiki {$this->mMinor}.{$this->mMajor}" );
	}

The hook is an extension to this, so isn't a major issue to be done now

#Comment by Reedy (talk | contribs)   20:22, 19 January 2012

Oh, wfParseUrl()

#Comment by Krinkle (talk | contribs)   00:56, 22 January 2012
-		$canUploadByUrl = UploadFromUrl::isEnabled() && $this->getUser()->isAllowed( 'upload_by_url' );
+		$canUploadByUrl = UploadFromUrl::isEnabled() && UploadFromUrl::isAllowed( $this->getUser() );

User::isAllowed doesn't appear to be called any more. This wasn't intentional?

#Comment by Reedy (talk | contribs)   01:19, 22 January 2012

Yup, reduces code duplication with what's in UploadFromUrl::isAllowed(), and also does the parent functions to check the user has upload rights

#Comment by Krinkle (talk | contribs)   01:58, 22 January 2012

Aha, UploadFromUrl::isAllowed() already existed before this commit and contains the call to User::isAllowed(). I confused it with the added method "isAllowedHost()", but that's not being called here. Thanks :)

#Comment by Siebrand (talk | contribs)   12:23, 22 January 2012

This is a feature that should not be added during a slush. Please revert.

#Comment by Reedy (talk | contribs)   13:43, 22 January 2012

The reason for doing so, as this is a requisite for the enabling of a long requested feature for commons, allowing upload by URL from other WMF projects.

The code is somewhat standalone, and if nothing is set, it's not going to change anything; it is also easy enough to test

I'd rather this actually got into 1.19, rather than reverting it out, hanging onto a patch for a while, putting into trunk/1.19 then merging to 1.19wmf1. If I had had the time available prior to have done this, I would have

#Comment by Siebrand (talk | contribs)   13:49, 22 January 2012

I understand, but you're increasing review workload, and we have agreed we would not do that. Please take it out. If you are allowed to add features, there is no reason to disallow or disencourage others to do it.

#Comment by Reedy (talk | contribs)   14:01, 22 January 2012

Mmm... Is it worth leaving the message in?

#Comment by Siebrand (talk | contribs)   13:50, 22 January 2012

Btw, I think the slush is already taking too long, over two weeks now. I'm looking forward to ending it and getting it over with!

#Comment by Reedy (talk | contribs)   14:00, 22 January 2012

Haha. Now that's something I agree with!

Status & tagging log

  • 17:35, 22 January 2012 Reedy (talk | contribs) changed the status of r109562 [removed: fixme added: reverted]
  • 12:23, 22 January 2012 Siebrand (talk | contribs) changed the status of r109562 [removed: new added: fixme]
  • 20:28, 19 January 2012 Reedy (talk | contribs) changed the status of r109562 [removed: fixme added: new]
  • 19:31, 19 January 2012 Platonides (talk | contribs) changed the status of r109562 [removed: new added: fixme]