r89637 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r89636‎ | r89637 | r89638 >
Date:09:20, 7 June 2011
Author:nagelp
Status:resolved (Comments)
Tags:
Comment:
Initial commit of new extension Notificator
Modified paths:
  • /trunk/extensions/Notificator (added) (history)
  • /trunk/extensions/Notificator/Notificator.body.php (added) (history)
  • /trunk/extensions/Notificator/Notificator.i18n.php (added) (history)
  • /trunk/extensions/Notificator/Notificator.php (added) (history)
  • /trunk/extensions/Notificator/Notificator.sql (added) (history)
  • /trunk/extensions/Notificator/SpecialNotificator.php (added) (history)
  • /trunk/extensions/Notificator/diff-in-mail.css (added) (history)

Diff [purge]

Index: trunk/extensions/Notificator/Notificator.sql
@@ -0,0 +1,6 @@
 2+CREATE TABLE IF NOT EXISTS notificator (
 3+ page_id int(10) NOT NULL,
 4+ rev_id int(10) NOT NULL,
 5+ receiver_email tinytext NOT NULL,
 6+ PRIMARY KEY (page_id, rev_id, receiver_email(10))
 7+);
\ No newline at end of file
Index: trunk/extensions/Notificator/Notificator.body.php
@@ -0,0 +1,200 @@
 2+<?php
 3+
 4+if (!defined('MEDIAWIKI')) {
 5+ echo "Not a valid entry point";
 6+ exit(1);
 7+}
 8+
 9+class Notificator {
 10+
 11+public static function notificator_Render($parser, $receiver = '', $receiverLabel = '') {
 12+ global $wgScript, $wgTitle;
 13+
 14+ if(! $receiverLabel) {
 15+ $receiverLabel = $receiver;
 16+ }
 17+
 18+ // Check that the database table is in place
 19+ if(! Notificator::checkDatabaseTableExists()) {
 20+ $output = '<span class="error">' . wfMsg('notificator-db-table-does-not-exist') . '</span>';
 21+ return array($output, 'noparse' => true, 'isHTML' => true);
 22+ }
 23+
 24+ // Check whether the parameter is a valid e-mail address or not
 25+ if($receiver && Notificator::checkEmailAddress($receiver)) {
 26+ // Valid e-mail address available, so just show a button
 27+ $output = '<form action="' . $wgScript . '/Special:Notificator" method="post" enctype="multipart/form-data">
 28+<input type="hidden" name="pageId" value="' . $wgTitle->getArticleID() . '" />
 29+<input type="hidden" name="revId" value="' . $wgTitle->getLatestRevID() . '" />
 30+<input type="hidden" name="receiver" value="' . $receiver . '" />
 31+<input type="submit" value="' . wfMsg('notify-address-or-name', htmlspecialchars($receiverLabel)) . '" />
 32+</form>';
 33+ } else {
 34+ // No valid e-mail address available, show text entry field and button
 35+ $output = '<form action="' . $wgScript . '/Special:Notificator" method="post" enctype="multipart/form-data">
 36+<input type="hidden" name="pageId" value="' . $wgTitle->getArticleID() . '" />
 37+<input type="hidden" name="revId" value="' . $wgTitle->getLatestRevID() . '" />
 38+<input type="text" name="receiver" value="' . wfMsg('e-mail-address') . '" onfocus="if (this.value == \'' . wfMsg('e-mail-address') . '\') {this.value=\'\'}" />
 39+<input type="submit" value="' . wfMsg('notify') . '" />
 40+</form>';
 41+ }
 42+
 43+ return $parser->insertStripItem($output, $parser->mStripState);
 44+}
 45+
 46+private function checkDatabaseTableExists() {
 47+ $dbr = wfGetDB(DB_SLAVE);
 48+ $res = $dbr->tableExists('notificator');
 49+ return $res;
 50+}
 51+
 52+private function getDiffCss() {
 53+ $ret = '';
 54+ $file = fopen(dirname(__FILE__) . '/diff-in-mail.css', 'r');
 55+ while(!feof($file)) {
 56+ $ret = $ret . fgets($file, 4096);
 57+ }
 58+ fclose ($file);
 59+ return $ret;
 60+}
 61+
 62+public static function checkEmailAddress($string) {
 63+// from http://www.linuxjournal.com/article/9585
 64+ $isValid = true;
 65+ $atIndex = strrpos($string, "@");
 66+ if (is_bool($atIndex) && !$atIndex) {
 67+ $isValid = false;
 68+ } else {
 69+ $domain = substr($string, $atIndex+1);
 70+ $local = substr($string, 0, $atIndex);
 71+ $localLen = strlen($local);
 72+ $domainLen = strlen($domain);
 73+ if ($localLen < 1 || $localLen > 64) {
 74+ // local part length exceeded
 75+ $isValid = false;
 76+ }
 77+ else if ($domainLen < 1 || $domainLen > 255) {
 78+ // domain part length exceeded
 79+ $isValid = false;
 80+ }
 81+ else if ($local[0] == '.' || $local[$localLen-1] == '.') {
 82+ // local part starts or ends with '.'
 83+ $isValid = false;
 84+ }
 85+ else if (preg_match('/\\.\\./', $local)) {
 86+ // local part has two consecutive dots
 87+ $isValid = false;
 88+ }
 89+ else if (!preg_match('/^[A-Za-z0-9\\-\\.]+$/', $domain)) {
 90+ // character not valid in domain part
 91+ $isValid = false;
 92+ }
 93+ else if (preg_match('/\\.\\./', $domain)) {
 94+ // domain part has two consecutive dots
 95+ $isValid = false;
 96+ }
 97+ else if (!preg_match('/^(\\\\.|[A-Za-z0-9!#%&`_=\\/$\'*+?^{}|~.-])+$/', str_replace("\\\\","",$local))) {
 98+ // character not valid in local part unless
 99+ // local part is quoted
 100+ if (!preg_match('/^"(\\\\"|[^"])+"$/', str_replace("\\\\","",$local))) {
 101+ $isValid = false;
 102+ }
 103+ }
 104+ if ($isValid && !(checkdnsrr($domain,"MX") || checkdnsrr($domain,"A"))) {
 105+ // domain not found in DNS
 106+ $isValid = false;
 107+ }
 108+ }
 109+ return $isValid;
 110+}
 111+
 112+public static function getLastNotifiedRevId($pageId, $revId, $receiver) {
 113+ // Returns -1 if any parameter is missing
 114+ // Returns -2 if the database revId is the same as the given revId (= notified already)
 115+ // Returns revId from the database - if there is no record, return 0
 116+
 117+ if (! $pageId || ! $revId || ! $receiver) {
 118+ return -1;
 119+ }
 120+
 121+ // Get $oldRevId from database
 122+ $dbr = wfGetDB(DB_SLAVE);
 123+ $res = $dbr->select('notificator', // table
 124+ 'rev_id', // vars (columns of the table)
 125+ array('page_id' => (int)$pageId, 'receiver_email' => $receiver) // conds
 126+ );
 127+
 128+ $row = $dbr->fetchRow($res);
 129+
 130+ $oldRevId = $row[rev_id];
 131+
 132+ if(! $oldRevId) {
 133+ $oldRevId = 0;
 134+ } elseif ($oldRevId == $revId) {
 135+ $oldRevId = -2;
 136+ }
 137+
 138+ return $oldRevId;
 139+}
 140+
 141+public static function getNotificationDiffHtml($oldRevId, $revId) {
 142+ $oldRevisionObj = Revision::newFromId($oldRevId);
 143+ $newRevisionObj = Revision::newFromId($revId);
 144+
 145+ if($oldRevisionObj->getTitle() != $newRevisionObj->getTitle()) {
 146+ return '<span class="error">' . wfMsg('revs-not-from-same-title') . '</span>';
 147+ }
 148+
 149+ $titleObj = $oldRevisionObj->getTitle();
 150+
 151+ $differenceEngineObj = new DifferenceEngine($titleObj, $oldRevId, $revId);
 152+
 153+ $notificationDiffHtml = '<style media="screen" type="text/css">' . Notificator::getDiffCss() . '</style><table class="diff">
 154+<col class="diff-marker" />
 155+<col class="diff-content" />
 156+<col class="diff-marker" />
 157+<col class="diff-content" />
 158+' . $differenceEngineObj->getDiffBody() . '
 159+</table>';
 160+
 161+ return $notificationDiffHtml;
 162+}
 163+
 164+public static function sendNotificationMail($receiver, $mailSubject, $notificationText) {
 165+ global $ngFromAddress;
 166+ $headers = 'From: ' . $ngFromAddress . "\r\n" .
 167+ 'X-Mailer: PHP/' . phpversion() . "\r\n" .
 168+ 'MIME-Version: 1.0' . "\r\n" .
 169+ 'Content-type: text/html; charset=utf-8' . "\r\n";
 170+ $encodedMailSubject = mb_encode_mimeheader($mailSubject,"UTF-8", "B", "\n");
 171+
 172+ return mail($receiver, $encodedMailSubject, $notificationText, $headers);
 173+}
 174+
 175+public static function recordNotificationInDatabase($pageId, $revId, $receiver) {
 176+ $lastNotifiedRevId = Notificator::getLastNotifiedRevId($pageId, $revId, $receiver);
 177+ if($lastNotifiedRevId > 0) {
 178+ $dbw = wfGetDB(DB_MASTER);
 179+ $res = $dbw->update('notificator', // table
 180+ array('rev_id' => (int)$revId), // vars (columns of the table)
 181+ array('page_id' => (int)$pageId, 'receiver_email' => $receiver) // conds
 182+ );
 183+ return $res;
 184+ } elseif($lastNotifiedRevId == 0) {
 185+ $dbw = wfGetDB(DB_MASTER);
 186+ $res = $dbw->insert('notificator', // table
 187+ array('page_id' => (int)$pageId,
 188+ 'rev_id' => (int)$revId,
 189+ 'receiver_email' => $receiver) // "$a"
 190+ );
 191+ return $res;
 192+ } elseif($lastNotifiedRevId < 0) {
 193+ return false;
 194+ }
 195+}
 196+
 197+public static function getReturnToText($linkToPage, $pageTitle) {
 198+ return '<p style="margin-top: 2em;">' . wfMsg('return-to') . ' <a href="' . $linkToPage . '">' . $pageTitle . '</a>.';
 199+}
 200+
 201+}
