[MediaWiki-CodeReview] [MediaWiki r89637]: New comment added, and revision status changed
User Patrick Nagel changed the status of MediaWiki.r89637. Old Status: fixme New Status: new User Patrick Nagel also posted a comment on MediaWiki.r89637. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/89637#c21653 Commit summary: Initial commit of new extension Notificator Comment: I think besides small non-critical details that I'll get back to later, it's in a good state. Setting to 'new' now. Nikerabbit, do you agree? ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r89637]: New comment added
User Nikerabbit posted a comment on MediaWiki.r89637. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/89637#c21657 Commit summary: Initial commit of new extension Notificator Comment: Good enough for me. ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r89637]: New comment added
User Nikerabbit posted a comment on MediaWiki.r89637.
Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/89637#c20989
Commit summary:
Initial commit of new extension Notificator
Comment:
pre+if (!defined('MEDIAWIKI')) {/pre This check is not needed if the file
only contains a class. Please also run stylize.php on the files and indent
stuff inside class with one level, as is the normal practice.
getDiffCss can most likely be replaced with plain file_get_contents(). Though I
don't like how the css is duplicated here. Does it somehow differ from the
normal diff css or why do we need to copy it?
___
MediaWiki-CodeReview mailing list
mediawiki-coderev...@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r89637]: New comment added
User Nikerabbit posted a comment on MediaWiki.r89637. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/89637#c20990 Commit summary: Initial commit of new extension Notificator Comment: Ignore my comment about stylize.php, it was already done later. ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r89637]: New comment added
User Patrick Nagel posted a comment on MediaWiki.r89637. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/89637#c20995 Commit summary: Initial commit of new extension Notificator Comment: Basically diff-in-mail.css just contains skins/common/diff.css minus comments and minus 'background-color: white'. The first is due to the fact that the CSS is sent with every notification e-mail, and it would just be a waste of bandwidth to send the developer-targeted comments along. The latter because it looked weird when the mail client window's background colour was grey. I don't like the duplication either, but I think mangling 'skins/common/diff.css' in some way isn't elegant, either. What do you think? ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r89637]: New comment added
User Hashar posted a comment on MediaWiki.r89637. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/89637#c20731 Commit summary: Initial commit of new extension Notificator Comment: nikerabbit, do you mind reviewing this extension again ? :-) ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r89637]: New comment added
User Hashar posted a comment on MediaWiki.r89637. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/89637#c20732 Commit summary: Initial commit of new extension Notificator Comment: Revisions above manually added as follows-up. ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r89637]: New comment added
User Peachey88 posted a comment on MediaWiki.r89637. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/89637#c17719 Commit summary: Initial commit of new extension Notificator Comment: I believe you need to set up your svn autoprops for svn:eol style ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r89637]: New comment added
User Patrick Nagel posted a comment on MediaWiki.r89637. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/89637#c17722 Commit summary: Initial commit of new extension Notificator Comment: Oh, right! I did that on the other box, but forgot to do it here. Hope I fixed that correctly now... ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r89637]: New comment added, and revision status changed
User Nikerabbit changed the status of MediaWiki.r89637. Old Status: new New Status: fixme User Nikerabbit also posted a comment on MediaWiki.r89637. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/89637#c17723 Commit summary: Initial commit of new extension Notificator Comment: You need to do a lot more escaping of things you output into html. I also think we already have a function for validating email address. +global $wgPasswordSender, $ngFromAddress; +if(! $ngFromAddress) $ngFromAddress = $wgPasswordSender; Don't do that, it is security vulnerability. I assume this extension was written a long time ago? ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
[MediaWiki-CodeReview] [MediaWiki r89637]: New comment added
User Patrick Nagel posted a comment on MediaWiki.r89637. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/89637#c17852 Commit summary: Initial commit of new extension Notificator Comment: Thanks for your advice, Nikerabbit. I gave it some more work now: * r89714: Removed wfLoadExtensionMessages() call, backwards-compatibility not needed, since this extension does not work with 1.17b1 anyway. Btw.: wfLoadExtensionMessages() is mentioned in http://www.mediawiki.org/wiki/Manual:Special_pages#The_Special_Page_File - maybe it should be mentioned there, that this call is no longer needed for extensions that don't need to be compatible with $old MW versions? * r89715: Changed $ngFromAddress init. Using $wgPasswordSenderName and $wgPasswordSender as default for $ngFromAddress. That fixes the register_globals vulnerability. * r89716: Replaced Notificator::checkEmailAddress() with MW's Sanitizer::validateEmail() * r89719: Switched to Notificator::checkEmailAddress() again, after discovering that Sanitizer::validateEmail() is not available in any released MW version; Lots of whitespace changes (ran stylize.php and limited line length to 100). [Should have done that in two commits, but it would have been a big hassle] About the You need to do a lot more escaping of things you output into html part - I think I had already escaped all user-provided input with htmlspecialchars(), what else needs to be done? ___ MediaWiki-CodeReview mailing list mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
