"Awjrichards" changed the status of Wikimedia.r475 to "fixme" and commented it. URL: http://www.mediawiki.org/wiki/Special:Code/Wikimedia/475#c27591
Old Status: ok > New Status: fixme Commit summary for Wikimedia.r475: Added abstract code for setting log levels. Included unit testing which is at 100% code coverage for all checked in code. Awjrichards's comment: Argh! The comment I initially wrote for this seems to have disappeared. The jist of it was that using extract() (like in the __construct() method of Listener/Adapter/Abstract.php) is fraught with various issues, namely: 1) It's really difficult to know what actually are valid/acceptable things to pass into the method. I know that in the __construct() example you've tried to get around this by listing out possible params in the comments, but code and code comments rarely stay in sync. In fact, using the same example, it looks like $parameters[ 'settings' ] is totally valid to pass into __construct(), however 'settings' is not documented in the method comments. This complicates things for other programmers who will work with your code, for yourself when you come back to this 6 months from now, or from your friendly neighborhood reviewer who has to spend a lot of time disecting the code to really understand what's going on rather than being able to tell just by glancing at it. 2) It would be easy to accidentally create a situation where extract() could overwrite local variables that you do not want overwritten. In my original comment I had lots of examples and places throughout this code base that I've seen extract() used, but we'll have to settle for this instead :) Long story short, it would be better to use explicit array keys if you're passing params in an associative array or otherwise explicitly assign params to a local variable rather than use extract(). You'll save yourself, other programmers, and reviewers a lot of time and headache. _______________________________________________ MediaWiki-CodeReview mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
