Page MenuHomePhabricator

Refactor MediaWikiServices::forceGlobalInstance to return void instead of old instance.
Open, Needs TriagePublic

Description

Refactor MediaWikiServices::forceGlobalInstance to return void instead of old instance.

Motivation:
Right now code contains a lot of legacy calls without Dependency Inversion to singleton MediaWikiService instance like:

MediaWikiService::getInstance()->getService() ->doSomething();

Example:

interface IColor {
  function getName(): string
}
interfact IColorer {
  function getColor(string $color): ?IColor
}
class ItemHandler {
 function coloredTo(string $color): void {
   $colorer = MediaWikiService::getInstance()->getColorer();
   $this->color = $colorer->getColor($color)
 }
 function getColor(): IColor { }
}

To cover ItemHandler::coloredTo by pure unit test we have to mock MediaWikiService somehow.

class ItemHandlerTest ... {
 function testColoredTo() {
   $item = new SomeClass;
   $this->assertNull($item->getColor())
   $item->coloredTo('red')
   $this->assertSame(
     'Red'
     $item->getColor()->getName()
   )
 }
}

We can't do that now, because MediaWikiServices::forceGlobalInstance has

$old = self::getInstance();

Line which on first call initialises filly-working service with valid config.
Note. This line violated the Single Responsibility Principle and causes side-effects.
To fix this problem we

  • can re-engineer
MediaWikiServices::forceGlobalInstance($): MediaWikiServices

to

MediaWikiServices::forceGlobalInstance($): void

remove the line:

$old = self::getInstance();

And change return void;

  • reengineer each call to it.

from

$oldInstance = MediaWikiServices::forceGlobalInstance($newInstance);

to

$oldInstance = MediaWikiServices::getInstance();
MediaWikiServices::forceGlobalInstance($newInstance);

Below is the draft of implementation:

class MockMediaWikiServicesRegistration {
   public static function register( TestCase $testInstance ) {
      $mock = new MockBuilder( $testInstance, MediaWikiServices::class );
      $service = $mock
         ->disableOriginalConstructor()
         ->getMock();
      MediaWikiServices::forceGlobalInstance( $service );
   }
} 
class MediaWikiServicesTest extends MediaWikiUnitTestCase {
   public function setUp() {
     parent::setUp();
     MockMediaWikiServicesRegistration::register($this);
   }
   public function tearDown() {
     parent::tearDown();
     MockMediaWikiServicesRegistration::destroy($);
   }
   public function testGetInstance() {
      MediaWikiServices::getInstance();
   }
}

Then tests get into:

class ItemTest extends MediaWikiUnitTestCase {
  public function setUp() {
     parent::setUp();
     MockMediaWikiServicesRegistration::register( $this );
  }
  public function tearDown() {
     parent::tearDown();
     MockMediaWikiServices::destroy();
  }
  function testColoredTo() {
    $colorer = $this->createMock( IColorer::class )
    $colorer->method( 'getName' )->willReturn( 'mock color' );
    $this->registerColorer($colorer);
    $item = new SomeClass;
    $this->assertNull($item->getColor())
    $item->coloredTo('red')
    $this->assertSame(
     'mock color'
      $item->getColor()->getName()
    )
  }
   private function registerColorer(IColorer $colorer) {
      /**
       * @var MediaWikiServices|MockObject $service
       */
      $service = MediaWikiServices::getInstance();
      $service
         ->method( 'getColorer' )
         ->willReturn( $colorer );
   }
}

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 23 2020, 2:28 PM
Peter.ovchyn updated the task description. (Show Details)Jan 23 2020, 2:31 PM

Assuming this task is about MediaWiki-ServiceContainer, hence adding project tag so others can find this task when searching for tasks under that project and not only in some team project.

daniel added a comment.EditedJan 23 2020, 8:04 PM

If I understand correctly, the idea is: make it possible to use the MediaWikiServices class in MediaWikiUnitTests, with a mocked instance of MediaWikiServices.

Right now, this does not work, because MediaWiki is not initialized. But with this small change, it would work.

Is that correct?

In your draft, I don't see a way to inject a mock service. How will that work?

Peter.ovchyn added a comment.EditedJan 23 2020, 8:15 PM

As far I understand the goal is to do something like this:

$service = $mock
         ->disableOriginalConstructor()
         ->getMock();
      MediaWikiServices::forceGlobalInstance( $service );

At the moment it's impossilble to do that, because calling MediaWikiServices::forceGlobalInstance( $service ); is impossible because it immeately leads to creating service here: $old = self::getInstance(); and crash.

Peter.ovchyn removed daniel as the assignee of this task.Feb 12 2020, 10:05 AM
Peter.ovchyn added a subscriber: daniel.