MediaWiki

User:Catrope/Extension review/Translate/old

< User:Catrope | Extension review | Translate

Base revision: r95391

Yes Done Translate.php

edit
  • The efFoo() global functions should be in a static class, e.g. TranslateHooks
      Done rev:95755
  • What does efTranslateCheckPT() do? The way it interacts with the revtag_type is not really documented or intuitive
    • 'pt' isn't exactly a good memcached key. It should be something unique and descriptive, e.g. wfMemcKey( 'translate', 'pageTranslationVersion' )
      Done Method was removed in rev:95754

translate.sql

edit
  • The index on (trs_page) is completely useless given that the primary key is (trs_page, trs_key)
    Can I just remove it?
    Yes. You probably want an index on (trs_page, trs_order) instead because of the query in MessageGroups.php lines 752-755
  • What is this table for?
    Stores the extracted sections for translatable pages.

Yes Done TranslateUtils.php

edit
  • Line 49: can't you just use list( $key, $code ) = explode( '/', $text, 2 ); 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?
      Done No longer used as of rev:95759. Preparing for deletion of this code.
  • Line 139: you can also use $rows = iterator_to_array( $res );
      Done I didn't know it exists. rev:95758
  • Line 170: you may want to use a strict comparison here, otherwise simpleSelector( 'foo', array( '0', '00', '1' ), '00' ) will produce strange results
      Done Fixed in rev:95760
  • Line 202, 207: ZOMG, hardcoded English. Of all extensions... :)
      Done Broken code removed in rev: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
      Done Simplified in rev:95760

Yes Done TranslateTasks.php

edit
  • Line 361: $data is not escaped
      Done rev:95762.
  • Line 462-468: can't you just use array_diff() here?
      Done rev:95765.

TranslateEditAddons.php

edit
  • 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
      Done rev:95769.
    Rev doesn't address this at all. I can help you with the JS
  • Line 98: why would $next equal true?
      Done No idea, removed. I don't think anything will break, but we will find out. rev:95769
  • Line 111: what is SkinChihuahua?
      Done Clarified in rev:95752
  • Line 122: $object seems to be an EditPage, document that
      Done rev:95769.
  • Line 254-258: what does this logic do and why? There are no comments
      Done 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 selectField(). The function supports this by accident only
      Done rev: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 $res !== false , (bool)$res , $dbr->numRows() > 0 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 false === false is true
      Done rev:95769.

MessageGroups.php

edit

I added this stuff earlier but never saved it, may be out of date

  • Line 760: the regex seems to match stuff like bar . 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 CACHE_ANYTHING 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

edit
  • 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.
      Done r96007 and follow-ups r96008 and r96009.
Retrieved from "https://www.mediawiki.org/w/index.php?title=User:Catrope/Extension_review/Translate/old&oldid=439785"