Extension talk:RPED/Alpha release

Review

Tisane, I gave you those links on development for a reason.

• Your extension does not escape anything in SQL queries.
• It does not escape anything it outputs to the user too.
• You don't use MediaWiki's own DAL, resorting to non-portable direct database access functions.
• What will happen if someone upgrades the extension? They'll have to edit it again to restore their DB credentials.
• page_name text(1000) take a look at how MediaWiki stores page names (by the way, they're called page titles), and what is their maximum length.
• You should use Title class to normalise titles.
• If you don't use indexes, your extension will work just fine on development datasets of 5 pages and 20-30 links, but will kill the site with even mid-sized wikis.
• You shouldn't allow anons to update your link database.
• Have you tested if it works? if (fpointer>0) suggests that not really. Remember, all development must be done with error_reporting set to E_ALL | E_STRICT. And code must produce no warnings or notices.
• The code is simply disgusting to read, because it's not formatted properly.

Max Semenik 14:16, 28 February 2010 (UTC)

Reply

• Your extension does not escape anything in SQL queries.

Fixed

All the queries from the PHP files are escaped now. As for the Perl scripts, those are only going to be taking data from Wikipedia which is a safe source, so escaping is not needed for them.

• It does not escape anything it outputs to the user too.

Not necessary because unfiltered user data is not output to the user at any time.

• You don't use MediaWiki's own DAL, resorting to non-portable direct database access functions.

I spent some time reading http://svn.wikimedia.org/doc/classDatabaseBase.html and Manual:Database access, but couldn't figure out how to get it do what I wanted using those wrappers. I guess I'll take another look at it. I think one of the things I had trouble with was getting it to create a new table that wasn't a duplicate of one already existent (otherwise I could just use duplicateTableStructure)

• What will happen if someone upgrades the extension? They'll have to edit it again to restore their DB credentials.

Fixed

It now reads from RPEDConfig.php to get those credentials.

• page_name text(1000) take a look at how MediaWiki stores page names (by the way, they're called page titles), and what is their maximum length.

Fixed

I changed it to 256.

• You should use Title class to normalise titles.

Why?

• If you don't use indexes, your extension will work just fine on development datasets of 5 pages and 20-30 links, but will kill the site with even mid-sized wikis.

Fixed

They're indexed now.

• You shouldn't allow anons to update your link database.

Fixed

Access is now restricted to bureaucrats.

• Have you tested if it works? if (fpointer>0) suggests that not really. Remember, all development must be done with error_reporting set to E_ALL | E_STRICT. And code must produce no warnings or notices.

Fixed

It worked on E_ALL | E_STRICT.

• The code is simply disgusting to read, because it's not formatted properly.

Fixed

I formatted it.

Tisane 11:22, 8 March 2010 (UTC)