Index: trunk/extensions/Notificator/diff-in-mail.css
@@ -0,0 +1,49 @@
 2+td.diff-otitle,
 3+td.diff-ntitle {
 4+ text-align: center;
 5+}
 6+td.diff-marker {
 7+ text-align: right;
 8+}
 9+td.diff-lineno {
 10+ font-weight: bold;
 11+}
 12+td.diff-addedline {
 13+ background: #cfc;
 14+ font-size: smaller;
 15+}
 16+td.diff-deletedline {
 17+ background: #ffa;
 18+ font-size: smaller;
 19+}
 20+td.diff-context {
 21+ background: #eee;
 22+ font-size: smaller;
 23+}
 24+.diffchange {
 25+ color: red;
 26+ font-weight: bold;
 27+ text-decoration: none;
 28+ white-space: pre-wrap;
 29+ white-space: -moz-pre-wrap;
 30+}
 31+
 32+table.diff {
 33+ border: none;
 34+ width: 98%;
 35+ border-spacing: 4px;
 36+ table-layout: fixed;
 37+}
 38+table.diff td {
 39+ padding: 0;
 40+}
 41+table.diff col.diff-marker {
 42+ width: 2%;
 43+}
 44+table.diff col.diff-content {
 45+ width: 48%;
 46+}
 47+table.diff td div {
 48+ word-wrap: break-word;
 49+ overflow: auto;
 50+}
