Use Maintenance::getDB in setupStore.php, refs 2963
#2968
Conversation
|
@kghbln This passes the test suite which means it should be safe to use whether it addresses the |
| $store->setConnectionManager( | ||
| $connectionManager | ||
| ); | ||
|
|
There was a problem hiding this comment.
Looks like this should be a single function. No need to have these two new variables in scope longer, or to access the ones already in scope. Plus this function is already very long, with lots of random details (thus mixed levels of abstractions) in it.
| /** | ||
| * @since 3.0 | ||
| * | ||
| * @return mixed |
There was a problem hiding this comment.
Should this not be a connection thing?
There was a problem hiding this comment.
No, it is a generic callback therefore you type. The invoker is responsible to make the right choice.
There was a problem hiding this comment.
I understand that this function cannot guarantee the type that is returned. Though I think this does not really matter for the return type. The return type is about indicating what it should return. Of course this should match what the callback is supposed to return. Unless I am misunderstanding the code, in both cases this should be a connection, whatever a connection is. Weirdly enough the ConnectionProvider interface does not mention a specific type. Is that understanding wrong?
| * | ||
| * @param Closure $callback | ||
| */ | ||
| public function __construct( Closure $callback ) { |
There was a problem hiding this comment.
callable is less restrictive and has no downsides as far as I know
There was a problem hiding this comment.
Yes, but nothing is wrong with Closure and it doesn't impose a functional impairment.
There was a problem hiding this comment.
It does: you can only provide Closures like this. You can't pass in 'globalFunctionName', 'SomeClass::staticFunction' or $instanceOfClassThatIsCallable
There was a problem hiding this comment.
Right, yes that is the only valid functional argument I have seen in this thread that should be reconsidered to allow for something like [ $this, '...' ]. [0] requires PHP 5.4 which is fine given that min req. is PHP 5.5.
|
|
||
| $connection = $this->getMockBuilder( '\stdClass' ) | ||
| ->disableOriginalConstructor() | ||
| ->getMock(); |
There was a problem hiding this comment.
huh?! This is the same as $connection = new \stdClass() no?
There was a problem hiding this comment.
Also beware that this PHPUnit syntax is not compatible with the latest versions of PHPUnit. I don't know if it is easy to switch to using a more recent version of PHPUnit (with SMW being a MW extension and all...), though if possible, doing this sooner rather than later will save migration work.
There was a problem hiding this comment.
This should work in any version, or? and if not what changed?
to switch to using a more recent version of PHPUnit (with SMW being a MW extension and all...)
2713 has the details!
There was a problem hiding this comment.
Oh... actually this might indeed work in the latest PHPUnit. I'm not sure as I have not used this format recently. Which is because it does not make sense to use this form there. You'd just do $connection = $this->createMock( '\stdClass' )
At least you would if there was reason to use the mock API. Why not just create the instance like in my first comment?
| CallbackConnectionProvider::class, | ||
| new CallbackConnectionProvider( $callback ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
IMO this does not add value and is thus harmful clutter. (Yes I realize I introduced the first tests like this in the codebase, this was a mistake)
There was a problem hiding this comment.
harmful clutter
Disagree, this allows to check whether the constructor does work or not.
| ); | ||
| } | ||
|
|
||
| public function testGetConnection() { |
There was a problem hiding this comment.
This is not a descriptive name. I suggest doing something like testGetConnectionReturnsTheCallbacksReturnValue.
There was a problem hiding this comment.
Feel free to changed it, it works for me.
There was a problem hiding this comment.
I'm not set on having this particular bit of code look one way or the other. What I am doing here with my time is giving you some advice, which you are of course free to ignore. Naming your test methods badly indeed "will work". Naming them more descriptive will work better, both for yourself, and to a bigger degree for other people. Not writing any tests also "will work". I recall giving advice to this one specific contributor several years ago that writing tests is a good idea, and at first getting a bunch of outraged "oh, so now I need to write tests for everything?!". I was happy to see this person changed their mind, wrote a lot of tests, and now encourages other people to do the same. If they had stayed at "it works for me", that'd have been a shame.
There was a problem hiding this comment.
Let not mix functional relevancy (which is to use a unit test to verify that what is expected can actually be asserted) with cosmetic preference which the later is about. We do agree that descriptive names may help others to understand the code in question but for a class with one single responsibility we don't have go in circles here and keep discussing a dead horse.
There was a problem hiding this comment.
So you are saying that in classes with a single responsibility good naming is not important as long as the classes do their job? And also that I am wasting my time even bringing this up?
That is not true,it is covered by the integration test. |
mwjames
force-pushed
the
setup-db-handle
branch
from
e062ac6
to
34749ae
Compare
|
@kghbln Would be nice if you could test this with the |
What integration test? One that was already there? That verify the correctness of the new code if it was already there and did not fail before the new code was added. In such a case you could refactor the code, accidentally break the new behavior, and still have the test passing. |
I will switch sandbox today. Will it be possible to back-port this to 2.5.x once I confirmed it is working on master? |
|
@mwjames The field test just failed. "setupStore.php" still uses user and password defined with Privileges assigned:
|
Well. after browsing the |
No worries and thanks for the fix. I will do another field test after the next update on sandbox. |
Works perfect now. Thanks a lot! |
This PR is made in reference to: #2963
This PR addresses or contains:
CallbackConnectionProviderMaintenance::getDBinsetupStore.phpwhich is expected to address setupStore.php: Use wgDBadminuser instead wgDBuser #2963 ($wgDBadminuserissue) but has not explicitly tested whether this resolves it or notThis PR includes:
Fixes #2963