Page MenuHomePhabricator

Update DatabaseItemTermStore and DatabasePropertyTermStore to use callback logic in DatabaseTermIdsAcquirer
Closed, ResolvedPublic

Description

The initialization logic for DatabaseTermIdsAcquirer that will be passed to DatabaseItemTermStore and DatabasePropertyTermStore will be concerned about providing the callback function. These two classes receive the acquirer as a constructor rparameter. To hide the callback logic from the singleton/factory that creates the acquirer and pass it through to these two classes, we can do one of two things (from cleanest to dirtiest, the opposite order would put the least effort/most pragmatic ones first):

  • allow DatabaseTermIdsAcquirer to accept multiple callbacks, adding a method to add new ones to in stance, say addTermIdsConsumerCallback => these two classes can add their callbacks to the instance that is passed to them
  • just provide a setter method for the callback on DatabaseTermIdsAcquirer => these two classes can use to set their callbacks on the passed acquirer instance
  • initialize Acquirer inside these two classes instead of it being passed to the constructor as done now => provide the callback as part of initialization

Event Timeline

As a slight variation of the first suggestion – if there’s an addTermIdsConsumerCallback() method, I think we could remove the constructor parameter again. After all, we wouldn’t use it in the WikibaseRepo use case, as far as I understand.

I’ll go with my suggested variant of the first version – I think that’s very clean and shouldn’t be too much effort.

Oh, there’s an issue with this though: in the callback, we don’t have the item/property ID to which the terms belong… I think it actually needs to be a second argument to acquireTermIds(), not a persistent attribute of the acquirer?

Change 514747 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/Wikibase@master] Add callback to TermIdsAcquirer::acquireTermIds()

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

Change 514748 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/Wikibase@master] Use callback logic in Database{Item,Property}TermStore

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

Change 514747 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Add callback to TermIdsAcquirer::acquireTermIds()

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

Change 514748 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Use callback logic in Database{Item,Property}TermStore

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

alaa_wmde closed this task as Resolved.Jun 11 2019, 1:56 PM