r96400 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r96399‎ | r96400 | r96401 >
Date:02:57, 7 September 2011
Author:tstarling
Status:reverted (Comments)
Tags:
Comment:
Fix for total extension breakage due to protocol relative URLs
Modified paths:
  • /branches/wmf/1.17wmf1/extensions/OggHandler/OggHandler_body.php (modified) (history)

Diff [purge]

Index: branches/wmf/1.17wmf1/extensions/OggHandler/OggHandler_body.php
——@@ -515,8 +515,7 @@
516516 OggTransformOutput::$serial++;
517517
518518 if ( substr( $this->videoUrl, 0, 4 ) != 'http' ) {
519 - global $wgServer;
520 - $url = $wgServer . $this->videoUrl;
 519+ $url = wfExpandUrl( $this->videoUrl, PROTO_CURRENT );
521520 } else {
522521 $url = $this->videoUrl;
523522 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r96560OggHandler: Address issues with protocol-relative URLs. Live hack in r96400....catrope12:59, 8 September 2011
r972891.17wmf1 Revert r96400, MFT r96560reedy15:17, 16 September 2011

Comments

#Comment by Catrope (talk | contribs)   10:27, 7 September 2011

What breaks, exactly? I looked over all uses of $url (in trunk, admittedly) and using protocol-relative URLs seemed safe for all of them.

#Comment by P858snake (talk | contribs)   11:13, 7 September 2011

There was some discussion about it earlier today in -tech about the breakage

#Comment by Tim Starling (talk | contribs)   13:58, 7 September 2011

URLs like //commons.wikimedia.org//upload.wikimedia.org/something were produced, which obviously doesn't work. Also some reporters suspected that protocol-relative URLs will not work with Java, either this needs to be tested or full URLs need to be given.

#Comment by Catrope (talk | contribs)   14:02, 7 September 2011

Hmm, right. Is PROTO_CURRENT safe to use in this case, from a cache pollution standpoint? If so, then this rev is fine other than that the wfExpandUrl() call should really be unconditional (it doesn't touch fully-qualified URLs, and I trust wfExpandUrl()'s detection of those (using parse_url()) more than this simplistic if statement).

#Comment by Tim Starling (talk | contribs)   14:04, 7 September 2011

The HTML will be saved into the parser cache, so I suppose it is not safe to use. A possible alternative would be to supply a protocol-relative URL to JavaScript, and have JavaScript convert it to an absolute URL if it's necessary for Java or some other plugin.

#Comment by Catrope (talk | contribs)   15:38, 7 September 2011

Hmm. I'll hack up a quick patch that makes the URL saved in the parser cache protocol-relative, but expands it to the current protocol the second JavaScript gets its hands on it. Then I'll ask Michael Dale how this can be handled properly.

#Comment by Tim Starling (talk | contribs)   15:43, 7 September 2011

I'm not sure why you think Michael Dale is the relevant authority on the JavaScript side of this extension. 

$ svn ann OggPlayer.js | awk '{print $2}' | sort | uniq -c 
      4 brion
      7 dale
      3 gmaxwell
      6 j
      1 reedy
    754 tstarling
#Comment by Catrope (talk | contribs)   15:45, 7 September 2011

I stand corrected then :D

#Comment by Catrope (talk | contribs)   13:00, 8 September 2011

Should be addressed properly in r96560.

Status & tagging log