jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/347523 )
Change subject: Sanitize gadget name
......................................................................
Sanitize gadget name
MediaWikiGadgetsDefinition does some basic gadget name sanitization
and we have to do the same when checking "is gadget enabled for user"
Changes:
- sanitize gadget name same way as
MediaWikiGadgetsDefinitionRepo::newFromDefinition() does.
- add try{} catch() when loading gadget as getGadget might throw an
exception
Bug: T160081
Change-Id: Ia7a57e9dcfa3b25129d6d2bf75795372fad2b251
---
M includes/PopupsGadgetsIntegration.php
M tests/phpunit/PopupsGadgetsIntegrationTest.php
2 files changed, 106 insertions(+), 9 deletions(-)
Approvals:
jenkins-bot: Verified
Jdlrobson: Looks good to me, approved
diff --git a/includes/PopupsGadgetsIntegration.php
b/includes/PopupsGadgetsIntegration.php
index addc0d3..763999d 100644
--- a/includes/PopupsGadgetsIntegration.php
+++ b/includes/PopupsGadgetsIntegration.php
@@ -51,9 +51,19 @@
*/
public function __construct( Config $config , ExtensionRegistry
$extensionRegistry ) {
$this->extensionRegistry = $extensionRegistry;
- $this->navPopupsGadgetName = $config->get(
self::CONFIG_NAVIGATION_POPUPS_NAME );
+ $this->navPopupsGadgetName = $this->sanitizeGadgetName(
+ $config->get( self::CONFIG_NAVIGATION_POPUPS_NAME ) );
}
+ /**
+ * Sanitize gadget name
+ *
+ * @param $gadgetName
+ * @return string
+ */
+ private function sanitizeGadgetName( $gadgetName ) {
+ return str_replace( ' ', '_', trim( $gadgetName ) );
+ }
/**
* @return bool
*/
@@ -73,7 +83,12 @@
$gadgetsRepo = \GadgetRepo::singleton();
$match = array_search( $this->navPopupsGadgetName,
$gadgetsRepo->getGadgetIds() );
if ( $match !== false ) {
- return $gadgetsRepo->getGadget(
$this->navPopupsGadgetName )->isEnabled( $user );
+ try {
+ return $gadgetsRepo->getGadget(
$this->navPopupsGadgetName )
+ ->isEnabled( $user );
+ } catch ( \InvalidArgumentException $e ) {
+ return false;
+ }
}
}
return false;
diff --git a/tests/phpunit/PopupsGadgetsIntegrationTest.php
b/tests/phpunit/PopupsGadgetsIntegrationTest.php
index c93fd68..80a1ba3 100644
--- a/tests/phpunit/PopupsGadgetsIntegrationTest.php
+++ b/tests/phpunit/PopupsGadgetsIntegrationTest.php
@@ -106,8 +106,8 @@
->method( 'getGadgetIds' )
->willReturn( [] );
- $this->executeConflictsWithNavPopupsGadgetSafeCheck( $user,
$gadgetRepoMock,
- self::GADGET_DISABLED );
+ $this->executeConflictsWithNavPopupsGadgetSafeCheck( $user,
$this->getConfigMock(),
+ $gadgetRepoMock, self::GADGET_DISABLED );
}
/**
@@ -132,7 +132,7 @@
[ 'getGadgetIds', 'getGadget' ] );
$gadgetRepoMock->expects( $this->once() )
- ->method( 'getGadgetids' )
+ ->method( 'getGadgetIds' )
->willReturn( [ self::NAV_POPUPS_GADGET_NAME ] );
$gadgetRepoMock->expects( $this->once() )
@@ -140,22 +140,104 @@
->with( self::NAV_POPUPS_GADGET_NAME )
->willReturn( $gadgetMock );
- $this->executeConflictsWithNavPopupsGadgetSafeCheck( $user,
$gadgetRepoMock,
+ $this->executeConflictsWithNavPopupsGadgetSafeCheck( $user,
$this->getConfigMock(),
+ $gadgetRepoMock, self::GADGET_ENABLED );
+ }
+
+ /**
+ * Test the edge case when GadgetsRepo::getGadget throws an exception
+ * @covers ::conflictsWithNavPopupsGadget
+ */
+ public function testConflictsWithNavPopupsGadgetWhenGadgetNotExists() {
+ $this->checkRequiredDependencies();
+
+ $user = $this->getTestUser()->getUser();
+
+ $gadgetRepoMock = $this->getMock( GadgetRepo::class,
+ [ 'getGadgetIds', 'getGadget' ] );
+
+ $gadgetRepoMock->expects( $this->once() )
+ ->method( 'getGadgetIds' )
+ ->willReturn( [ self::NAV_POPUPS_GADGET_NAME ]
);
+
+ $gadgetRepoMock->expects( $this->once() )
+ ->method( 'getGadget' )
+ ->with( self::NAV_POPUPS_GADGET_NAME )
+ ->willThrowException( new
InvalidArgumentException() );
+
+ $this->executeConflictsWithNavPopupsGadgetSafeCheck(
$user, $this->getConfigMock(),
+ $gadgetRepoMock, self::GADGET_DISABLED );
+ }
+
+ /**
+ * @covers ::sanitizeGadgetName
+ * @dataProvider provideGadgetNamesWithSanitizedVersion
+ */
+ public function testConflictsWithNavPopupsGadgetNameSanitization(
$name, $sanitized ) {
+ $this->checkRequiredDependencies();
+
+ $user = $this->getTestUser()->getUser();
+
+ $configMock = $this->getMockBuilder( 'Config' )
+ ->setMethods( [ 'get', 'has' ] )
+ ->getMock();
+
+ $configMock->expects( $this->once() )
+ ->method( 'get' )
+ ->with(
PopupsGadgetsIntegration::CONFIG_NAVIGATION_POPUPS_NAME )
+ ->willReturn( $name );
+
+ $gadgetMock = $this->getMockBuilder( Gadget::class )
+ ->setMethods( [ 'isEnabled' ] )
+ ->disableOriginalConstructor()
+ ->getMock();
+
+ $gadgetMock->expects( $this->once() )
+ ->method( 'isEnabled' )
+ ->willReturn( self::GADGET_ENABLED );
+
+ $gadgetRepoMock = $this->getMock( GadgetRepo::class,
+ [ 'getGadgetIds', 'getGadget' ] );
+
+ $gadgetRepoMock->expects( $this->once() )
+ ->method( 'getGadgetIds' )
+ ->willReturn( [ $sanitized ] );
+
+ $gadgetRepoMock->expects( $this->once() )
+ ->method( 'getGadget' )
+ ->with( $sanitized )
+ ->willReturn( $gadgetMock );
+
+ $this->executeConflictsWithNavPopupsGadgetSafeCheck( $user,
$configMock, $gadgetRepoMock,
self::GADGET_ENABLED );
}
/**
- * Execute test and restore orGadgetRepo
+ * @return array
+ */
+ public function provideGadgetNamesWithSanitizedVersion() {
+ return [
+ [ ' Popups ', 'Popups' ],
+ [ 'Navigation_popups-API', 'Navigation_popups-API' ],
+ [ 'Navigation popups ', 'Navigation_popups' ]
+ ];
+ }
+
+ /**
+ * Execute test and restore GadgetRepo
*
* @param $user
+ * @param $config
* @param $repoMock
* @param $expected
*/
- private function executeConflictsWithNavPopupsGadgetSafeCheck( $user,
$repoMock, $expected ) {
+ private function executeConflictsWithNavPopupsGadgetSafeCheck( $user,
$config, $repoMock,
+ $expected ) {
+
$origGadgetsRepo = GadgetRepo::singleton();
GadgetRepo::setSingleton( $repoMock );
- $integration = new PopupsGadgetsIntegration(
$this->getConfigMock(),
+ $integration = new PopupsGadgetsIntegration( $config,
$this->getExtensionRegistryMock( true ) );
$this->assertEquals( $expected,
$integration->conflictsWithNavPopupsGadget( $user ) );
--
To view, visit https://gerrit.wikimedia.org/r/347523
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia7a57e9dcfa3b25129d6d2bf75795372fad2b251
Gerrit-PatchSet: 6
Gerrit-Project: mediawiki/extensions/Popups
Gerrit-Branch: master
Gerrit-Owner: Pmiazga <[email protected]>
Gerrit-Reviewer: Jdlrobson <[email protected]>
Gerrit-Reviewer: Krinkle <[email protected]>
Gerrit-Reviewer: Phuedx <[email protected]>
Gerrit-Reviewer: Pmiazga <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
