Phuedx has uploaded a new change for review.

  https://gerrit.wikimedia.org/r/316548

Change subject: [Hygiene] Extract minimal context interface
......................................................................

[Hygiene] Extract minimal context interface

As noted in the DocBlock of
MobileFrontend\ResponsiveImages\Hooks#__construct, MobileContext is
overly broad and complex. This complexity leaks into the tests of
those classes that depend on MobileContext as well.

By defining an interface for the context that
MobileFrontend\ResponsiveImages\Hooks depends on, we make explicit the
state that it depends on and ease testing by not directly depending on
MobileContext.

Changes:
* Add the MobileFrontend\ResponsiveImages\Context interface and make
  MobileContext implement it.
* Simplify the suite of unit tests for
  MobileFrontend\ResponsiveImages\Hooks.

Change-Id: Id4ed144890438ecccdf8159f6ad7035ffbc36d66
---
M extension.json
M includes/MobileContext.php
A includes/ResponsiveImages/Context.php
M includes/ResponsiveImages/Hooks.php
M tests/phpunit/ResponsiveImages/HooksTest.php
5 files changed, 67 insertions(+), 69 deletions(-)


  git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/MobileFrontend 
refs/changes/48/316548/1

diff --git a/extension.json b/extension.json
index 3a6913f..2985184 100644
--- a/extension.json
+++ b/extension.json
@@ -92,7 +92,8 @@
                "MobileFrontend\\Devices\\CustomHeaderDeviceDetector": 
"includes/devices/CustomHeaderDeviceDetector.php",
                "MobileFrontend\\Devices\\UADeviceDetector": 
"includes/devices/UADeviceDetector.php",
                "MobileFrontend\\Devices\\DeviceDetectorService": 
