User:Werdna/Multilingual LiquidThreads Feedback

Here is my feedback for the Multilingual LiquidThreads code:

General Comments

 * It's obvious that there are a number of significant authors, and unfortunately their editors are configured with different indentation options. Some lines are indented with a tab, some lines are indented with two spaces, and most lines are indented with some combination of the two. MediaWiki code is indented with one tab per indentation level. I've fixed up some of the indentation in my local working copy, and I can commit the changes if you like.
 * Some of your code is commented out. I'd prefer that either an explanatory comment is left about what the code does and why it's commented out, or it's removed entirely. This will avoid some confusion.

canAttach
You shouldn't use @ to suppress warnings. Instead, write: See Manual:Coding_conventions

attachDescriptions
I think you shouldn't have these parentheses hardcoded, and instead use an interface message to configure how the untranslated text appears. For starters, English usage rules would demand a space before the (.

Strip markers
I see that you're using strip markers to protect parts of the input from translation. However, I don't think that your strip markers are distinctive enough to avoid collisions with user input. You should look at the strip-marker system used in MediaWiki's parser (see Parser::insertStripItem in includes/parser/Parser.php. In particular, the MediaWiki strip markers use a non-ASCII control character:

Parse performance
In a lot of places you're looping over the text one character at a time to replace balanced delimiters. I'd recommend following the example of StringUtils::delimiterReplaceCallback (defined in includes/StringUtils.php), and using strcspn or preg_match to find the next delimiter.

saveReply</tt>
PHP objects are accessed by reference, this line is unnecessary.

onThreadCommands</tt>
This is an odd place to put this logic. I'm also a little leery about using setRoot</tt> for this purpose, because it will end up changing the root in the database. Maybe we should add a new "displayRoot" property for each thread, which isn't persisted to the database but is used for display purposes only.

I suppose this is here because you've only just changed the root and the old history URL will be wrong. It should go when I implement what I've described above.

onThreadPermalinkView</tt>
The variable name $threadParmalinkView</tt> is misspelled.

translateSubject</tt>
This should use an interface message, so that it appears in a user's language.

translateSubjectforTOC</tt>
Again, I'm leery of using setSubject</tt> to achieve your aims. We'd be better off storing a "display subject" like we would with setRoot</tt>, or moving the hook closer to display so you can display the subject instead.

onThreadMajorCommands</tt>
$thread->title</tt> should be $thread->article->getTitle</tt>. You want to refer to the talkpage, not to the thread's permalink. Consider using LqtView::threadInContextLink</tt>. Its signature is.

onTalkpageMain</tt>
is_nulL</tt> –> is_null</tt>