jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/317134 )
Change subject: Hygiene: Simplify MobileContext::shouldStripResponsiveImages
......................................................................
Hygiene: Simplify MobileContext::shouldStripResponsiveImages
MobileContext#shouldStripResponsiveImages should return false if
MobileContext#shouldDisplayMobileView returns false, which is captured
in MobileFrontend\ResponsiveImages\Hooks#shouldStripResponsiveImages.
Moving this dependency into the definition of
Changes:
* Make the result of MobileContext#shouldStripResponsiveImages depend on
the result of #shouldDisplayMobileView.
* Add unit tests for MobileContext#shouldStripResponsiveImages.
* Add unit tests for MobileFrontendHooks::onThumbnailBeforeProduceHTML.
Change-Id: If114aab998627001ea0390fd4014d7f3ea89eab3
---
M includes/MobileContext.php
M includes/MobileFrontend.hooks.php
M tests/phpunit/MobileContextTest.php
M tests/phpunit/MobileFrontend.hooksTest.php
4 files changed, 135 insertions(+), 30 deletions(-)
Approvals:
Pmiazga: Looks good to me, approved
jenkins-bot: Verified
diff --git a/includes/MobileContext.php b/includes/MobileContext.php
index 242a1ae..330a39a 100644
--- a/includes/MobileContext.php
+++ b/includes/MobileContext.php
@@ -1159,7 +1159,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/MobileFrontend.hooks.php
b/includes/MobileFrontend.hooks.php
index ab43d74..70eec12 100644
--- a/includes/MobileFrontend.hooks.php
+++ b/includes/MobileFrontend.hooks.php
@@ -367,20 +367,26 @@
}
/**
- * PageRenderingHash hook handler
- * @see https://www.mediawiki.org/wiki/Manual:Hooks/PageRenderingHash
+ * Varies the parser cache if responsive images should have their
variants
+ * stripped from the parser output, since the transformation happens
during
+ * the parse.
*
- * @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
+ * 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
+ *
+ * @param string &$confstr Reference to the parser cache key
+ * @param User $user The user that is requesting the page
+ * @param array &$forOptions The options used to generate the parser
cache key
*/
public static function onPageRenderingHash( &$confstr, User $user,
&$forOptions ) {
- $context = MobileContext::singleton();
-
- if (
- $context->shouldDisplayMobileView()
- && $context->shouldStripResponsiveImages()
- ) {
+ if ( MobileContext::singleton()->shouldStripResponsiveImages()
) {
$confstr .= '!responsiveimages=0';
}
}
@@ -1132,19 +1138,22 @@
}
/**
- * Omit srcset attributes from thumbnail image tags, to conserve
bandwidth.
+ * Removes the responsive image's variants from the parser output if
+ * configured to do so and the thumbnail's MIME type isn't whitelisted.
*
- * @param ThumbnailImage $thumbnail
- * @param array &$attribs
- * @param array &$linkAttribs
+ * See
https://www.mediawiki.org/wiki/Manual:Hooks/ThumbnailBeforeProduceHTML
+ * for more detail about the `ThumbnailBeforeProduceHTML` hook.
+ *
+ * @param ThumbnailImage $thumbnail The thumbnail
+ * @param array &$attribs The attributes of the DOMElement being
contructed
+ * to represent the thumbnail
+ * @param array &$linkAttribs The attributes of the DOMElement being
+ * constructed to represent the link to original file
*/
public static function onThumbnailBeforeProduceHTML( $thumbnail,
&$attribs, &$linkAttribs ) {
$context = MobileContext::singleton();
$config = $context->getMFConfig();
- if (
- $context->shouldDisplayMobileView() &&
- $context->shouldStripResponsiveImages()
- ) {
+ if ( $context->shouldStripResponsiveImages() ) {
$file = $thumbnail->getFile();
if ( !$file || !in_array( $file->getMimeType(),
$config->get(
'MFResponsiveImageWhitelist' ) ) ) {
diff --git a/tests/phpunit/MobileContextTest.php
b/tests/phpunit/MobileContextTest.php
index 5a37dc6..6a6a716 100644
--- a/tests/phpunit/MobileContextTest.php
+++ b/tests/phpunit/MobileContextTest.php
@@ -609,4 +609,36 @@
[ true, true ],
];
}
+
+ /**
+ * @dataProvider provideShouldStripResponsiveImages
+ * @covers MobileContext::shouldStripResponsiveImages
+ */
+ public function testShouldStripResponsiveImages(
+ $expected,
+ $forceMobileView,
+ $wgMFStripResponsiveImages,
+ $stripResponsiveImages = null
+ ) {
+ $context = MobileContext::singleton();
+ $context->setForceMobileView( $forceMobileView );
+
+ $this->setMwGlobals(
+ 'wgMFStripResponsiveImages',
+ $wgMFStripResponsiveImages
+ );
+
+ $context->setStripResponsiveImages( $stripResponsiveImages );
+
+ $this->assertEquals( $expected,
$context->shouldStripResponsiveImages() );
+ }
+
+ public static function provideShouldStripResponsiveImages() {
+ return [
+ [ true, true, true ],
+ [ false, true, false ],
+ [ false, false, true ],
+ [ false, true, true, false ],
+ ];
+ }
}
diff --git a/tests/phpunit/MobileFrontend.hooksTest.php
b/tests/phpunit/MobileFrontend.hooksTest.php
index 798fb4c..c5bd185 100644
--- a/tests/phpunit/MobileFrontend.hooksTest.php
+++ b/tests/phpunit/MobileFrontend.hooksTest.php
@@ -210,19 +210,12 @@
/**
* @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 = '';
@@ -241,9 +234,79 @@
public static function provideOnPageRenderingHash() {
return [
- [ true, true, true ],
- [ false, true, false ],
- [ false, false, true ],
+ [ true, true ],
+ [ false, false ],
+ ];
+ }
+
+ /**
+ * @dataProvider provideDoThumbnailBeforeProduceHTML
+ * @covers MobileFrontendHooks::onPageRenderingHash
+ */
+ public function testDoThumbnailBeforeProduceHTML(
+ $expected,
+ $mimeType,
+ $stripResponsiveImages = true
+ ) {
+ $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
+ ]
+ );
+
+ MobileContext::singleton()->setStripResponsiveImages(
$stripResponsiveImages );
+
+ // We're only asserting that the `srcset` attribute is unset.
+ $attribs = [ 'srcset' => 'bar' ];
+
+ $linkAttribs = [];
+
+ MobileFrontendHooks::onThumbnailBeforeProduceHTML(
+ $thumbnail,
+ $attribs,
+ $linkAttribs
+ );
+
+ $this->assertEquals( $expected, array_key_exists( 'srcset',
$attribs ) );
+ }
+
+ /**
+ * 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;
+ }
+
+ 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 image
variants from the parser
+ // output being disabled.
+ [ true, 'image/jpg', false ],
];
}
}
--
To view, visit https://gerrit.wikimedia.org/r/317134
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: If114aab998627001ea0390fd4014d7f3ea89eab3
Gerrit-PatchSet: 5
Gerrit-Project: mediawiki/extensions/MobileFrontend
Gerrit-Branch: master
Gerrit-Owner: Phuedx <samsm...@wikimedia.org>
Gerrit-Reviewer: Jdlrobson <jrob...@wikimedia.org>
Gerrit-Reviewer: Phuedx <samsm...@wikimedia.org>
Gerrit-Reviewer: Pmiazga <pmia...@wikimedia.org>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
