From 5e2da7627b41fc93f2cd8e9a25aa54736bc8be77 Mon Sep 17 00:00:00 2001 From: Kunal Mehta Date: Wed, 5 Aug 2015 12:41:12 -0700 Subject: [PATCH] Refactor and unify icon URL logic The logic to get the URL for an icon was duplicated in the EmailFormatter and BasicFormatter. It is now in the abstract NotificationFormatter, which EmailFormatter and BasicFormatter now use. Changes in logic: * Throw an exception if an invalid notification type is provided instead of a PHP notice * icons using 'url' may have different ltr/rtl icons * Throw exception if icon is supposed to have different icons for ltr/rtl, but doesn't, instead of debug logging The new function is static so it can be used in EmailFormatter as it does not inherit from NotificationFormatter. Bug: T60726 Change-Id: Ia3c01c35f58eed8cc2c039249ab1ec1a80a8abbb --- includes/EmailFormatter.php | 23 +------ includes/formatters/BasicFormatter.php | 22 +------ includes/formatters/NotificationFormatter.php | 45 +++++++++++++ .../formatters/NotificationFormatterTest.php | 63 +++++++++++++++++++ 4 files changed, 111 insertions(+), 42 deletions(-) diff --git a/includes/EmailFormatter.php b/includes/EmailFormatter.php index 45b6a4320..d8472b886 100644 --- a/includes/EmailFormatter.php +++ b/includes/EmailFormatter.php @@ -169,28 +169,9 @@ abstract class EchoEmailMode { * @return string */ public static function getNotifIcon( $icon ) { - global $wgEchoNotificationIcons, $wgExtensionAssetsPath, $wgLang; + global $wgLang; - $iconInfo = $wgEchoNotificationIcons[$icon]; - if ( isset( $iconInfo['url'] ) && $iconInfo['url'] ) { - $iconUrl = $iconInfo['url']; - } else { - if ( !isset( $iconInfo['path'] ) || !$iconInfo['path'] ) { - $iconInfo = $wgEchoNotificationIcons['placeholder']; - } - if ( is_array( $iconInfo['path'] ) ) { - $dir = $wgLang->getDir(); - if ( isset( $iconInfo['path'][$dir] ) ) { - $path = $iconInfo['path'][$dir]; - } else { - wfDebugLog( 'Echo', "The \"$icon\" icon does not have anything set for $dir direction." ); - $path = $wgEchoNotificationIcons['placeholder']['path']; // Fallback - } - } else { - $path = $iconInfo['path']; - } - $iconUrl = "$wgExtensionAssetsPath/$path"; - } + $iconUrl = EchoNotificationFormatter::getIconUrl( $icon, $wgLang->getDir() ); return wfExpandUrl( $iconUrl, PROTO_CANONICAL ); } diff --git a/includes/formatters/BasicFormatter.php b/includes/formatters/BasicFormatter.php index d973acbaa..9b2755b4b 100644 --- a/includes/formatters/BasicFormatter.php +++ b/includes/formatters/BasicFormatter.php @@ -181,27 +181,7 @@ class EchoBasicFormatter extends EchoNotificationFormatter { return $this->formatNotificationTitle( $event, $user )->text(); } - $iconInfo = $wgEchoNotificationIcons[$this->icon]; - if ( isset( $iconInfo['url'] ) && $iconInfo['url'] ) { - $iconUrl = $iconInfo['url']; - } else { - if ( !isset( $iconInfo['path'] ) || !$iconInfo['path'] ) { - // Fallback in case icon is not configured; mainly intended for 'site' - $iconInfo = $wgEchoNotificationIcons['placeholder']; - } - if ( is_array( $iconInfo['path'] ) ) { - $dir = $wgLang->getDir(); - if ( isset( $iconInfo['path'][$dir] ) ) { - $path = $iconInfo['path'][$dir]; - } else { - wfDebugLog( 'Echo', "The \"{$this->icon}\" icon does not have anything set for $dir direction." ); - $path = $wgEchoNotificationIcons['placeholder']['path']; // Fallback - } - } else { - $path = $iconInfo['path']; - } - $iconUrl = "$wgExtensionAssetsPath/$path"; - } + $iconUrl = $this->getIconUrl( $this->icon, $wgLang->getDir() ); // Assume html as the format for the notification $output = Html::element( diff --git a/includes/formatters/NotificationFormatter.php b/includes/formatters/NotificationFormatter.php index 3bd0d89b5..0230b383e 100644 --- a/includes/formatters/NotificationFormatter.php +++ b/includes/formatters/NotificationFormatter.php @@ -119,6 +119,51 @@ abstract class EchoNotificationFormatter { return $ts; } + /** + * @todo this shouldn't be static + * @param string $icon Name of icon as registered in BeforeCreateEchoEvent hook + * @param string $dir either 'ltr' or 'rtl' + * @return string + */ + public static function getIconUrl( $icon, $dir ) { + global $wgEchoNotificationIcons, $wgExtensionAssetsPath; + if ( !isset( $wgEchoNotificationIcons[$icon] ) ) { + throw new InvalidArgumentException( "The $icon icon is not registered" ); + } + + $iconInfo = $wgEchoNotificationIcons[$icon]; + $needsPrefixing = true; + + // Now we need to check it has a valid url/path + if ( isset( $iconInfo['url'] ) && $iconInfo['url'] ) { + $iconUrl = $iconInfo['url']; + $needsPrefixing = false; + } elseif ( isset( $iconInfo['path'] ) && $iconInfo['path'] ) { + $iconUrl = $iconInfo['path']; + } else { + // Fallback to hardcoded 'placeholder'. This is used if someone + // doesn't configure the 'site' icon for example. + $icon = 'placeholder'; + $iconUrl = $wgEchoNotificationIcons['placeholder']['path']; + } + + // Might be an array with different icons for ltr/rtl + if ( is_array( $iconUrl ) ) { + if ( !isset( $iconUrl[$dir] ) ) { + throw new UnexpectedValueException( "Icon type $icon doesn't have an icon for $dir directionality" ); + } + + $iconUrl = $iconUrl[$dir]; + } + + // And if it was a 'path', stick the assets path in front + if ( $needsPrefixing ) { + $iconUrl = "$wgExtensionAssetsPath/$iconUrl"; + } + + return $iconUrl; + } + /** * Returns a revision snippet * @param EchoEvent $event The event that the notification is for. diff --git a/tests/phpunit/formatters/NotificationFormatterTest.php b/tests/phpunit/formatters/NotificationFormatterTest.php index dd0024148..26d86727a 100644 --- a/tests/phpunit/formatters/NotificationFormatterTest.php +++ b/tests/phpunit/formatters/NotificationFormatterTest.php @@ -247,4 +247,67 @@ class EchoNotificationFormatterTest extends MediaWikiTestCase { } return $event; } + + /** + * @dataProvider provideGetIconUrl + */ + public function testGetIconUrl( $global, $icon, $dir, $expected, $exception = null ) { + $this->setMwGlobals( array( + 'wgEchoNotificationIcons' => $global, + 'wgExtensionAssetsPath' => 'http://example.org' + ) ); + + if ( $exception ) { + $this->setExpectedException( $exception ); + } + + $url = EchoNotificationFormatter::getIconUrl( $icon, $dir ); + $this->assertEquals( $expected, $url ); + } + + public static function provideGetIconUrl() { + $standard = array( + 'foo' => array( + 'path' => 'foo.png', + ), + 'bar' => array( + 'path' => array( + 'ltr' => 'bar.png' + ), + ), + 'baz' => array( + 'path' => array( + 'ltr' => 'baz-ltr.png', + 'rtl' => 'baz-rtl.png', + ), + ), + 'site' => array( + 'url' => false, + ), + 'placeholder' => array( + 'path' => 'placeholder.png', + ), + ); + $tests = array( + // Standard, no ltr/rtl + array( $standard, 'foo', 'ltr', 'http://example.org/foo.png' ), + array( $standard, 'foo', 'rtl', 'http://example.org/foo.png' ), + // Only ltr configured (bad!) + array( $standard, 'bar', 'ltr', 'http://example.org/bar.png' ), + array( $standard, 'bar', 'rtl', '', 'UnexpectedValueException' ), + // Different for ltr/rtl + array( $standard, 'baz', 'ltr', 'http://example.org/baz-ltr.png' ), + array( $standard, 'baz', 'rtl', 'http://example.org/baz-rtl.png' ), + // Not registered + array( $standard, 'invalid', 'ltr', '', 'InvalidArgumentException' ), + // No url set, fallback to placeholder + array( $standard, 'site', 'ltr', 'http://example.org/placeholder.png' ), + ); + + // Set the 'url', and it doesn't fallback anymore + $standard['site']['url'] = 'http://example.com/site.png'; + $tests[] = array( $standard, 'site', 'ltr', 'http://example.com/site.png' ); + + return $tests; + } }