Phuedx has uploaded a new change for review. https://gerrit.wikimedia.org/r/316542
Change subject: [Hygiene] [QA] Extract responsive image hooks ...................................................................... [Hygiene] [QA] Extract responsive image hooks Make MobileFrontendHooks less complex by removing two related static hook handlers from it and make those handlers more easily testable by migrating them using the guidelines in docs/injection.txt [0]. This follows on from I343e5da5. Changes: * Move the cautionary note about about stripping responsive image variants into the README. * Move MobileFrontendHooks::onPageRenderingHash and MobileFrontend\\ResponsiveImages\\Hooks#doPageRenderingHash and * Move MobileFrontendHooks::onThumbnailBeforeProduceHTML to MobileFrontend\\ResponsiveImages\\Hooks#doThumbnailBeforeProduceHTML and cover the method with unit tests. [0]: https://github.com/wikimedia/mediawiki/blob/d7410db0fdf99dab9dcd4244e608383016e2dfb1/docs/injection.txt#L212-L224 Change-Id: I04e927e3a3c903839d62ac9f8156c53f89cc644e --- M README.md M extension.json M includes/MobileFrontend.hooks.php A includes/ResponsiveImages/Hooks.php M tests/phpunit/MobileFrontend.hooksTest.php A tests/phpunit/ResponsiveImages/HooksTest.php 6 files changed, 269 insertions(+), 93 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/MobileFrontend refs/changes/42/316542/1 diff --git a/README.md b/README.md index 6439c3b..f315434 100644 --- a/README.md +++ b/README.md @@ -702,9 +702,13 @@ #### $wgMFResponsiveImageWhitelist -Whitelist of source file mime types to retain srcset attributes on when using -$wgMFStripResponsiveImages. Defaults to allow rasterized SVGs since they -usually are diagrams that compress well and benefit from the higher resolution. +Whitelist of source file mime types to retain srcset attributes on when +`$wgMFStripResponsiveImages` is truthy. Defaults to allow rasterized SVGs since +they usually are diagrams that compress well and benefit from the higher +resolution. + +In future, the `srcset` attribute may include small-screen-friendly image +variants as well as density variants, so this should be used with caution. * Type: `Array` * Default: diff --git a/extension.json b/extension.json index efa101f..3a6913f 100644 --- a/extension.json +++ b/extension.json @@ -91,7 +91,8 @@ "MobileFrontend\\Devices\\AMFDeviceDetector": "includes/devices/AMFDeviceDetector.php", "MobileFrontend\\Devices\\CustomHeaderDeviceDetector": "includes/devices/CustomHeaderDeviceDetector.php", "MobileFrontend\\Devices\\UADeviceDetector": "includes/devices/UADeviceDetector.php", - "MobileFrontend\\Devices\\DeviceDetectorService": "includes/devices/DeviceDetectorService.php" + "MobileFrontend\\Devices\\DeviceDetectorService": "includes/devices/DeviceDetectorService.php", + "MobileFrontend\\ResponsiveImages\\Hooks": "includes/ResponsiveImages/Hooks.php" }, "ResourceModules": { "skins.minerva.base.reset": { @@ -1919,10 +1920,10 @@ "MobileFrontendHooks::onResourceLoaderGetLessVars" ], "ThumbnailBeforeProduceHTML": [ - "MobileFrontendHooks::onThumbnailBeforeProduceHTML" + "MobileFrontend\\ResponsiveImages\\Hooks::onThumbnailBeforeProduceHTML" ], "PageRenderingHash": [ - "MobileFrontendHooks::onPageRenderingHash" + "MobileFrontend\\ResponsiveImages\\Hooks::onPageRenderingHash" ], "AfterBuildFeedLinks": [ "MobileFrontendHooks::onAfterBuildFeedLinks" diff --git a/includes/MobileFrontend.hooks.php b/includes/MobileFrontend.hooks.php index 6622b17..f372cc5 100644 --- a/includes/MobileFrontend.hooks.php +++ b/includes/MobileFrontend.hooks.php @@ -366,25 +366,6 @@ } /** - * PageRenderingHash hook handler - * @see https://www.mediawiki.org/wiki/Manual:Hooks/PageRenderingHash - * - * @param string &$confstr Reference to a hash key string which can be modified - * @param User $user User object that is requesting the page - * @param array &$forOptions Array of options used to generate the $confstr hash key - */ - public static function onPageRenderingHash( &$confstr, User $user, &$forOptions ) { - $context = MobileContext::singleton(); - - if ( - $context->shouldDisplayMobileView() - && $context->shouldStripResponsiveImages() - ) { - $confstr .= '!responsiveimages=0'; - } - } - - /** * ResourceLoaderGetConfigVars hook handler * This should be used for variables which: * - vary with the html @@ -1289,34 +1270,6 @@ */ public static function onHTMLFileCache_useFileCache() { return !MobileContext::singleton()->shouldDisplayMobileView(); - } - - /** - * Omit srcset attributes from thumbnail image tags, to conserve bandwidth. - * - * @param ThumbnailImage $thumbnail - * @param array &$attribs - * @param array &$linkAttribs - */ - public static function onThumbnailBeforeProduceHTML( $thumbnail, &$attribs, &$linkAttribs ) { - $context = MobileContext::singleton(); - $config = $context->getMFConfig(); - if ( - $context->shouldDisplayMobileView() && - $context->shouldStripResponsiveImages() - ) { - $file = $thumbnail->getFile(); - if ( !$file || !in_array( $file->getMimeType(), - $config->get( 'MFResponsiveImageWhitelist' ) ) ) { - // Remove all responsive image 'srcset' attributes, except - // from SVG->PNG renderings which usually aren't too huge, - // or other whitelisted types. - // Note that in future, srcset may be used for specifying - // small-screen-friendly image variants as well as density - // variants, so this should be used with caution. - unset( $attribs['srcset'] ); - } - } } /** diff --git a/includes/ResponsiveImages/Hooks.php b/includes/ResponsiveImages/Hooks.php new file mode 100644 index 0000000..0977d34 --- /dev/null +++ b/includes/ResponsiveImages/Hooks.php @@ -0,0 +1,126 @@ +<?php + +namespace MobileFrontend\ResponsiveImages; + +use MobileContext; +use User; +use ThumbnailImage; + +class Hooks { + + /** + * @see Hooks::doPageRenderingHash + */ + public static function onPageRenderingHash( + &$confstr, + User $user, + &$forOptions + ) { + self::newFromGlobalState() + ->doPageRenderingHash( $confstr, $user, $forOptions ); + } + + /** + * @see Hooks::doThumbnailBeforeProduceHTML + */ + public static function onThumbnailBeforeProduceHTML( + ThumbnailImage $thumbnail, + &$attribs, + &$linkAttribs + ) { + self::newFromGlobalState() + ->doThumbnailBeforeProduceHTML( $thumbnail, $attribs, $linkAttribs ); + } + + /** + * Creates an instance of the `Hooks` class with the singleton instance of + * `MobileContext` so that the hook handlers can be dispatched statically. + * + * @return Hooks + */ + private static function newFromGlobalState() { + $context = MobileContext::singleton(); + $whitelist = $context->getMFConfig() + ->get( 'MFResponsiveImageWhitelist' ); + + return new self( $context, $whitelist ); + } + + /** + * @var MobileContext $context + */ + private $context; + + /** + * A list of MIME types of images that shouldn't have their variants removed. + * + * @var string[] $whitelist + */ + private $whitelist; + + /** + * @TODO Only the smallest slice of `MobileContext` is required here: + * `MobileContext#shouldStripResponsiveImages`. Create an interface that + * reflects this to increase flexibility and ease testing. + */ + public function __construct( MobileContext $context, array $whitelist ) { + $this->context = $context; + $this->whitelist = $whitelist; + } + + /** + * Varies the parser cache if responsive images should have their variants + * stripped from the parser output, since the transformation happens during + * the parse. + * + * See `$wgMFStripResponsiveImages` and `$wgMFResponsiveImageWhitelist` for + * more detail about the stripping of responsive images. + * + * See https://www.mediawiki.org/wiki/Manual:Hooks/PageRenderingHash for more + * detail about the `PageRenderingHash` hook. + * + * @param string &$confstr The parser cache key + * @param User $user User + * @param array &$forOptions + */ + public function doPageRenderingHash( &$confstr, User $user, &$forOptions ) { + if ( $this->shouldStripResponsiveImages() ) { + $confstr .= '!responsiveimages=0'; + } + } + + /** + * Removes the responsive image's variants from the parser output if + * configured to do so and the thumbnail's MIME type isn't whitelisted. + * + * See https://www.mediawiki.org/wiki/Manual:Hooks/ThumbnailBeforeProduceHTML + * for more detail about the `ThumbnailBeforeProduceHTML` hook. + * + * @param ThumbnailImage $thumbnail + * @param array &$attribs + * @param array &$linkAttribs + */ + public function doThumbnailBeforeProduceHTML( + ThumbnailImage $thumbnail, + &$attribs, + &$linkAttribs + ) { + if ( $this->shouldStripResponsiveImages() ) { + $file = $thumbnail->getFile(); + + if ( !$file || !in_array( $file->getMimeType(), $this->whitelist ) ) { + unset( $attribs['srcset'] ); + } + } + } + + /** + * Gets whether responsive images should be stripped from the parser output. + * + * @return bool + */ + private function shouldStripResponsiveImages() { + return $this->context->shouldDisplayMobileView() + && $this->context->shouldStripResponsiveImages(); + } +} diff --git a/tests/phpunit/MobileFrontend.hooksTest.php b/tests/phpunit/MobileFrontend.hooksTest.php index 361164d..e9ac88f 100644 --- a/tests/phpunit/MobileFrontend.hooksTest.php +++ b/tests/phpunit/MobileFrontend.hooksTest.php @@ -200,44 +200,4 @@ $this->assertArrayEquals( $expected, $urls ); } - - /** - * @dataProvider provideOnPageRenderingHash - * @covers MobileFrontendHooks::onPageRenderingHash - * - * @param bool $shouldConfstrChange Whether $confstr parameter should have - * changed - * @param bool $forceMobileView - * @param bool $stripResponsiveImages - */ - public function testOnPageRenderingHash( - $shouldConfstrChange, - $forceMobileView, - $stripResponsiveImages - ) { - $context = MobileContext::singleton(); - $context->setForceMobileView( $forceMobileView ); - $context->setStripResponsiveImages( $stripResponsiveImages ); - - $expectedConfstr = $confstr = ''; - - if ( $shouldConfstrChange ) { - $expectedConfstr = '!responsiveimages=0'; - } - - $user = new User(); - $forOptions = []; - - MobileFrontendHooks::onPageRenderingHash( $confstr, $user, $forOptions ); - - $this->assertEquals( $expectedConfstr, $confstr ); - } - - public static function provideOnPageRenderingHash() { - return [ - [ true, true, true ], - [ false, true, false ], - [ false, false, true ], - ]; - } } diff --git a/tests/phpunit/ResponsiveImages/HooksTest.php b/tests/phpunit/ResponsiveImages/HooksTest.php new file mode 100644 index 0000000..6d3dbba --- /dev/null +++ b/tests/phpunit/ResponsiveImages/HooksTest.php @@ -0,0 +1,132 @@ +<?php + +namespace Tests\MobileFrontend\ResponsiveImages; + +use MediaWikiTestCase; +use User; +use MobileContext; +use MobileFrontend\ResponsiveImages\Hooks; +use ThumbnailImage; + +class HooksTest extends MediaWikiTestCase { + + protected function setUp() { + parent::setUp(); + + MobileContext::resetInstanceForTesting(); + $this->context = MobileContext::singleton(); + + $whitelist = [ 'image/svg+xml' ]; + + $this->hooks = new Hooks( $this->context, $whitelist ); + } + + /** + * @dataProvider provideDoPageRenderingHash + * @covers MobileFrontend\\ResponsiveImages\\Hooks::doPageRenderingHash + * + * @param bool $shouldConfstrChange Whether the $confstr parameter should have + * changed + */ + public function testDoPageRenderingHash( + $shouldConfstrChange, + $forceMobileView, + $stripResponsiveImages + ) { + $expectedConfstr = $confstr = ''; + + if ( $shouldConfstrChange ) { + $expectedConfstr = '!responsiveimages=0'; + } + + $user = new User(); + $forOptions = []; + + $this->context->setForceMobileView( $forceMobileView ); + $this->context->setStripResponsiveImages( $stripResponsiveImages ); + + $this->hooks->doPageRenderingHash( $confstr, $user, $forOptions ); + + $this->assertEquals( $expectedConfstr, $confstr ); + } + + public static function provideDoPageRenderingHash() { + return [ + [ true, true, true ], + [ false, true, false ], + [ false, false, true ], + ]; + } + + /** + * @dataProvider provideDoThumbnailBeforeProduceHTML + * @covers MobileFrontend\\ResponsiveImages\\Hooks::doPageRenderingHash + */ + public function testDoThumbnailBeforeProduceHTML( + $expected, + $mimeType, + $stripResponsiveImages = true + ) { + $this->context->setForceMobileView( true ); + $this->context->setStripResponsiveImages( $stripResponsiveImages ); + + $file = $mimeType ? $this->factoryFile( $mimeType ) : null; + $thumbnail = new ThumbnailImage( + $file, + + // The following is stub data that stops `ThumbnailImage#__construct` + // triggering a warning. + '/foo.svg', + false, + [ + 'width' => 375, + 'height' => 667 + ] + ); + + // We're only asserting that the `srcset` attribute is unset. + $attribs = [ 'srcset' => 'bar' ]; + + $linkAttribs = []; + + $this->hooks->doThumbnailBeforeProduceHTML( + $thumbnail, + $attribs, + $linkAttribs + ); + + $this->assertEquals( $expected, array_key_exists( 'srcset', $attribs ) ); + } + + public static function provideDoThumbnailBeforeProduceHTML() { + return [ + [ false, 'image/jpg' ], + + // `ThumbnailImage#getFile` can return `null`. + [ false, null ], + + // It handles an image with a whitelisted MIME type. + [ true, 'image/svg+xml' ], + + // It handles the stripping of responsive images from the parser output + // being disabled. + [ true, 'image/jpg', false ], + ]; + } + + /** + * Creates an instance of `File` which has the given MIME type. + * + * @return File + */ + private function factoryFile( $mimeType ) { + $file = $this->getMockBuilder( 'File' ) + ->disableOriginalConstructor() + ->getMock(); + + $file->method( 'getMimeType' ) + ->willReturn( $mimeType ); + + return $file; + } +} -- To view, visit https://gerrit.wikimedia.org/r/316542 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I04e927e3a3c903839d62ac9f8156c53f89cc644e Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/MobileFrontend Gerrit-Branch: master Gerrit-Owner: Phuedx <samsm...@wikimedia.org> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
