r53282 MediaWiki - Code Review archive

The merge seems to have messed up and overwritten some changes. At least in DefaultSettings.php and Skin.php. You've re-added KHTMLFixes.css, deleted a bunch of RELEASE-NOTES, undid recent wikibits.js changes, removed a recently-added comment from MonoBook.php, etc. Looks like you merged to the wrong revision or something?

oky fixed the above mentioned issues in r53285

...yea it does appear the merge was slightly out of sync...

• Recently added Skin.php stuff is clobbered more or less on-purpose since we re-factored the same things.
• Fixed DefaultSettings.php in r53283
• Removed KHTMLFixes.css

A couple of Skin.php changes shouldn't be clobbered, though:

• Comment for getHeadScripts()
• Moving makeGlobalVariableScript() to saner call type

Also, you seem to have made inadvertent changes to all the messages files you touched (en, lzh, tk, yue, zh_hans, zh_hant). Some of the changed messages don't look related to upload at all. And you removed a comment from MonoBook.php. I don't see any other bad changes at the moment, but it's hard to say.

inadvertent changes to all the messages files you touched (en, lzh, tk, yue, zh_hans, zh_hant). : Reverted with r53291

I see conflicts at SpecialUpload too, please check all files for conflicts.

SpecialUpload changes are pretty dramatically ... tired to keep things in sync .. but better to list any features that where lost rather than "general code conflicts"

Some examples, not an exhaustive list because I don't have time to do exhaustive review.

Code quality:

