Last modified: 2012-01-09 19:00:20 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T32712, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 30712 - mw.Uri throws error for protocol-relative urls
mw.Uri throws error for protocol-relative urls
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
JavaScript (Other open bugs)
unspecified
All All
: Normal normal (vote)
: ---
Assigned To: Neil Kandalgaonkar
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-02 21:43 UTC by Krinkle
Modified: 2012-01-09 19:00 UTC (History)
4 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Krinkle 2011-09-02 21:43:17 UTC
Right now mw.Uri doesn't use wgUrlProtocols to build it's regex. This is both a good and a bad thing.

The good thing is that it isn't limited in interpretation to urls that MediaWiki accepts or finds 'safe'. The down side is that it is harder to detect if a url has a protocol that the MediaWiki instance accepts and it doesn't support protocol-relative urls.

I suggest one or more of the following:
* Change the loose/strict parse regex to accept procol-relative urls
* Add a static method: mw.Uri.expandUrl (which adds protocol (if not present) and perhaps wgServer as well if needed)
* Add prototype function that returns true/false depending on whether the protocol is in wgUrlProtocols

I think we should implement expandUrl no matter what. In addition we can either require that input to the mw.Uri constructor be an expanded url (and if users of it want to they can call expandUrl first), or we allow protocol to be empty in case the regex needs to be changed.

Since 1.18 is about protocol-relative support, I think this should be handled before release. Adding dependency to the milestone
Comment 1 Neil Kandalgaonkar 2011-09-08 00:44:05 UTC
(In reply to comment #0)

I would like to understand this issue a bit more. Where exactly would problems arise? 


> * Change the loose/strict parse regex to accept procol-relative urls

Makes sense to me


> * Add a static method: mw.Uri.expandUrl (which adds protocol (if not present)
> and perhaps wgServer as well if needed)

That makes the current URL an implicit argument. That seems wrong and could be confusing. When do you use .expandUrl() and when do you just use .toString() ?

There is no confusion in the model of the URL, which does always have a protocol and an authority (server) section. What I think you want, maybe, is some different way of outputting that URL, that suppresses the protocol and authority section.

But since this runs in the browser only, we always know the protocol and authority anyway. So why would we ever want a protocol-relative URL at that point?

So would this be okay?

    # within https://sample.com/

    var url = new mw.Uri( 'http://sample.com/foo/bar' );
    console.log( url )  // prints "http://sample.com/foo/bar"

    var PRurl = new mw.Uri( '//foo/bar' );
    console.log( PRurl )  // prints "https://sample.com/foo/bar"



> * Add prototype function that returns true/false depending on whether the
> protocol is in wgUrlProtocols

Why not do it like this?

    $.inArray( wgUrlProtocols, url.protocol )
Comment 2 Krinkle 2011-09-08 00:58:51 UTC
(In reply to comment #1)
> (In reply to comment #0)
> 
> I would like to understand this issue a bit more. Where exactly would problems
> arise? 

Michael Dale was describing an issue where he passed a url from an API (or other foreign source, perhaps element.src/.href) to new mw.Url() and it turned out to throw an exception.

> 
> > * Change the loose/strict parse regex to accept procol-relative urls
> 
> Makes sense to me

Cool. I originally thought of using wgUrlProtocols to create the regex but that may not be a good idea, as that 1) limits us to protocols mediawiki allows, which doesn't always make sense, 2) makes the lib dependant on having wgUrlProtocols, which makes it not usable for third parties or out of a mw-index.php context.

> 
> > * Add a static method: mw.Uri.expandUrl (which adds protocol (if not present)
> > and perhaps wgServer as well if needed)

Like I said "one or more of the following". Some make other suggestions redundant. The thought behind this was in case you thought PR shouldn't go into the regex:

  var loc = new mw.Uri ( mw.Uri.expand( input ) );

  Note that that might still be handy for relative _path_'s (like expanding "/w/Foo:Bar" with the server to become a complete url (which may or may not be PR )

> But since this runs in the browser only, we always know the protocol and
> authority anyway. So why would we ever want a protocol-relative URL at that
> point?
> 
> So would this be okay?
> 
>     # within https://sample.com/
> 
>     var url = new mw.Uri( 'http://sample.com/foo/bar' );
>     console.log( url )  // prints "http://sample.com/foo/bar"
> 
>     var PRurl = new mw.Uri( '//foo/bar' );
>     console.log( PRurl )  // prints "https://sample.com/foo/bar"

I'm not sure that makes sense. I can imagine some html5 funky cross-domain situations where we'd want to preserve the protocol (or absence thereof). Besides //sample.com works pretty much everywhere as is, especially in the front-end. 

> > * Add prototype function that returns true/false depending on whether the
> > protocol is in wgUrlProtocols
> 
> Why not do it like this?
> 
>     $.inArray( wgUrlProtocols, url.protocol )

Well, it'd be more like

      $.inArray( mw.config.get( 'urlProtocols' ), new mw.Uri( input ).protocol ) !== -1

But still, it doesn't need a function in mw.Uri, it's fine :)
Comment 3 Krinkle 2012-01-08 01:56:33 UTC
If I recall correctly, this was fixed by Niel. Anything left ?
Comment 4 Mark A. Hershberger 2012-01-09 19:00:07 UTC
(In reply to comment #3)
> If I recall correctly, this was fixed by Niel. Anything left ?

Closing.  If there is something left, reopen it with a list of things TBD.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links