Page MenuHomePhabricator

(Property|Item)TermStore::getTerms() is unused
Closed, ResolvedPublic

Description

In the trailblaze of creating the new term store, there's a really complicated logic built to get terms of entities (PropertyTermStore::getTerms() and copied logic in items, more context) but a) it's flawed, like "read new" for the term store is actually "read both" (let's see if the first one has it, if it doesn't, let's read it from the second one which is wasteful and would hide bugs like holes in the term store) and b) it's not used anywhere, if something needs all terms of an entity, it just loads it, that's actually way faster than hitting the term store and if you need the terms for just one language/term type. You would use prefetching term lookup instead.

We should get rid of this.

Event Timeline

If this is done, I’d say the interface(s) should be renamed, because having a “store” that’s write-only and doesn’t support reads doesn’t make sense to me.

If this is done, I’d say the interface(s) should be renamed, because having a “store” that’s write-only and doesn’t support reads doesn’t make sense to me.

I agree, also I'd be bold and would create new interfaces in data model services, abandoning https://github.com/wmde/wikibase-term-store for good.

I'm also probably adding un thought through ideas to this but...
If TermStores can't read is having them in lib the right place? Wouldn't we be smarter to lock them up in repo?

I'm also probably adding un thought through ideas to this but...
If TermStores can't read is having them in lib the right place? Wouldn't we be smarter to lock them up in repo?

Depends on whether the actual implementations implement just that interface (or also others for reading), but I think we could move some to repo, yes.

Change 571769 had a related patch set uploaded (by Hoo man; owner: Hoo man):
[mediawiki/extensions/Wikibase@master] Remove ItemTermStore/PropertyTermStore::getTerms

https://gerrit.wikimedia.org/r/571769

PR for new datamodel services release (which will feature our new interfaces): https://github.com/wmde/WikibaseDataModelServices/pull/235

Change 572207 had a related patch set uploaded (by Hoo man; owner: Hoo man):
[mediawiki/extensions/Wikibase@master] Rename (Property|Item)TermStoreWriter to more specific name

https://gerrit.wikimedia.org/r/572207

Change 572207 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Rename (Property|Item)TermStoreWriter to more specific name

https://gerrit.wikimedia.org/r/572207

abandoning https://github.com/wmde/wikibase-term-store for good.

Yes, lets get rid of this lib.

For creating the new narrow interfaces in WikibaseDataModelServices: https://github.com/wmde/WikibaseDataModelServices/pull/234

Looks like a good step!

Change 571769 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Remove (Item|Property)TermStore::getTerms implementations

https://gerrit.wikimedia.org/r/571769

Okay, relevant patch was https://gerrit.wikimedia.org/r/#/c/mediawiki/vendor/+/572652/ just it was not linked to this ticket!