"Bawolff" posted a comment on MediaWiki.r111424. URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/111424#c30925
Commit summary for MediaWiki.r111424: follow-up r111412 . Added the possibility of defining a whitelist of allowed Etherpad Lite servers; using wfMsgForContent Bawolff's comment: To be nitpicky (as if i wasn't being nitpicky already ;) in code comment ''do not use wfMsgForContent but wfMsgForContent'' should be ''do not use wfMsg but wfMsgForContent''. And I'm not sure about using Http::isValidURI (not this revision though) - that method is more to verify that the url is something that can be handled by the Http class (aka Http::get and friends) rather then to validate if its a valid uri. I personally think that <code>preg_match( '/^(?:' . wfUrlProtocols() . ')/', $url )</code> would be a better test to check if its a url (bearing in mind that is also the test that Sanitizer::validateAttributes does). Generally its preferred to instead of doing: return wfMsgForContent( 'etherpadlite-invalid-pad-url', htmlspecialchars( $src ) ); do return htmlspecialchars( wfMsgForContent( 'etherpadlite-invalid-pad-url', $src ) ); (Messages that can potentially output html are discouraged in new code.) Otherwise it looks ok to me. _______________________________________________ MediaWiki-CodeReview mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
