r94375 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r94374‎ | r94375 | r94376 >
Date:19:55, 12 August 2011
Author:dantman
Status:resolved (Comments)
Tags:needs-php-test 
Comment:
Touch up Title::get[Full|Local]URL. This concept of "if not an existing interwiki getFullURL calls getLocalURL" and "if external (interwiki) getLocalURL calls getFullURL" is ridiculous. In fact if mInterwiki just happens to not be '' and not exist, you create infinite recursion and php dies.
Instead of having getFullURL and getLocalURL call each other, move the interwiki workload into getLocalURL, and make getFullURL simply call getLocalURL then expand it (use the wf method in case it's already a full url) and append the fragment to it.
Modified paths:
  • /trunk/phase3/includes/Title.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/Title.php
——@@ -837,30 +837,14 @@
838838 public function getFullURL( $query = '', $variant = false ) {
839839 global $wgServer, $wgRequest;
840840
841 - if ( is_array( $query ) ) {
842 - $query = wfArrayToCGI( $query );
843 - }
844 -
845 - $interwiki = Interwiki::fetch( $this->mInterwiki );
846 - if ( !$interwiki ) {
847 - $url = $this->getLocalURL( $query, $variant );
848 -
849 - // Ugly quick hack to avoid duplicate prefixes (bug 4571 etc)
850 - // Correct fix would be to move the prepending elsewhere.
851 - if ( $wgRequest->getVal( 'action' ) != 'render' ) {
852 - $url = $wgServer . $url;
853 - }
854 - } else {
855 - $namespace = $this->getNsText();
856 - if ( $namespace != '' ) {
857 - # Can this actually happen? Interwikis shouldn't be parsed.
858 - # Yes! It can in interwiki transclusion. But... it probably shouldn't.
859 - $namespace .= ':';
860 - }
861 - $url = $interwiki->getURL( $namespace . $this->getDBkey() );
862 - $url = wfAppendQuery( $url, $query );
863 - }
864 -
 841+ # Hand off all the decisions on urls to getLocalURL
 842+ $url = $this->getLocalURL( $query, $variant );
 843+
 844+ # Expand the url to make it a full url. Note that getLocalURL has the
 845+ # potential to output full urls for a variety of reasons, so we use
 846+ # wfExpandUrl instead of simply prepending $wgServer
 847+ $url = wfExpandUrl( $url );
 848+
865849 # Finally, add the fragment.
866850 $url .= $this->getFragmentForURL();
867851
——@@ -887,15 +871,16 @@
888872 $query = wfArrayToCGI( $query );
889873 }
890874
891 - if ( $this->isExternal() ) {
892 - $url = $this->getFullURL();
893 - if ( $query ) {
894 - // This is currently only used for edit section links in the
895 - // context of interwiki transclusion. In theory we should
896 - // append the query to the end of any existing query string,
897 - // but interwiki transclusion is already broken in that case.
898 - $url .= "?$query";
 875+ $interwiki = Interwiki::fetch( $this->mInterwiki );
 876+ if ( $interwiki ) {
 877+ $namespace = $this->getNsText();
 878+ if ( $namespace != '' ) {
 879+ # Can this actually happen? Interwikis shouldn't be parsed.
 880+ # Yes! It can in interwiki transclusion. But... it probably shouldn't.
 881+ $namespace .= ':';
899882 }
 883+ $url = $interwiki->getURL( $namespace . $this->getDBkey() );
 884+ $url = wfAppendQuery( $url, $query );
900885 } else {
901886 $dbkey = wfUrlencode( $this->getPrefixedDBkey() );
902887 if ( $query == '' ) {

Follow-up revisions

RevisionCommit summaryAuthorDate
r94458Followup r94375; Use PROTO_RELATIVE so that when $wgServer is a protocol rela...dantman14:07, 14 August 2011
r95603Unused variables....platonides18:28, 27 August 2011

Comments

#Comment by Catrope (talk | contribs)   09:50, 14 August 2011 
+		$url = wfExpandUrl( $url );

Calling wfExpandUrl() is not the same as prepending $wgServer since I rewrote wfExpandUrl() recently. You will now need to use wfExpandUrl( $url, PROTO_RELATIVE ) to achieve the same effect. With the current code, a protocol-relative $wgServer will lead to getFullUrl() returning HTTP URLs for HTTP requests and HTTPS URLs for HTTPS requests, leading to cache pollution (of the parser cache if nothing else).

Looks good to me otherwise.

#Comment by Dantman (talk | contribs)   12:02, 14 August 2011

Hmmm... I don't think the cache pollution issue is quite there, but there is a different issue I thought of.

Some small notes first:

  • Have people even used protocol relative urls for $wgServer? That sounds like it would have been liable to break //www.mediawiki.org and Template:Fullurl: since we only added support for protocol relatives in content now.

Rather a url without a protocol isn't even a full url, it's a relative url.

  • Don't worry too much about the parser cache. The parser uses the Linker, which uses Title::getLinkUrl, which uses getLocalURL+getFragmentForURL for internal links. So internal links won't be full urls anyway.

However thinking about it, wfExpandUrl doesn't just expand. It unexpands and re-expands urls, so this would have the side effect of making http interwiki links https on a https wiki potentially breaking the interwiki link of the interwiki site doesn't have https, or is like Wikipedia currently is and doesn't use the same domain.

What do you think is the best option? (Implying a check to ensure we don't break interwikis, so put that aside for a moment)

  • Provide a $wg config var to decide the PROTO_ policy for full urls. (Mixed http/https wiki can use PROTO_RELATIVE to ensure relative urls, and the single proto wiki can keep PROTO_CURRENT as $wgServer currently gives them)
  • Provide a new PROTO_ option that doesn't ignore whether $wgServer contains a protocol or not and will act like PROTO_CURRENT if it contains a protocol but like PROTO_RELATIVE if $wgServer is protocol relative. Links will work the same as before but local links will potentially benefit from PROTO_CURRENT behaviour on wiki with an absolute $wgServer. (Unless we want to permit a https wiki's internal links to point to the http wiki when it has been configured with an absolute http $wgServer?)
  • Provide a new PROTO_ option or alternative wfExpandUrl that sticks to the basic behaviour of just expanding with $wgServer.
  • Just test if the url is absolute and prepend $wgServer if not already expanded (same behaviour as the prev, but hardcoded in Title instead of generalised in a function or PROTO_ option).

That aside, if we previously had a wfExpandUrl that strait up used $wgServer and may actually be used in contexts that output to the parser cache, why are we replacing it with a wfExpandUrl which by default (ie: if any extensions used it) will start outputting http(s):// instead of // even when $wgServer is a protocol relative url? That sounds like the bigger bug.

#Comment by Catrope (talk | contribs)   12:50, 14 August 2011

We are going to use protocol-relative $wgServer on Wikimedia, so we can *replace* the secure gateway with 'real' HTTPS. I've worked on improving support for protocol-relative URLs in MediaWiki over the past few weeks. Things like [{{fullurl:Foo}} Bar] would then expand to [//example.com/wiki/Foo Bar], you're right about that, so I added support for that kind of link as well.

I do worry about the parser cache because interwiki targets can be protocol-relative, so interwiki links can pollute it.

wfExpandUrl DOES NOT unexpand and re-expand. wfExpandUrl('[http://foo', http://foo',] PROTO_HTTPS) == '[http://foo' http://foo']. So we don't need to add a new PROTO_ option or any of that.

As to what the default behavior of wfExpandUrl (in terms of PROTO_*) should be, that's an interesting question.

Your comment seems to be mostly based on misconceptions as to what wfExpandUrl() does; could you reformulate it based on what it actually does?

#Comment by Dantman (talk | contribs)   14:10, 14 August 2011

Fixed in r94458; Since we determined there are problems with both PROTO_RELATIVE and PROTO_CURRENT as a default for wfExpandUrl in various places and that it would be best if the PROTO arg was required I applied PROTO_RELATIVE to the call.

Status & tagging log