https://bugzilla.wikimedia.org/show_bug.cgi?id=20246
--- Comment #20 from Brion Vibber <[email protected]> 2011-05-07 01:15:15 UTC --- Overall the code looks pretty nice and has comments and stuff which makes me happy. ;) Couple things that stick out to me: * static functions as hook callbacks are intermixed with a non-static singleton class which feels a bit odd to me; it's hard to tell what's what sometimes. * not sure what's up with the mPages, mMaps member variables; what's the lifetime of the ExtTransliterator object? If batch jobs are running, will this in-process cache get updated by actions in another process? Or will it be discarded between jobs within the same process? It looks like a new object is created at ParserFirstCallInit time... offhand I'm not sure whether a new parser will get created for job runs or not. Probably won't break anything in practice, but it's worth looking at for -- in-process caches are always dangerous in a multi-node environment. * Several functions accept reference parameters, like isMapPage( &$title ) which don't appear that they should. Unless it's possible to *replace an entire object parameter with another object* or *alter a scalar value or array contents*, references should not be used. If these are to match hook definitions, in many cases the hooks probably need fixing upstream, and when they are fixed these functions will fail on PHP 5.3 unless they are also fixed to remove the references; I'd also recommend naming the functions with an 'on' prefix and the actual name of the calling hook if possible, to make it clearer what's going on. In general I'd also recommend looking out for what happens when you're given a huge amount of input; the default NFD decomposition implementation is not very efficient, and it might be more likely to keel over and die if, say, you accidentally don't close the tag and try to transliterate a very large page full of non-English text. -- Configure bugmail: https://bugzilla.wikimedia.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the assignee for the bug. You are on the CC list for the bug. _______________________________________________ Wikibugs-l mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