Index: trunk/extensions/Notificator/Notificator.i18n.php
@@ -0,0 +1,54 @@
 2+<?php
 3+
 4+$aliases = array();
 5+$messages = array();
 6+
 7+$aliases['en'] = array(
 8+ 'Notificator' => array( 'Notificator' ),
 9+);
 10+
 11+$messages['en'] = array(
 12+ 'notificator' => 'Notificator',
 13+ 'notificator-description' => 'Notifies someone by e-mail about changes to a page when a button on that page gets clicked.',
 14+ 'notificator-db-table-does-not-exist' => 'Database table \'notificator\' does not exist. The update.php maintenance script needs to be run before the Notificator extension can be used.',
 15+ 'e-mail-address' => 'e-mail address',
 16+ 'notify' => 'Notify', // Button label
 17+ 'notify-address-or-name' => 'Notify $1', // Button label
 18+ 'revs-not-from-same-title' => 'Revision IDs are not from the same title/article', // Unlikely to be seen by the user
 19+ 'return-to' => 'Return to', // Return to <link> - at the bottom of the special page
 20+ 'special-page-accessed-directly' => 'This special page cannot be accessed directly. It is intended to be used through a Notificator button.',
 21+ 'e-mail-address-invalid' => 'The provided e-mail address is invalid.',
 22+ 'notification-not-sent' => 'Notification <em>not</em> sent.',
 23+ 'change-tag' => 'change',
 24+ 'new-tag' => 'new',
 25+ 'notification-text-changes' => '$1 wants to notify you about the following changes to $2:',
 26+ 'notification-text-new' => '$1 wants to notify you about $2.',
 27+ 'following-e-mail-sent-to' => 'The following e-mail has been sent to <em>$1</em>:',
 28+ 'subject' => 'Subject:', // The subject line of an e-mail
 29+ 'error-sending-e-mail' => 'There was an error when sending the notification e-mail to <em>$1</em>.',
 30+ 'error-parameter-missing' => 'Error: Missing parameter.', // Unlikely to be seen by the user
 31+ 'notified-already' => '$1 has been notified about this page or page change before.',
 32+);
 33+
 34+$messages['de'] = array(
 35+ 'notificator' => 'Notificator',
 36+ 'notificator-description' => 'Sendet Benachrichtungs-Mails über Seitenänderungen, wenn ein Knopf auf der entsprechenden Seite betätigt wird.',
 37+ 'notificator-db-table-does-not-exist' => 'Datenbanktabelle \'notificator\' existiert nicht. Das update.php Maintenance Script muss ausgeführt werden, bevor die Notificator-Extension verwendet werden kann.',
 38+ 'e-mail-address' => 'E-Mail-Adresse',
 39+ 'notify' => 'Benachrichtigen',
 40+ 'notify-address-or-name' => '$1 benachrichtigen',
 41+ 'revs-not-from-same-title' => 'Revision-IDs gehören nicht zum selben Titel/Artikel',
 42+ 'return-to' => 'Zurück zu',
 43+ 'special-page-accessed-directly' => 'Auf diese Spezial-Seite kann nicht direkt zugegriffen werden. Sie sollte über einen Notificator-Knopf verwendet werden.',
 44+ 'e-mail-address-invalid' => 'Die angegebene E-Mail-Adresse ist ungültig.',
 45+ 'notification-not-sent' => 'Benachrichtigung <em>nicht</em> gesendet.',
 46+ 'change-tag' => 'Änderung',
 47+ 'new-tag' => 'Neu',
 48+ 'notification-text-changes' => '$1 möchte Sie zu folgenden Änderungen an $2 benachrichtigen:',
 49+ 'notification-text-new' => '$1 möchte Sie zu $2 benachrichtigen.',
 50+ 'following-e-mail-sent-to' => 'Folgende E-Mail wurde an <em>$1</em> gesendet:',
 51+ 'subject' => 'Betreff:',
 52+ 'error-sending-e-mail' => 'Beim Versenden der Benachrichtigungs-Mail an <em>$1</em> ist ein Fehler aufgetreten.',
 53+ 'error-parameter-missing' => 'Fehler: Fehlender Parameter.',
 54+ 'notified-already' => '$1 wurde bereits zu dieser Seite oder Seitenänderung benachrichtigt.',
 55+);
