r76544 MediaWiki - Code Review archive

This review is for the entire GNSM extension. I've copied these review notes from Bug 21919, hopefully to make them more readable

From RK

(from his IRC review)

• Detection for FlaggedRevs presence is fragile and I'm not sure it'll even work any more
• Also, there's an arbitrary, unlimited number of joins against categorylinks, that's a no-go (has potential to make the DB servers sweat)
• It's doing DIY category intersection
• Also, at least one of the two sort modes is likely to be very DB-inefficient. Can't tell for sure cause I can't read the query too well, the code is not very pretty and the query is complex and very dynamic
• Amgine: That's fine; if you can give me a general idea of what the query is gonna look like, I can poke at it (to optimize the above)
• Amgine: What I've given you just now should keep a dev busy for a few hours already. Am of course available for general help, but don't think it'd be useful to do an exhaustive review before those things have been taken care of

From baowolff in IRC

• In the ideal world, since its essentially a direct copy of DPL, DPL would just split some of the html formatting code from the make a query to the db code, then this extension could just depend on DPL, and there would be much less duplication making dpl more modular would be nice for other reasons too...

From me:

• echo and print?!?! Don't do it. That and direct output (e.g. closing ?>) should not be done. Note that this is (part of) what is causing using the template format not to work.
  • To read: http://www.mediawiki.org/wiki/Manual:Special_pages#OutputPage.php
• Don't try to create XML yourself, it doesn't work. Use XMLWriter or DOM to create XML. That way you don't have do xmlEncode().
• Check Google News' docs and sitemaps.org docs: it looks like there are some things in the GNSM format that should be handled.
  • should limit number of urls (1000?)
  • Limit the age of pages included to 2 days?
  • is there a way to integrate multiple site maps?
  • document robots.txt?
• Hate the use of params as member variable.
  • You should just copy things from wgRequest after checking them.
  • Having a member variable named params seems like it is the beginning of confusion.
  • Also put wgRequest directly in the foreach, or at the very least closer to the point of use to avoid confusion with the others.
  • If you don't intend for the result of $wgRequest to be used, then it shouldn't be in $params at all.
• code indention looks lacking.
• Incorrect use of wfEmptyMsg(??) not sure about this but the function def doesn't match function declaration in GlobalFunctions while examples like the one used here are abundent elsewhere.

For what it's worth, I read "RK" as "Ryan Kaldari." "Roan" is two extra characters. :P

Granted, but I used RK here because Roan used it in a comment in the code.

I suffer from "I was here first" bias in that respect. When being introduced to Ryan Kaldari, the fact that he shares my initials didn't register with me, so I continued to use them :)

Thanks for making me aware of this; I should start using "Roan" instead of "RK"

The second parameter for wfEmptyMsg was removed at some point because it was no longer needed.

A couple of points missed from the review discussion:

• The max number of joins is configurable.
• Iirc the age of pages is also configurable.

- ~~~~

A year old review of the extension is here

Setting this to fixme so the review doesn't get lost until it is addressed.

I'd half rather you didn't, considering I just did some cleanup... :P

I certainly don't want to label this commit as wrong but I don't want to lose the comments. I'll try to come back soon and put the comments in the right places.

Surely the below code shouldn't be in the body's execute function?

$wgFeedClasses[] = array( 'sitemap' => 'SitemapFeed' );