-$lnk = $sk->linkKnown(
+$lnk = $sk->makeKnownLinkObj( $file->getTitle(), , 'redirect=no' );

Bugfixes

-if( $this->mDesiredDestName !=  ) {
+if( $this->mDesiredDestName ) {
-$pageText = '== ' . wfMsgForContent ( 'filedesc' ) . " ==\n" . $comment . "\n" .
+$pageText = '== ' . wfMsg ( 'filedesc' ) . " ==\n" . $comment . "\n" .

hopefully fixed a few of theses: (r53420 & r53419)

One more fix: bug #19728

Fixed in r53300

Notice: Undefined property: SkinMonoBook::$pagecss in /usr/local/apache2/htdocs/w/skins/MonoBook.php on line 51

added some isset calls in r53320

On my local wiki at Special:Upload:

Strict Standards: Declaration of UploadForm::execute() should be compatible with that of SpecialPage::execute() in F:\xampp\htdocs\wiki2\includes\specials\SpecialUpload.php on line 1101

Strict Standards: Declaration of UploadFromUrl::initialize() should be compatible with that of UploadBase::initialize() in F:\xampp\htdocs\wiki2\includes\upload\UploadFromUrl.php on line 80

Notice: Undefined property: UploadForm::$mForReUpload in F:\xampp\htdocs\wiki2\includes\specials\SpecialUpload.php on line 757

Notice: Undefined property: UploadForm::$mForReUpload in F:\xampp\htdocs\wiki2\includes\specials\SpecialUpload.php on line 774

Notice: Undefined property: UploadForm::$mForReUpload in F:\xampp\htdocs\wiki2\includes\specials\SpecialUpload.php on line 822

Notice: Undefined property: UploadForm::$mForReUpload in F:\xampp\htdocs\wiki2\includes\specials\SpecialUpload.php on line 859

Notice: Undefined property: UploadForm::$mForReUpload in F:\xampp\htdocs\wiki2\includes\specials\SpecialUpload.php on line 892

Notice: Undefined property: UploadForm::$mForReUpload in F:\xampp\htdocs\wiki2\includes\specials\SpecialUpload.php on line 918

Notice: Undefined property: UploadForm::$mForReUpload in F:\xampp\htdocs\wiki2\includes\specials\SpecialUpload.php on line 974

Notice: Undefined property: SkinMonoBook::$pagecss in F:\xampp\htdocs\wiki2\skins\MonoBook.php on line 51

fixed in r53341

* $wgDebugJavaScript = false; .. .this variable lets you always get "always fresh javascript". When used with the script-loader it does not minify the script-loader output.

Would this be more useful with a user right that can be given to specific group of users?

Also, shouldn't configuration variable changes be documented also in RELEASE_NOTES.

added in r53323

wgDebugJavaScript is for when you want to debug javascript locally on every page load. If your want to disable minification at run-time just add ?debug=true to the page url.

In HttpFunctions you are changing the functions in non-bc way. I believe these callers would now be broken:

extensions/CodeReview/Subversion.php:           $blob = Http::get( $target, $this->mTimeout );
extensions/HoneypotIntegration/HoneypotIntegration.class.php:           $data = Http::get( $wgHoneypotURLSource, 'default',
extensions/MWSearch/MWSearch_body.php:          $data = Http::get( $searchUrl, $wgLuceneSearchTimeout, $curlOpts );
extensions/Translate/TranslateEditAddons.php:                   $suggestions = Http::get( $url, $timeout );
extensions/SecurePoll/includes/Auth.php:                $value = Http::get( $url, 20, $curlParams );
includes/Import.php:            $data = Http::request( $method, $url );
maintenance/dumpInterwiki.inc:  $intermap = Http::get( '[http://meta.wikimedia.org/w/index.php?title=Interwiki_map&action=raw', http://meta.wikimedia.org/w/index.php?title=Interwiki_map&action=raw',] 30 );
maintenance/rebuildInterwiki.inc:       $intermap = Http::get( '[http://meta.wikimedia.org/w/index.php?title=Interwiki_map&action=raw', http://meta.wikimedia.org/w/index.php?title=Interwiki_map&action=raw',] 30 );

Yes, Importing pages fails now:

ImportStreamSource::newFromURL: opening http://en.wikipedia.org/wiki/Special:Export/Academy_Award?templates=1 HttpRequest::doCurlReqError sending request: #6 Couldn't resolve host 'POST'

restored bc for request function and updated ::get and ::post methods in r53330

Most of the new php code doesn't seem to adhere MediaWiki coding starts with regards to whitespace. Examples:

}else{
function foo( ){

Use tabs only at line-start indendation, not in middle of lines.

In SpecialUpload.php:

+$tokenInput = "\n<input type='hidden' value=\"$token\" name=\"wpEditToken\" />\n";

Why not Xml::hidden()?

+"<input type='radio' id='wpSourceTypeURL' name='wpSourceType' value='Url' " .
+"<input type='radio' id='wpSourceTypeURL' name='wpSourceType' value='url' />" .

Inconsistent casing, code duplication? Xml-functions not used.

var is php4... we can use public/private/protected.

+print "class name: $className";

Cause for the debug output mentioned in above bug?

• looks like the code got run through some MediaWiki style updates in: r53324
• updated html token output to use Xml::hidden()
• and used xml-functions for input items.

in r53333

The code duplication is because we use jquery to apply javascript actions if $wgEnableJS2system is turned on. So this changes the html output. Once we switch over to js2 permanently we can remove these fall-backs.

$token = htmlspecialchars( $wgUser->editToken() );
Xml::hidden('wpEditToken', $token)

Token is now escaped twice in both places.

In the API module:

return $this->getResult()->addValue( null, $this->getModuleName(),
                                    array( 'error' => 'invalid-session-key'
                            ));

Please use dieUsage or dieUsageMsg here instead of hacking up your own error message. Same goes for pretty much all the code in performUpload(): use dieUsage() there as well. action=edit uses result="Failure" for errors with additional machine-readable information (CAPTCHA data in that case), but that has been obsoleted by the recent addition of the $extradata parameter to dieUsage().

Please use 'enablechuncks' => false, instead of null to indicate it's a boolean parameter. Same for 'httpstatus'

if enablechunks is set to false instead of null it fails the "is_null" check in requireOnlyOneParameter in ApiBase.php

• changed type of httpstatus (since its not a required param)
• changed return $this->getResult() to use dieUsageMsg()

in r53327

Some notes:

• defining $wgExtensionMessages and use wfLoadExtensionMessages() in mwScriptLoader.php does not work since r52503. You'll need either so set it permanently in DefaultSettings.php (which would be bad for a core feature) or merge the file in langauges/messages/MessagesEn.php.
• get_headers() (HttpFunctions.php line 43) produces the follwing warning: "http:// wrapper is disabled in the server configuration by allow_url_fopen=0"

committed a solution in r53337 ... tested with LanguageSelector extension and seems to work.

some work still remains to fix setups where curl is not available

as mentioned bellow should be working in r53414

Running r53349 in a default configuration (none of the new possibilities enabled):

Special:Upload is broken.

1. Selecting a file from disk: works 2. Automatic fill out of the destination file name: broken (means: field is empty) 3. Entering a destination filename manually 4. Click: "Upload". broken. The form returns emtpy with exception of the manually entered destination filename.

yep .. looks like the fill destination name was not working when js2 was disabled and upload-by-url is off. Should be fixed in r53416

Spotted on my local wiki at Special:Log:

Notice: Undefined variable: method in F:\xampp\htdocs\wiki2\includes\HttpFunctions.php on line 376

Notice: Undefined variable: method in F:\xampp\htdocs\wiki2\includes\HttpFunctions.php on line 382

Notice: Undefined variable: timeout in F:\xampp\htdocs\wiki2\includes\HttpFunctions.php on line 384

Notice: Undefined variable: url in F:\xampp\htdocs\wiki2\includes\HttpFunctions.php on line 388

yea apologies... the non-curl path was not tested before the merge ... should be working in r53414

Running r53416 in a default configuration (none of the new possibilities enabled):

• Select a file which already is uploaded
• The warning "A file with this name exists already, please check ... " (message 'fileexist') is shown together with a thumbnail
• Change the destination filename. The warning should disappear. But it stucks.

should be fixed in r53446

Marking this resolved for now; basic functionality seems all cleaned up. :)

Regarding the internalhttp parameter: I don't like exposing internal function to the outside world. If the upload backend is written sufficiently well, any core function should not need to call ApiUpload, but call the backend directly. If this is too complicated, we should consider changes to make it less complicated.

its very convenient to call it internally since we need to generate the api output to send to the client. In other words we need to go via the API to do all the upload checks, maps all the errors to the API and output all that in a predictable way identical to normal api upload output

Recreating that would be excessively complicated and or would have to tie in at a given entry point anyway ... right now that entry point is a parameter flag (the ~normal~ way to pass things into the api afaik) ...There are more hackish ways like ~globals~ to achieve the same effect ... that would at least would have the effect of "hiding" the internalhttp parameter but be kind of messy in a different way.

As far as I see it now it is only used in a maintenance script? Can you elaborate a bit on where you intend to use it?

I think the best way is too create a class ApiUploadInternalHttpSession extends ApiUpload. The normal ApiUpload module's main method may need a bit of refactoring though, but that needs to be done sooner or later anyway.

I'll mark it as resolved for now, as it is probably a blocker and as it is only used internally it should not matter if the parameter disappears magically at some point.

its used for background http uploading so that we can send updates to the user from their session then we are done we can init the UploadFromFile handler from a session rather than from a submitted form.

We /should/ clean it up with another class as you describe. But I think this should be part of adding a "preview" parameter to the API. bug 16437 Then we should include more flexibility as to where we seed the asset info from (aside from "stash" that also is mostly driven from a form submitting. I don't think we should block on that for this version ;)

Still has thousands of lines of unreviewed code. I'm working through it and fixing the security vulnerabilities and performance regressions as I find them. Marking fixme in case Brion is ignoring his email and thinks it's OK to deploy this.

A combined diff of this revision plus r53285, r53291, r53419, r53420 and r53443 is available at http://tstarling.com/stuff/js2-combined.diff

Other revisions that probably belong to this: r55604, r55605, r55613, r55619, r55659, r55620

(that's the upload part, most of it does not cover js2)

This merge seems to have broken user JS preview on the MonoBook skin: see bug 4801.