Event Timeline
General comment: I don’t like all the print_r() in here – to me that sounds like a debugging function. We don’t want to print “human-readable information about a variable”, we want to print strings, and the right method for that in a maintenance script (since this is implemented as a maintenance script) would be $this->output(). That’s also the method used in ImportConstraintEntities.php, another maintenance script that prints configuration. (var_export( $value, true ) can be used to get the representation of $value as a string instead of printing it directly.)
L38'entityNamespaces' => $source->getEntityNamespaceIds(),
This is missing the entity slot names. Compare the WikibaseClient entitySources default setting in the change for T285471.
L77if ( $settings->hasSetting( 'entitySources' ) && !empty( $settings->getSetting( 'entitySources' ) ) ) {
If the client already has entitySources configured, why do we bother printing a config at all in that case?
Okay, but this is still strange:
if ( $settings->hasSetting( 'entitySources' ) && empty( $settings->getSetting( 'entitySources' ) ) ) {
We parse the legacy entity sources if the entitySources setting is present and empty, but not if it’s totally missing? And if entitySources is configured, it’ll now crash if I’m not mistaken (because the rest of the block runs with an unset $sources variable).
I didn’t realize at first where this difference between repo and client came from, but then I looked and apparently pre-service migration WikibaseClient had no getEntitySourceDefinitions() method, just a private $entitySourceDefinitions field. I assume that’s why you did the client part differently… but still, that probably means we should only print new client entitySources if the setting isn’t configured already.
Maybe we can do the same thing in the repo too? We could still use WikibaseRepo::getEntitySourceDefinitions(), but only use it and print anything if entitySources isn’t configured yet. Then repo and client would at least be more similar.
Updated with suggested changes.
done
I didn’t realize at first where this difference between repo and client came from, but then I looked and apparently pre-service migration WikibaseClient had no getEntitySourceDefinitions() method, just a private $entitySourceDefinitions field. I assume that’s why you did the client part differently… but still, that probably means we should only print new client entitySources if the setting isn’t configured already.
yes, from REL1_35 -> now this stuff has changed so much and that was the only way to have a common entrypoint
Maybe we can do the same thing in the repo too? We could still use WikibaseRepo::getEntitySourceDefinitions(), but only use it and print anything if entitySources isn’t configured yet. Then repo and client would at least be more similar.
Sounds good, however WikibaseRepo::getEntitySourceDefinitions() only exists in REL1_36+
Alright, I think this looks good to me now. (The second $settings->hasSetting( 'entitySources' ) is redundant but doesn’t hurt.) I can’t think of any other settings that would need to be added – both of the legacy parsers hard-code the 'local' entity source name, so I don’t think we need to configure the repo localEntitySourceName / client itemAndPropertySourceName.
Is there any reason we provide this script in phabricator and not in the code itself?
Well, it uses the Wikibase 1.36 code, so it wouldn’t make sense to ship the script with the 1.37 release. I guess adding it to a 1.36.x release would be possible, but that seems like more effort to me.
If you name the script config-migrate-1.36.php or equivalent and include instructions, it should be clear when to use it. In my opinion, all code we offer for use should be included in the repo.