https://bugzilla.wikimedia.org/show_bug.cgi?id=38622
--- Comment #15 from Tim Starling <[email protected]> 2012-08-10 07:54:33 UTC --- I could see those revisions once I deleted the local branch and recreated it. Here's a few review comments for you. There are still a lot of long lines. Pretty much every time there is a comment on the end of a line, it makes the line too long, so you should probably not do that. What are the changes to DairikiDiff.php for? The code changes don't match the commit message. In DifferenceEngine.php: } elseif( Hooks::isRegistered( 'ArticleViewCustom' ) && !wfRunHooks( 'ArticleViewCustom', array( ContentHandler::getContentText( $this->mNewContent ), $this->mNewPage, $out ) ) ) { #NOTE: deperecated hook, B/C only This is a common pattern. I suggest you provide a wrapper function for running legacy hooks so that code like this will be shorter and more legible. Something like: ContentHandler::runLegacyHook( 'ArticleViewCustom', array( $this->mNewContent, $this->mNewPage, $out ) ) with runLegacyHook() something along the lines of function runLegacyHook( $hookName, $args ) { foreach ( $args as $key => $value ) { if ( $value instanceof Content ) { $args[$key] = ContentHandler::getContentText( $value ); } } return wfRunHooks( $hookName, $args ); } #XXX: generate a warning if $old or $new are not instances of TextContent? #XXX: fail if $old and $new don't have the same content model? or what? These are good questions, and there are many more fixme comments like them throughout the Wikidata code. I can't approve it with these kinds of flaws remaining. If nothing will ever call this function with anything other than TextContent, then throwing an exception is the right thing to do. Diffing the serialized representation does not seem right to me, for non-text content. #XXX: text should be "already segmented". what does that mean? It means that $wgContLang->segmentForDiff() should have been run on it. The comment is an error introduced by me in r13029, you certainly should *not* segment the input of that function. In Article.php: $contentHandler = ContentHandler::getForTitle( $this->getTitle() ); $de = $contentHandler->createDifferenceEngine( $this->getContext(), $oldid, $diff, $rcid, $purge, $unhide ); Surely the content type used to get the diff engine should depend on the content of the revisions involved, not just the title. Same issue throughout commit 88f8ab8e90a77f1d51785ba63f2eac10599c3063. -- Configure bugmail: https://bugzilla.wikimedia.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ Wikibugs-l mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