\ No newline at end of file
Index: trunk/extensions/Notificator/Notificator.php
@@ -0,0 +1,51 @@
 2+<?php
 3+
 4+if (!defined('MEDIAWIKI')) {
 5+ echo "Not a valid entry point";
 6+ exit(1);
 7+}
 8+
 9+$wgExtensionCredits['parserhook'][] = array(
 10+ 'path' => __FILE__, // Magic so that svn revision number can be shown
 11+ 'name' => 'Notificator',
 12+ 'descriptionmsg' => 'notificator-description',
 13+ 'version' => '1.0.2',
 14+ 'author' => 'Patrick Nagel',
 15+ 'url' => "http://www.mediawiki.org/wiki/Extension:Notificator",
 16+);
 17+
 18+$wgAutoloadClasses['Notificator'] = dirname(__FILE__) . '/Notificator.body.php';
 19+$wgAutoloadClasses['SpecialNotificator'] = dirname(__FILE__) . '/SpecialNotificator.php';
 20+
 21+$wgHooks['LoadExtensionSchemaUpdates'][] = 'notificator_AddDatabaseTable';
 22+$wgHooks['ParserTestTables'][] = 'notificator_ParserTestTables';
 23+$wgHooks['ParserFirstCallInit'][] = 'notificator_Setup';
 24+$wgHooks['LanguageGetMagic'][] = 'notificator_Magic';
 25+
 26+$wgSpecialPages['Notificator'] = 'SpecialNotificator';
 27+
 28+$wgExtensionMessagesFiles['Notificator'] = dirname( __FILE__ ) . '/Notificator.i18n.php';
 29+
 30+global $wgPasswordSender, $ngFromAddress;
 31+if(! $ngFromAddress) $ngFromAddress = $wgPasswordSender;
 32+
 33+function notificator_AddDatabaseTable() {
 34+ global $wgExtNewTables;
 35+ $wgExtNewTables[] = array('notificator', dirname( __FILE__ ) . '/Notificator.sql');
 36+ return true;
 37+}
 38+
 39+function notificator_ParserTestTables(&$tables) {
 40+ $tables[] = 'notificator';
 41+ return true;
 42+}
 43+
 44+function notificator_Setup(&$parser) {
 45+ $parser->setFunctionHook('notificator', 'Notificator::notificator_Render');
 46+ return true;
 47+}
 48+
 49+function notificator_Magic(&$magicWords, $langCode) {
 50+ $magicWords['notificator'] = array( 0, 'notificator' );
 51+ return true;
 52+}
