User:Catrope/Extension review/ShortUrl

Base revision: r95304

General notes

 * You're adding a table where you permanently map an (ns,title) pair to an ID. Is this because you want the ID to keep referring to the same title through moves etc, unlike page IDs? I guess my question is: is there a specific reason why you're not using page IDs?
 * Another thing to consider may be deletion of a page and re-creation of the same page at a later time. As well as disambiguation/split of page, where keeping the page_id would make an assumption of which definition would be intended (instead of the title, which would then have become a disambigpage) Krinkle 00:05, 29 September 2011 (UTC)
 * There's a layout bug in IE7, see this screenshot

ShortUrl.php

 * Indented line Done in r103035


 * is unnecessary. jQuery is guaranteed to be present
 * Configuration variables (you have only one in this case) should be near the top of the file, clearly marked as such, have a documentation comment. Since the default is null, you should explain what that means too

ShortUrl.i18n.php

 * Indented line Done in r103035


 * Don't Use Title Case (Or Otherwise Use Too Many Capitals), The People At TranslateWiki Don't Like That. Specifically,  should not be capitalized

ShortUrl.hooks.php

 * Indented line Done in r103035


 * Line 24: don't do a loose comparison with null, do a strict comparison instead
 * Line 25: you need to use  here instead of   so things will work properly on wikis with protocol-relative URLs. You could've have known this because   and most of the other protocol-relative URL support code didn't even exist yet when you wrote this code

ShortUrl.utils.php

 * Indented line Done in r103035


 * Line 28: you probably want  rather than  . The latter will return different values for   and , even though they'll map to the same su_id and the same base36 thing.
 * Line 32: there's a convenient function called  that returns a single row object or false. It's specifically intended for queries like these where you only want one row. It adds LIMIT 1 to the query but that's not a problem here
 * does not account for the possibility that the short URL ID does not occur in the database. In that case it returns an invalid (!!!) Title object with an empty string as the title. There is a check in  that shows an error page if   returns false or null or something, but that code is never reached because it always returns an object. Instead, the code will try to redirect to a page with an empty name, and it so happens that that redirects you to the main page.
 * The way the error message is done looks wrong anyway.  should be used for that

shorturls.sql

 * Indented line Done in r103035


 * The way the PRIMARY KEY and UNIQUE KEY are defined is not SQLite-compatible. See the manual on SQLite compatibility

ext.shortUrl.js

 * Indented line Done in r103035


 * Line 4:  is not escaped here. Use something like   so things are escaped properly

ext.shortUrl.css

 * Indented line Done in r103035


 * should not be capitalized
 * Applying  to the link has a weird side effect where the clickable area extends all the way to the right, see this screenshot

populateSortUrlTable.php
mysql> SELECT COUNT(*) FROM page; +--+ +--+ +--+ 1 row in set (2 min 38.66 sec)
 * Line 29: you can't just select all rows from the page table in one query. Here's why:
 * COUNT(*) |
 * 24849480 |
 * Using  to insert rows in batches of 100 is a good idea. However, you should also
 * select stuff from the page table in 100-row batches too. You can do that with something like
 * print progress markers for each batch rather than for each row, so you get something like  instead of
 * increment  before printing, not after. Currently it starts at zero and finishes one too low
 * call  after inserting each batch to make sure replication lag doesn't go through the roof while you populate your 25 million-row table
 * print "done" at the end so the user knows the script finished properly rather than dying in the middle or something