User:Catrope/Extension review/Translate/old

Base revision: r95391

✅ Translate.php

 * The  global functions should be in a static class, e.g.
 * ✅ 95755
 * What does  do? The way it interacts with the   is not really documented or intuitive
 * isn't exactly a good memcached key. It should be something unique and descriptive, e.g.
 * ✅ Method was removed in 95754

translate.sql

 * The index on  is completely useless given that the primary key is
 * Can I just remove it?
 * Yes. You probably want an index on  instead because of the query in   lines 752-755
 * What is this table for?
 * Stores the extracted sections for translatable pages.

✅ TranslateUtils.php

 * Line 49: can't you just use  or something along those lines?
 * It doesn't necessarily return array with two elements
 * Hmm, right.
 * Line 132: you're mixing ASC and DESC in an ORDER BY, and one of the fields you're ordering by is a derivative field (result of a function). That in combination with the large size of the recentchanges table on most WMF wikis (5M rows on enwiki) means this feature will have to be behind $wgMiserMode or otherwise disableable
 * Let's say target wikis are mediawiki.org, meta and commons. How does this hold up then? At translatewiki we have an RC table that is about 2.5M rows.
 * Doesn't the namespace and timestamp conditions filter the resultset to small enough?
 * Anyway, this code is only callable from command line scripts, so it should be safe.
 * Hmm, OK.
 * Line 136: what batch existence query?
 * ✅ No longer used as of 95759. Preparing for deletion of this code.
 * Line 139: you can also use
 * ✅ I didn't know it exists. 95758
 * Line 170: you may want to use a strict comparison here, otherwise  will produce strange results
 * ✅ Fixed in 95760
 * Line 202, 207: ZOMG, hardcoded English. Of all extensions... :)
 * ✅ Broken code removed in 95761
 * Line 256, 280: what is so performance-sensitive here that you have to factor out a simple function call? That makes little sense to me
 * ✅ Simplified in 95760

✅ TranslateTasks.php

 * Line 361:  is not escaped
 * ✅ 95762.
 * Line 462-468: can't you just use  here?
 * ✅ 95765.

TranslateEditAddons.php

 * Line 44, 49: you're using strtr here to replace spaces/underscores and str_replace elsewhere. Also, don't you have a function lying around that does lowercasing and space/underscore stuff?
 * strtr/str replace do the same in this case, right? Function: no I don't think so.
 * Line 53: -666 ?!?!?
 * Any index which doesn't exist and which doesn't have existing neighbours.
 * OK, very clever, but add a comment to explain what the weird choice of number is about.
 * Generally, you should mention what hook each function is hooked into in a doc comment, that makes understanding the code easier without having to go back to look up the $wgHooks assignments each time
 * Line 193: eww, onclick
 * I don't know how to fix in the best way
 * ✅ 95769.
 * Rev doesn't address this at all. I can help you with the JS
 * Line 98: why would $next equal true?
 * ✅ No idea, removed. I don't think anything will break, but we will find out. 95769
 * Line 111: what is SkinChihuahua?
 * ✅ Clarified in 95752
 * Line 122: $object seems to be an EditPage, document that
 * ✅ 95769.
 * Line 254-258: what does this logic do and why? There are no comments
 * ✅ It tries to figure when it can replace the contents of the editing area with stuff from elsewhere. MediaWiki doesn't make it easy.
 * Line 294: don't pass an array as the second parameter to . The function supports this by accident only
 * ✅ 95769.
 * Line 296: don't you really just mean to check that the second query didn't return zero results? If you meant it, say it, with something like ,   ,   or whatever. As currently written this function will return true in the (admittedly strange) case where 'fuzzy' isn't in the revtag_type table because   is true
 * ✅ 95769.

MessageGroups.php
I added this stuff earlier but never saved it, may be out of date
 * Line 760: the regex seems to match stuff like  . That syntax is kinda strange, is that intended?
 * Yes it is the horrible syntax I came up. Fortunately I don't think anybody uses it so I could change it to something better.
 * Line 881-884: doesn't  do what you want?
 * I was pretty sure that I had some reason. IRCC it was not implemented in a good way until recently. This also relates to the point that some data needs permanent storage (throttles etc) to work, while some data can be always reconstructed. I think it is safe to use CACHE_ANYTHING now.
 * Line 928: this query filesorts. Looking up the ID of tp:mark in a separate query, then doing the other query (on page and revtag) but grouping by rt_page instead of page_id should work better
 * Was fixed already with the new schema.

utils/TranslationHelpers.php

 * Line 1224: ??? makeVariablesScript shouldn't be used this way. You should export variables through a MakeGlobalVariablesScript hook, and you shouldn't just export the return value of wfMsg, use the normal message exporting system instead
 * Do you mean resource loader? This code was written way before RL.
 * D'oh, of course. Yes, I guess it just never got RL-ified then, that should be done.
 * ✅ and follow-ups  and.