User "Raindrift" changed the status of MediaWiki.r103881. Old Status: fixme New Status: new
User "Raindrift" also posted a comment on MediaWiki.r103881. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/103881#c26403 Commit summary: MoodBar Phase 2 - Feedback response API Comment: + mbfr_user_ip varchar(255) binary NULL, -- If anonymous, user's IP address Elsewhere in Mediawiki, IP addresses are stored as varbinary(40). You may want to make that change, since it shouldn't effect the rest of your code and it'd be consistent. Also, performance might be slightly higher. I'm not really clear on why MBFeedbackResponseItem has a constructor (which the comment says not to use, but which does things anyway), a separate create() method that's not called from the constructor, and then two initializers. The two separate init schemes makes sense, but why not choose which one to run directly from the constructor? throw new MWException( "Attempt to set invalid property $key" ); If you create new exceptions for things like this, and add them to @throws in your docblock, it'll be clearer what's going on later, and it'll be possible for a caller to catch some exceptions and not others. At the end of the file, you can just create new exception subclasses like so: class MWFeedbackResponseItemPropertyException extends MWException {}; or name it something shorter... the point is to tell it apart from every other MWException. if you want to see an example, check out includes/upload/UploadStash.php. A lot of Mediawiki code doesn't do this, so I guess it's optional, but more granularity here is better. I'm not sure if any of these are FIXME-worthy. I'll set this rev back to new so maybe other reviewers will take a look at it. _______________________________________________ MediaWiki-CodeReview mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