Index: trunk/extensions/Notificator/SpecialNotificator.php
@@ -0,0 +1,74 @@
 2+<?php
 3+
 4+if (!defined('MEDIAWIKI')) {
 5+ echo "Not a valid entry point";
 6+ exit(1);
 7+}
 8+
 9+class SpecialNotificator extends SpecialPage {
 10+
 11+function __construct() {
 12+ parent::__construct( 'Notificator' );
 13+ wfLoadExtensionMessages('Notificator');
 14+}
 15+
 16+function execute($par) {
 17+ global $wgRequest, $wgOut, $wgUser;
 18+
 19+ $this->setHeaders();
 20+
 21+ # Get request data from, e.g.
 22+ $pageId = $wgRequest->getText('pageId');
 23+ $revId = $wgRequest->getText('revId');
 24+ $receiver = $wgRequest->getText('receiver');
 25+
 26+ if(! $pageId || ! $revId || ! $receiver ) {
 27+ $output = '<span class="error">' . wfMsg('special-page-accessed-directly') . '</span>';
 28+ } else {
 29+ $titleObj = Title::newFromID($pageId);
 30+ $pageTitle = $titleObj->getFullText();
 31+ $linkToPage = $titleObj->getFullURL();
 32+
 33+ if(! Notificator::checkEmailAddress($receiver)) {
 34+ $output = '<span class="error">' . wfMsg('e-mail-address-invalid') . ' ' . wfMsg('notification-not-sent') . '</span>';
 35+ $output .= Notificator::getReturnToText($linkToPage, $pageTitle);
 36+ $wgOut->addHTML($output);
 37+ return;
 38+ }
 39+
 40+ $oldRevId = Notificator::getLastNotifiedRevId($pageId, $revId, $receiver);
 41+
 42+ if($oldRevId >= 0) {
 43+ if($oldRevId > 0) {
 44+ // Receiver has been notified before - send the diff to the last notified revision
 45+ $mailSubjectPrefix = '[' . wfMsg('change-tag') . '] ';
 46+
 47+ $wgOut->addModules('mediawiki.legacy.diff');
 48+ $diff = Notificator::getNotificationDiffHtml($oldRevId, $revId);
 49+ $notificationText = wfMsg('notification-text-changes', htmlspecialchars($wgUser->getName()), '<a href="' . $linkToPage . '">' . $pageTitle . '</a>') . '<div style="margin-top: 1em;">' . $diff . '</div>';
 50+ } else {
 51+ // Receiver has never been notified about this page - so don't send a diff, just the link
 52+ $mailSubjectPrefix = '[' . wfMsg('new-tag') . '] ';
 53+ $notificationText = wfMsg('notification-text-new', htmlspecialchars($wgUser->getName()), '<a href="' . $linkToPage . '">' . $pageTitle . '</a>');
 54+ }
 55+ $mailSubject = htmlspecialchars($mailSubjectPrefix . $pageTitle);
 56+
 57+ if (Notificator::sendNotificationMail($receiver, $mailSubject, $notificationText)) {
 58+ $output = '<strong>' . wfMsg('following-e-mail-sent-to', htmlspecialchars($receiver)) . '</strong><div style="margin-top: 1em;"><h3>' . wfMsg('subject') . ' ' . $mailSubject . '</h3><p>' . $notificationText . '</p></div>';
 59+ Notificator::recordNotificationInDatabase($pageId, $revId, $receiver);
 60+ } else {
 61+ $output = '<span class="error">' . wfMsg('error-sending-e-mail', htmlspecialchars($receiver)) . '</span>';
 62+ }
 63+ } elseif($oldRevId == -1) {
 64+ $output = '<span class="error">' . wfMsg('error-parameter-missing') . '</span>';
 65+ } elseif($oldRevId == -2) {
 66+ $output = '<strong>' . wfMsg('notified-already', htmlspecialchars($receiver)) . ' ' . wfMsg('notification-not-sent') . '</strong>';
 67+ }
 68+
 69+ $output .= Notificator::getReturnToText($linkToPage, $pageTitle);
 70+ }
 71+
 72+ $wgOut->addHTML($output);
 73+}
 74+
 75+}