"includes/devices/DeviceDetectorService.php",
-               "MobileFrontend\\ResponsiveImages\\Hooks": 
"includes/ResponsiveImages/Hooks.php"
+               "MobileFrontend\\ResponsiveImages\\Hooks": 
"includes/ResponsiveImages/Hooks.php",
+               "MobileFrontend\\ResponsiveImages\\Context": 
"includes/ResponsiveImages/Context.php"
        },
        "ResourceModules": {
                "skins.minerva.base.reset": {
diff --git a/includes/MobileContext.php b/includes/MobileContext.php
index 52823d2..647f1ce 100644
--- a/includes/MobileContext.php
+++ b/includes/MobileContext.php
@@ -5,11 +5,12 @@
 
 use MediaWiki\MediaWikiServices;
 use MobileFrontend\Devices\DeviceDetectorService;
+use MobileFrontend\ResponsiveImages\Context as ResponsiveImagesContext;
 
 /**
  * Provide various request-dependant methods to use in mobile context
  */
-class MobileContext extends ContextSource {
+class MobileContext extends ContextSource implements ResponsiveImagesContext {
        const USEFORMAT_COOKIE_NAME = 'mf_useformat';
        const USER_MODE_PREFERENCE_NAME = 'mfMode';
        const LAZY_LOAD_IMAGES_COOKIE_NAME = 'mfLazyLoadImages';
@@ -1127,7 +1128,8 @@
         */
        public function shouldStripResponsiveImages() {
                if ( $this->stripResponsiveImagesOverride === null ) {
-                       return $this->getMFConfig()->get( 
'MFStripResponsiveImages' );
+                       return $this->shouldDisplayMobileView()
+                               && $this->getMFConfig()->get( 
'MFStripResponsiveImages' );
                } else {
                        return $this->stripResponsiveImagesOverride;
                }
diff --git a/includes/ResponsiveImages/Context.php 
b/includes/ResponsiveImages/Context.php
new file mode 100644
index 0000000..90c2629
--- /dev/null
+++ b/includes/ResponsiveImages/Context.php
@@ -0,0 +1,15 @@
+<?php
+
+namespace MobileFrontend\ResponsiveImages;
+
+interface Context {
+
+       /**
+        * Gets whether the stripping of responsive image variants is enabled.
+        *
+        * @note This method is named `shouldStripResponsiveImages` and not
+        *  `shouldStripResponsiveImageVariants` for compatibility with
+        *  `MobileContext`.
+        */
+       function shouldStripResponsiveImages();
+}
diff --git a/includes/ResponsiveImages/Hooks.php 
b/includes/ResponsiveImages/Hooks.php
index 0977d34..72c096f 100644
--- a/includes/ResponsiveImages/Hooks.php
+++ b/includes/ResponsiveImages/Hooks.php
@@ -47,7 +47,7 @@
        }
 
        /**
-        * @var MobileContext $context
+        * @var Context $context
         */
        private $context;
 
@@ -58,12 +58,7 @@
         */
        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 ) 
{
+       public function __construct( Context $context, array $whitelist ) {
                $this->context = $context;
                $this->whitelist = $whitelist;
        }
@@ -84,7 +79,7 @@
         * @param array &$forOptions
         */
        public function doPageRenderingHash( &$confstr, User $user, 
&$forOptions ) {
-               if ( $this->shouldStripResponsiveImages() ) {
+               if ( $this->context->shouldStripResponsiveImages() ) {
                        $confstr .= '!responsiveimages=0';
                }
        }
@@ -105,22 +100,12 @@
                &$attribs,
                &$linkAttribs
        ) {
-               if ( $this->shouldStripResponsiveImages() ) {
+               if ( $this->context->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/ResponsiveImages/HooksTest.php 
b/tests/phpunit/ResponsiveImages/HooksTest.php
index 6d3dbba..320ab14 100644
--- a/tests/phpunit/ResponsiveImages/HooksTest.php
+++ b/tests/phpunit/ResponsiveImages/HooksTest.php
@@ -4,58 +4,55 @@
 
 use MediaWikiTestCase;
 use User;
-use MobileContext;
 use MobileFrontend\ResponsiveImages\Hooks;
+use MobileFrontend\ResponsiveImages\Context;
 use ThumbnailImage;
+
+class StubContext implements Context {
+       public function __construct( $shouldStripResponsiveImages ) {
+               $this->shouldStripResponsiveImages = 
$shouldStripResponsiveImages;
+       }
+
+       public function shouldStripResponsiveImages() {
+               return $this->shouldStripResponsiveImages;
+       }
+}
 
 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';
-               }
-
+       public function testDoPageRenderingHash() {
+               $confstr = '';
                $user = new User();
                $forOptions = [];
 
-               $this->context->setForceMobileView( $forceMobileView );
-               $this->context->setStripResponsiveImages( 
$stripResponsiveImages );
+               $this->factoryHooks( true )
+                       ->doPageRenderingHash( $confstr, $user, $forOptions );
 
-               $this->hooks->doPageRenderingHash( $confstr, $user, $forOptions 
);
+               $this->assertEquals( '!responsiveimages=0', $confstr );
 
-               $this->assertEquals( $expectedConfstr, $confstr );
+               $confstr = '';
+
+               $this->factoryHooks( false )
+                       ->doPageRenderingHash( $confstr, $user, $forOptions );
+
+               $this->assertEquals(
+                       '',
+                       $confstr,
+                       'It handles the stripping of responsive image variants 
from the parser out being disabled.'
+               );
        }
 
-       public static function provideDoPageRenderingHash() {
-               return [
-                       [ true, true, true ],
-                       [ false, true, false ],
-                       [ false, false, true ],
-               ];
+       /**
+        * @return Hooks
+        */
+       private function factoryHooks( $shouldStripResponsiveImages ) {
+               $context = new StubContext( $shouldStripResponsiveImages );
+               $whitelist = [ 'image/svg+xml' ];
+
+               return new Hooks( $context, $whitelist );
        }
 
        /**
@@ -67,9 +64,6 @@
                $mimeType,
                $stripResponsiveImages = true
        ) {
-               $this->context->setForceMobileView( true );
-               $this->context->setStripResponsiveImages( 
$stripResponsiveImages );
-
                $file = $mimeType ? $this->factoryFile( $mimeType ) : null;
                $thumbnail = new ThumbnailImage(
                        $file,
@@ -89,11 +83,12 @@
 
                $linkAttribs = [];
 
-               $this->hooks->doThumbnailBeforeProduceHTML(
-                       $thumbnail,
-                       $attribs,
-                       $linkAttribs
-               );
+               $this->factoryHooks( $stripResponsiveImages )
+                       ->doThumbnailBeforeProduceHTML(
+                               $thumbnail,
+                               $attribs,
+                               $linkAttribs
+                       );
 
                $this->assertEquals( $expected, array_key_exists( 'srcset', 
$attribs ) );
        }
@@ -108,8 +103,8 @@
                        // 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.
+                       // It handles the stripping of responsive images 
variants from the parser
+                       // output being disabled.
                        [ true, 'image/jpg', false ],
                ];
        }

-- 
To view, visit https://gerrit.wikimedia.org/r/316548
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Id4ed144890438ecccdf8159f6ad7035ffbc36d66
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

Reply via email to