-
-
Notifications
You must be signed in to change notification settings - Fork 226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SMWSearch: Fix unit test; Move access to MediaWikiServices to ApplicationFactory #1916
Conversation
* @return \LoadBalancer | ||
*/ | ||
protected function getLoadBalancer() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have thought you make this available to the ApplicationFactory
as dedicated service hiding MediaWikiServices
as much we can to avoid issues when things change again on the MW side.
The DI container would always return a DBLoadBalancer
instance ( avoiding the null
case) and internally makes the call to either MediaWikiServices
or LBFactory
but it is hidden from the consumer who calls ApplicationFactory::getMediaWikiDbLoadBalancer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, no problem.
What about getDefaultSearchEngineTypeForDB
?
Not sure if I should register a callback or just add the method to ApplicationFactory
as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, assuming that we want to ban MediaWikiServices
access directly and given that we already have a reference in the DI container, I'd say just register a service and call it in ApplicationFactory
.
PS: For the curious mind, during testing all objects that are available via the DI container can be overridden from the outside (because sometimes you cannot control the injection) such as:
use SMW\Tests\TestEnvironment
private $testEnvironment;
protected function setUp() {
parent::setUp();
$store = $this->getMockBuilder( '\SMW\Store' )
->disableOriginalConstructor()
->getMockForAbstractClass();
$this->testEnvironment= new TestEnvironment();
$this->testEnvironment->registerObject( 'Store', $store );
// Example
// Reset the Title/TitleParser otherwise a singleton instance holds an outdated
// content language reference
$this->testEnvironment->resetMediaWikiService( '_MediaWikiTitleCodec' );
$this->testEnvironment->resetMediaWikiService( 'TitleParser' );
}
protected function tearDown() {
$this->testEnvironment->tearDown();
parent::tearDown();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS: I'll be back in a week, in case something is unclear don't get frustrated, I'll try to catch up as soon as possible.
This doesn't seems to be an issue on 1.27+ [0] but all others return with a:
[0] https://travis-ci.org/SemanticMediaWiki/SemanticMediaWiki/builds/166264508 |
Expecting this to fix #1916. |
This is becoming a nightmare. Could have sworn I've run the tests. Oh well... |
Thanks. |
This PR is made in reference to:
This PR addresses or contains:
(not sure if that is the correct thing to do)
This PR includes: