User "Catrope" changed the status of MediaWiki.r92559. Old Status: deferred New Status: fixme
User "Catrope" also posted a comment on MediaWiki.r92559. Full URL: https://secure.wikimedia.org/wikipedia/mediawiki/wiki/Special:Code/MediaWiki/92559#c20747 Commit summary: Major update of stable features from development code. * For non-existing info pages (Wx/xx[x] pages), add a welcome page (either missing, existing or closed wikis) * It automatically checks the database list if available (which is the case on WMF wikis), like SiteMatrix * Pages belonging to existing wikis are not editable and show a link to the existing wiki. You will be redirected automatically if URL &testwiki or your preference is set to that test wiki. * Show a logo per-project, and possibility to set it per-wiki (if URL &testwiki or your preference is set to that test wiki) This is developed against 1.17wmf1 so everything should work on Incubator itself. Comment: <pre> +$wmincScriptDir = $wgScriptPath . '/extensions/WikimediaIncubator/'; </pre> First, you should use <code>$wgExtensionAssetsPath</code> instead of <code>$wgScriptPath</code> when constructing paths to web-accessible extension resources. <pre> + 'remoteExtPath' => $wmincScriptDir, </pre> Second, remoteExtPath expects a path that is relative to <code>$wgExtensionAssetsPath</code>, so you should use <code>'remoteExtPath' => 'WikimediaIncubator'</code>. 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. <pre> + preg_match( '/^(' . implode( '|', $wmincPseudoCategoryNSes ) .'):.+$/', $title->getText() ) ) { </pre> The entries in that array need to be escaped with <code>preg_quote( $foo, '/' )</code> before they're stuck in a regex. <pre> + preg_replace( '/ /', '_', $prefixdata['realtitle'] ) ); </pre> Can be done with <code>str_replace()</code>. <pre> + } elseif( !isset( $prefix ) || $prefix['error'] ) { </pre> The <code>isset()</code> 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. <pre> + return preg_replace('/-/', '_', $prefix['lang'] ) . </pre> Can be done with <code>str_replace()</code>. <pre> + return isset( $prefix ) ? $prefix . '/' . $msg : $msg; </pre> Don't use <code>isset( $prefix )</code> when what you really mean is <code>!is_null( $prefix )</code> or <code>$prefix !== null</code>. <pre> + $prefixForPageTitle = preg_replace('/\//', '-', strtolower( $setLogo['prefix'] ) ); </pre> Can be done with <code>str_replace()</code>. <pre> + $projectForFile = preg_replace('/ /', '-', strtolower( $project ) ); </pre> Can be done with <code>str_replace()</code>. <pre> + $wgOut->addModules( 'WikimediaIncubator.InfoPage' ); </pre> Because your CSS applies to HTML that is generated by PHP, not by JS, use <code>$wgOut->addModuleStyles()</code> to avoid a flash of unstyled content. <pre> + Html::rawElement( 'div', array( 'class' => 'wminc-infopage-title' ), + wfMsg( 'wminc-infopage-title', $this->mProjectName, $this->mLangName ) . + $aftertitle ) . </pre> There is no reason to use <code>rawElement</code> here, because the input is a message. Use <code>element()</code> so the text is escaped properly. <pre> + $created = isset( $this->mCreated ) ? $this->mCreated : ''; + $bug = isset( $this->mBug ) ? $this->mBug : ''; </pre> ??? Neither of these are referenced anywhere else. <pre> + $content = Html::rawElement( 'div', + array( 'class' => 'wminc-infopage-status' ), + wfMsg( 'wminc-infopage-status-' . $this->mSubStatus, $subdomainLink ) </pre> Again, don't use rawElement, use element so the message text is escaped properly. _______________________________________________ MediaWiki-CodeReview mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
