Page MenuHomePhabricator

getMobileUrl should work on protocol relative URLS
Closed, ResolvedPublic

Description

From irc:

ori jdlrobson: can you do me a favor? I'm juggling a few things at the moment. Can you file a bug about MobileContext::getMobileUrl() not working for protocol-relative (//foo.com/foo) and relative (/foo) URLs? It returns an empty string instead, which is going to blow up in our face sooner or later. (It almost did earlier.)

Basically we should add some unit tests and make it work as expected.

Details

Related Gerrit Patches:

Event Timeline

Jdlrobson raised the priority of this task from to Needs Triage.
Jdlrobson updated the task description. (Show Details)
Jdlrobson added subscribers: Jdlrobson, ori.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 30 2015, 9:39 PM
Florian claimed this task.Jul 31 2015, 3:58 PM
Florian added a subscriber: Florian.

Local URLs don't work, let me upload a change to fix that. But protocol relative URLs should work already, we have a unit test for this, too, which is working locally.

Change 228294 had a related patch set uploaded (by Florianschmidtwelzow):
Implement a way in MobileContext::getMobileUrl() to work with local URLs

https://gerrit.wikimedia.org/r/228294

This is an absolute url.... do we need to worry about relative too?
e.g. 'Article'

@ori is there any examples where relative urls are used rather than absolute?

ori added a comment.Jul 31 2015, 5:24 PM

@ori is there any examples where relative urls are used rather than absolute?

It nearly bit us here: https://gerrit.wikimedia.org/r/#/c/226642/5/wmf-config/CommonSettings.php

Adam was planning on changing the URL used by CentralNotice to record banner impressions to a relative URL ('/beacon/impression'). If we had gone ahead with that change as-is, impression tracking on mobile would have been broken, because CentralNotice calls MobileContext::singleton()->getMobileUrl( $wgCentralBannerRecorder ) to construct the endpoint URL for mobile.

Got it. Thanks for the context.

Change 228294 merged by jenkins-bot:
Implement a way in MobileContext::getMobileUrl() to work with local URLs

https://gerrit.wikimedia.org/r/228294

Jdlrobson closed this task as Resolved.Jul 31 2015, 6:47 PM
Jdlrobson moved this task from Incoming to 2017-18 Q3 on the Readers-Web-Backlog board.

Should be fixed.

Jhernandez set Security to None.