r92559 MediaWiki - Code Review archive

Is this register_globals vulnerability?

+$wmincClosedWikis = isset( $wgSiteMatrixClosedSites ) ? $wgSiteMatrixClosedSites : null;

That variable is always defined, why would it be undefined?

+               } elseif( !isset( $wmincExistingWikis ) || !is_array( $wmincExistingWikis ) ) {

$wgUser->getSkin()->

You can use the trick Dantman showed me. You can get rid of one global.

$linker = class_exists( 'DummyLinker' ) ? new DummyLinker : new Linker;

Could you document getSubdomain?

$params can be an array, no need to build it by appending to a string.

+$url = $mainpage->getFullURL( $params );

return as in return from the function or return some value (what?).

+# return if MediaWiki: Incubator-logo-wx/xx(x) does not exists

Please use !$file or $file === null, depending on which is appropriate.

+               if ( $file == null ) {

What is this rule for?

+#ca-nstab-main a {
+       color: black;
+}

In InfoPage.php there are some NULLs (should be lowercase) and missing whitespace. Could you also document the infopage messages (if not done already), thanks.

Most fixed in r92790.

As for the register_globals: outside of WMF it might be a vulnerability, but WMF defines $wgSiteMatrixClosedSites. Do you have a suggestion how I could fix it? Maybe define $wmincClosedWikis = false; and then configure it in Wikimedia to $wgSiteMatrixClosedSites (although I prefer to leave configuration in the extension itself :p).

The CSS rule is to make the tab black instead of red (a bit nicer).

$wmincClosedWikis set to false by default in r94434.

+$wmincScriptDir = $wgScriptPath . '/extensions/WikimediaIncubator/';

First, you should use $wgExtensionAssetsPath instead of $wgScriptPath when constructing paths to web-accessible extension resources.

+		'remoteExtPath' => $wmincScriptDir,

Second, remoteExtPath expects a path that is relative to $wgExtensionAssetsPath, so you should use 'remoteExtPath' => 'WikimediaIncubator'. Even though the setting is currently wrong, it won't break for you unless you use debug mode or use IE6/7 and use images in your CSS.

+			preg_match( '/^(' . implode( '|', $wmincPseudoCategoryNSes ) .'):.+$/', $title->getText() ) ) {

The entries in that array need to be escaped with preg_quote( $foo, '/' ) before they're stuck in a regex.

+				preg_replace( '/ /', '_', $prefixdata['realtitle'] ) );

Can be done with str_replace().

+		} elseif( !isset( $prefix ) || $prefix['error'] ) {

The isset() is useless here. Calling a function without a required (non-optional) parameter is unacceptable, period, you don't need to check for that. If you want the parameter to be optional, make it default to null and check for null instead.

+		return preg_replace('/-/', '_', $prefix['lang'] ) .

Can be done with str_replace().

+		return isset( $prefix ) ? $prefix . '/' . $msg : $msg;

Don't use isset( $prefix ) when what you really mean is !is_null( $prefix ) or $prefix !== null.

+		$prefixForPageTitle = preg_replace('/\//', '-', strtolower( $setLogo['prefix'] ) );

Can be done with str_replace().

+		$projectForFile = preg_replace('/ /', '-', strtolower( $project ) );

Can be done with str_replace().

+		$wgOut->addModules( 'WikimediaIncubator.InfoPage' );

Because your CSS applies to HTML that is generated by PHP, not by JS, use $wgOut->addModuleStyles() to avoid a flash of unstyled content.

+			Html::rawElement( 'div', array( 'class' => 'wminc-infopage-title' ),
+				wfMsg( 'wminc-infopage-title', $this->mProjectName, $this->mLangName ) .
+				$aftertitle ) .

There is no reason to use rawElement here, because the input is a message. Use element() so the text is escaped properly.

+		$created = isset( $this->mCreated ) ? $this->mCreated : '';
+		$bug = isset( $this->mBug ) ? $this->mBug : '';

??? Neither of these are referenced anywhere else.

+		$content = Html::rawElement( 'div',
+			array( 'class' => 'wminc-infopage-status' ),
+			wfMsg( 'wminc-infopage-status-' . $this->mSubStatus, $subdomainLink )

Again, don't use rawElement, use element so the message text is escaped properly.

Not introduced in this rev, but:

 		$listProjects =	implode( '', array_keys( $wmincProjects ) ); # something like: pbtqn
 		if( !preg_match( '/^W['.$listProjects.']\/[a-z-]+' .

This is a very dirty hack which assumes that $wmincProjects will always be an array of single-character strings that have no special meaning in regexes. A proper implementation would use implode( '|' , ... ) and would preg_quote() the elements before using them.

Things like preg_replace() -> str_replace() were already done in one of my recent commits. The rest should all have been addressed in r94482: As for the rawElement, that was not possible because the "a" tags would then be escaped, thus not forming the URL. Instead, I used wfMsgWikiHtml(). As for the $created and $bug, that's something intended for future use. Maybe should've left that out :) And for the addModuleStyles(): awesome! it's much better without the flash of unstyled content :-)