Last modified: 2011-03-02 00:47:33 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 T16418, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 14418 - Add rd_interwiki colum to redirect table
Add rd_interwiki colum to redirect table
Product: MediaWiki
Classification: Unclassified
Database (Other open bugs)
All All
: Normal enhancement with 3 votes (vote)
: ---
Assigned To: Nobody - You can work on this!
: patch, patch-need-review, schema-change
Depends on:
Blocks: 6363 15147 9237 16012
  Show dependency treegraph
Reported: 2008-06-05 19:21 UTC by Chad H.
Modified: 2011-03-02 00:47 UTC (History)
5 users (show)

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

Basic implementation and population of rd_interwiki column (2.27 KB, patch)
2008-07-16 17:32 UTC, Chad H.
Updated patch (3.13 KB, patch)
2008-07-31 01:15 UTC, Chad H.
Proposed patch (8.86 KB, patch)
2008-08-06 12:50 UTC, Roan Kattouw
Update previous patch (11.30 KB, patch)
2009-02-18 19:26 UTC, Chad H.

Description Chad H. 2008-06-05 19:21:41 UTC
While trying to fix 9237, it turns out that interwiki redirects are stored in the database without their interwiki prefix. While 9237 has a patch to remove interwiki redirects entirely, it would make more sense to keep them. Looking at the database, it appears the prefix is stripped entirely from the title.

After adding a redirect from [[Test]] to [[fr:Platypus]] on my local install, the query return this:
Rd_from = 58
Rd_namespace = 0
Rd_title = Platypus

Adding this column (and making sure it's populated correctly, I'm not 100% sure where this is done), allows bug 9237 to be fixed very easily and also opens the door to something such as [[Special:InterwikiRedirects]].
Comment 1 Roan Kattouw 2008-06-11 20:34:35 UTC
Seems like I'm not the only one who wants this. In my view, the redirect table should cover *all types* of redirects, so we can actually use it to redirect people, rather than parsing the article text like we do now.
Comment 2 Chad H. 2008-07-16 17:32:40 UTC
Created attachment 5076 [details]
Basic implementation and population of rd_interwiki column

In this patch, I have implemented the rd_interwiki column for the redirect table. Changes to Article class to make sure it is set/updated as needed. Nothing makes use of this yet, but it's working properly. 

Could probably use a maintenance script to back-fill all current redirects.
Comment 3 Roan Kattouw 2008-07-17 20:24:25 UTC
Patch looks good, but what's the whole "username of blocker" and "global blocking" thing about? I don't see how that ties in to rd_interwiki. Also, the comment in question doesn't seem to describe (at all) what the rd_interwiki field is about. Futhermore, why do you declare it as CHAR (32) rather than VARCHAR (32)?
Comment 4 Chad H. 2008-07-22 14:34:02 UTC
(In reply to comment #3)
> Patch looks good, but what's the whole "username of blocker" and "global
> blocking" thing about? I don't see how that ties in to rd_interwiki. Also, the
> comment in question doesn't seem to describe (at all) what the rd_interwiki
> field is about. Futhermore, why do you declare it as CHAR (32) rather than
> VARCHAR (32)?

The blocking comment was an oversight from a copy+paste of a different sql patch. Sorry about that.

I chose char(32) instead of varchar(32) as it's what the interwiki table uses for the iw_prefix column.
Comment 5 Brion Vibber 2008-07-30 22:31:10 UTC
I'd recommend using the varchar form; we should update the interwiki table if it's still using char.

My inclination is also to let the field be NULL for non-interwikis, rather than forcing it to empty string.

Would it be desirable to also include a field for target fragments? (eg [[Dest#section]]) Currently to get the total destination link you'd have to load up the source text and parse it.
Comment 6 Chad H. 2008-07-31 01:15:53 UTC
Created attachment 5111 [details]
Updated patch

Updated patch to incorporate feedback:

1) Using varchar instead of char
2) Use null as default instead of empty string
3) Remove unrelated comment from sql patch
4) Add rd_fragment column to populate with section links

Redirecting [[Test]] to [[wikipedia:Platypus#Virus]] then querying the redirect table returns the following:

rd_from = 2
rd_namespace = 0
rd_title = Platypus
rd_interwiki = wikipedia
rd_fragment = Virus

Changing values for the iw prefix, target page, fragment, et all update columns as expected.
Comment 7 Roan Kattouw 2008-08-04 14:48:18 UTC
Looking good. With rd_interwiki *and* rd_fragment, we can finally use the redirect table to resolve redirects rather than parsing the page contents. I attempted to do this in my fix fro bug 10931, but that failed for interwiki and fragment redirects.
Comment 8 Roan Kattouw 2008-08-06 12:50:52 UTC
Created attachment 5125 [details]
Proposed patch

Expanded on Demon's patch. Basically, it automatically sets rd_interwiki and rd_fragment for redirects that don't have them yet when they're viewed, and (at last!) implements redirect resolution using the redirect table rather than the page text. I've tested it and it works for me (even fragment+interwiki redirects).

In detail:
* Get and insert/update rd_interwiki and rd_fragment where needed
* Change Article::getRedirectTarget() to cover all cases and to update the row if rd_interwiki or rd_fragment is NULL (i.e. if it's a leftover from the old schema without those fields)
* Rewrite Article::followRedirect() to incorporate most of the code from followRedirectText() (which has undergone a minor cleanliness rewrite) and to use getRedirectTarget()
* Remove followRedirectText() as it's unneeded now
* Add $interwiki parameter to Title::makeTitle(), makeTitleSafe() and makeName()
* Document and update Title::getRedirectsHere()
Comment 9 Chad H. 2008-08-06 13:43:14 UTC
Looks good to me. I was also doing some testing last night (already spoke to Roan about this), and the refreshLinks maintenance script will work just fine for back-filling any non-existent redirect fields. Eventually the code goes through Article::updateRedirectOn(), which is part of the overall updates here, so everything should populate as needed.
Comment 10 Siebrand Mazeland 2008-08-16 22:35:14 UTC
How about committing something and closing the issue?
Comment 11 Chad H. 2008-08-17 01:49:06 UTC
Waiting for Brion to coordinate the schema changes, afaik.
Comment 12 Chad H. 2009-02-18 19:26:13 UTC
Created attachment 5828 [details]
Update previous patch

Same patch, works exactly as intended. Updated to apply cleanly to HEAD.
Comment 13 Tim Starling 2009-03-11 00:12:48 UTC
Put both field additions into the same patch. They can share an add_field call. 

Change the rd_ns_title index, it will need to have rd_interwiki at the start of it.

-			array('rd_namespace', 'rd_title'),
+			array( 'rd_namespace', 'rd_title', 'rd_interwiki', 'rd_fragment' ),

Use "*", it's shorter

-				'rd_title' => $retval->getDBKey()
+				'rd_title' => $retval->getDBKey(),
+				'rd_title' => $retval->getDBKey(),

You probably only need one of these.

 	public function insertRedirect() {
-		$retval = Title::newFromRedirect( $this->getContent() );
+		$retval = Title::newFromRedirectRecurse( $this->getContent() );

This appears to be wrong and broken. There are two choices: have the redirect table point to the final destination and use a separate tracking table to update the table when the intermediate redirects change, or have the redirect table point to the immediate destination and follow the chain on fetch. This appears to be like the first option, except missing the complex update logic.

Comment 14 Chad H. 2010-12-17 16:35:46 UTC
This was done in r69181, r51583.

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