r93137 MediaWiki - Code Review archive

This seams a little verbose, that could be done in one line instead of 7+comments.

$this->mUpload = new UploadFromStash( class_exists( 'RequestContext' ) ? $this->createContext()->user : $GLOBALS['wgUser'] );

Comments are nice.

Sure, but separating the class_exists into two whole conditional blocks and completely duplicating the $this->mUpload = new UploadFromStash(...);?

I see no reason for getting panties in a bunch over it...it looks just fine to me. TBH: this fix can probably be merge straight to 1.17wmf1 and then reverted in trunk.

I used to write a lot of Perl with deeply-nested map {} sort {} grep {} map {} @array; things. I thought it was very clever. Then, I realized my code was hard to read a year later. Lately, I just use intermediate containers since that's what's happening internally anyway. Along similar lines, I'm pretty shy about the ternary operator unless what's going on is just blindingly obvious.

I'm not too concerned about having a similar line in there twice. My usual test is that if I'm duplicating more than 2-3 lines of code, it's time to break it out. If I used the User object in two places, for example, I'd have stored it and moved the new UploadStash() outside the block.

My current style is probably influenced by my switch to a modern editor with block folding, and access to large monitors. I understand that people using other tools might be more concerned about the use of vertical space. So, if that's you, apologies.

I find this ok. But why is 1.17 compatibility code added to phase3?

I'm guessing he doesn't have commit access to 1.17wmf1, so committing it to trunk, for someone to merge was probably easier...

Add to REL1_17?

There is no request context in REL1_17, so no need to merge.

Then again, if this is not something needed for 1.17 anywhere, why is this needed? :)

I'm not quite sure I understand, but let me see if this clears it up:

This version of UploadStash is needed to make the current version of UploadWizard work, and also to fix a bug that prevented simultaneous file uploads from working. Neilk and I were working on merging it into the current release branch so it could be deployed before 1.18 is released.

This code is present because the RequestContext dependency is the only thing that prevented this code from working in 1.17, but we're actively developing it in trunk. I can't commit changes against anything but trunk or my own branch, and branching for this seemed wrong. Making a change for production in a working copy and wrapping it up in a patch (but not checking it in) also seemed wrong.

It's necessary to grab the User object in the constructor because new uses of $wgUser are apparently frowned upon in 1.18+, so I'm not accessing the global each place it's used. The UploadStash from 1.17 isn't aware of the current user at all, since it stores its state in the session.

I'm sure there's a correct way to introduce code that's only needed when back-porting a feature to production. If someone feels like explaining that part of the process to me, that'd be the best.

Reedy, you are correct! Platonides, I added the tag (still figuring these tags out), but I believe Neilk already packaged this up in a patch for deployment, along with the other UploadStash changes, and sent it over to Roan.

(and I could learn to use the "reply" button...)

The tag for requesting merge with 1.17 is "1.17" REL1_17 is the name of the branch in branches svn folder, where you could have changed it directly.

Can leave it be... I'm guessing you're going to be replacing ApiUpload in 1.17wmf1 with the trunk version of ApiUpload too?