Search entities in the correct namespace #23
Conversation
| @@ -14,13 +14,15 @@ public function __construct( LoadBalancer $loadBalancer ) { | |||
| } | |||
|
|
|||
| public function getStatementCount( EntityId $entityId ) { | |||
| global $wgWBRepoSettings; | |||
There was a problem hiding this comment.
Does this extension depend on Wikibase Repo? Not seeing that in composer.json. Then again, there is no wikibase repo specific package (which makes depending on it all the more insane)
If it does not, then you presumably want to default in this code
There was a problem hiding this comment.
I’m not sure I understand your concern. This extension already uses Wikibase classes (DataModel, Lib, and Repo) everywhere; surely it logically depends on Wikibase Repo, whether that’s encoded in the composer.json or not?
I guess a more object-oriented approach would be to pass an EntityNamespaceLookup to the PagePropsStatementCountLookup and call getEntityNamespaces() instead of looking at the settings, but that’s still a Wikibase class.
There was a problem hiding this comment.
I’ve changed it now to use WikibaseRepo::getEntityNamespaces() instead of accessing the global variable directly. The WikibaseRepo class is already used several times in the importer (and always with getDefaultInstance() as far as I can tell).
| $db = $this->loadBalancer->getConnection( DB_MASTER ); | ||
|
|
||
| $res = $db->selectRow( | ||
| array( 'page_props', 'page' ), | ||
| array( 'pp_value' ), | ||
| array( | ||
| 'page_namespace' => 0, | ||
| 'page_namespace' => array_values( $wikibaseRepo->getEntityNamespaces() ), |
There was a problem hiding this comment.
thanks for the pull request.
I think page_namespace here should be only the namespace for the given entity type (of the entityId) and not all entity namespaces.
we could get this from EntityNamespaceLookup.
also, EntityNamespaceLookup should be set in the constructor (ideally the dependency injected)
There was a problem hiding this comment.
Thanks, that makes a lot of sense. I’ve uploaded a new version that uses an EntityNamespaceLookup.
lucaswerkmeister
changed the title
Instead of assuming that all entities are in namespace 0 (which is only true in a Wikidata-like installation, but not in a default Wikibase installation, and even then only for items, not properties), take the actual namespace of the requested entity from an injected EntityNamespaceLookup. Also, if we can’t find the entity, throw an exception instead of returning count 0: it’s better to fail the import than to duplicate statements. Fixes filbertkm#22.
|
I encountered the same error, can we push this to master? |
|
The Wikidata/WikibaseImport fork has this and a few other fixes merged, try using that one. |
|
Hi Lucas, you worked quite a bit on this repository ... do you know if pull requests are merged at some point? If not, would you take pull request in your repository? Salut |
|
Before pulling this, I believe another change is needed for compatibility with Mediawiki 1.37.
I am getting the following error when trying to import entities in Mediawiki 1.38:
|
Instead of assuming that all entities are in namespace 0 (which is only true in a Wikidata-like installation, but not in a default Wikibase installation, and even then only for items, not properties), take the actual namespace of the requested entity from an injected
EntityNamespaceLookup.Also, if we can’t find the entity, throw an exception instead of returning count 0: it’s better to fail the import than to duplicate statements.
Fixes #22.