Follow-up revisions

RevisionCommit summaryAuthorDate
r89714removing wfLoadExtensionMessages() call, backwards-compatibility not needed, ...nagelp02:18, 8 June 2011
r89715Changed $ngFromAddress init. Using $wgPasswordSenderName and $wgPasswordSende...nagelp03:01, 8 June 2011
r89716Replaced Notificator::checkEmailAddress() with MW's Sanitizer::validateEmail()nagelp03:34, 8 June 2011
r89719Switched to Notificator::checkEmailAddress() again, after discovering that Sa...nagelp04:50, 8 June 2011
r94859Followup to r89637 - replaced function getDiffCss() with a call to file_get_c...nagelp03:28, 18 August 2011
r94861Followup to r89637 - removed valid entry point checks from files that only co...nagelp03:31, 18 August 2011

Comments

#Comment by P858snake (talk | contribs)   09:33, 7 June 2011

I believe you need to set up your svn autoprops for svn:eol style

#Comment by Patrick Nagel (talk | contribs)   09:49, 7 June 2011

Oh, right! I did that on the other box, but forgot to do it here.

Hope I fixed that correctly now...

#Comment by Nikerabbit (talk | contribs)   12:21, 7 June 2011

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?

#Comment by Patrick Nagel (talk | contribs)   05:09, 8 June 2011

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?

#Comment by Hashar (talk | contribs)   12:38, 14 August 2011

Revisions above manually added as follows-up.

#Comment by Hashar (talk | contribs)   12:37, 14 August 2011

nikerabbit, do you mind reviewing this extension again ? :-)

#Comment by Nikerabbit (talk | contribs)   10:44, 17 August 2011
+if (!defined('MEDIAWIKI')) {

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?

#Comment by Nikerabbit (talk | contribs)   10:44, 17 August 2011

Ignore my comment about stylize.php, it was already done later.

#Comment by Patrick Nagel (talk | contribs)   11:03, 17 August 2011

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?

#Comment by Nikerabbit (talk | contribs)   09:46, 21 August 2011

You can use CSSMin::minify and add an extra rule which overrides the background color?

#Comment by Patrick Nagel (talk | contribs)   06:03, 31 August 2011

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?

#Comment by Nikerabbit (talk | contribs)   07:33, 31 August 2011

Good enough for me.

Status & tagging log