Page MenuHomePhabricator

[Task] Move Wikibase Lib code that belongs in DataModel Services to this new component
Closed, ResolvedPublic

Description

This is to collect and track services to move from Wikibase Lib to Wikibase DataModel Services as per https://phabricator.wikimedia.org/T93741

A similar task, for the services in Wikibase DataModel has already been completed: https://phabricator.wikimedia.org/T104187

Please review items in the "needing feedback" list and promote them to the "list with +1". This will allow us to start with moving those classes for which there are no serious concerns, while we work out what to do with the other ones separately.

List with +1:

  • Wikibase\Lib\Store\EntityRedirect (could go in DM)
  • Wikibase\Lib\EntityIdFormatter
  • Wikibase\Lib\EscapingEntityIdFormatter
  • Wikibase\Lib\PlainEntityIdFormatter
  • Wikibase\Lib\TypedValueFormatter
  • Wikibase\Lib\EntityIdLabelFormatter (depends on LabelDescriptionLookup)
  • Wikibase\Lib\Parsers\SuffixEntityIdParser
  • Wikibase\Lib\ClaimGuidValidator
  • Wikibase\Lib\Store\EntityLookup (there already is an ItemLookup and PropertyLookup in DMS)
  • Wikibase\Lib\Store\TermLookup
  • Wikibase\Lib\Store\LabelDescriptionLookup
  • Wikibase\Lib\Store\EntityRedirectLookup
  • Wikibase\Lib\Store\EntityRetrievingTermLookup (depends on TermLookup and EntityLookup)
  • Wikibase\Lib\Store\LanguageLabelDescriptionLookup (depends on LabelDescriptionLookup and TermLookup)
  • Wikibase\Lib\EntityRetrievingDataTypeLookup (depends on EntityLookup)
  • Wikibase\Lib\Store\RedirectResolvingEntityLookup (depends on EntityLookup and EntityRedirectResolvingDecorator)
  • Wikibase\PropertyLabelResolver (this is in lib/includes/store, the NS is just different)
  • Wikibase\Lib\Store\EntityPrefetcher and NullEntityPrefetcher
  • Wikibase\Store\TermBuffer (this is in lib/includes/store, the NS is just different)
  • Wikibase\ValuesFinder
  • Wikibase\WikibaseDiffOpFactory
  • Wikibase\Lib\Store\EntityRedirectResolvingDecorator (depends on UnresolvedRedirectException which derives from StorageException and MWException. That would need changing)

Event Timeline

JeroenDeDauw raised the priority of this task from to Needs Triage.
JeroenDeDauw updated the task description. (Show Details)
JeroenDeDauw added a subscriber: JeroenDeDauw.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 7 2015, 6:37 AM
JeroenDeDauw updated the task description. (Show Details)Aug 7 2015, 6:46 AM
JeroenDeDauw set Security to None.
JeroenDeDauw updated the task description. (Show Details)Aug 7 2015, 6:49 AM
JeroenDeDauw updated the task description. (Show Details)Aug 7 2015, 6:52 AM
JeroenDeDauw updated the task description. (Show Details)Aug 7 2015, 6:58 AM
JeroenDeDauw updated the task description. (Show Details)Aug 7 2015, 7:04 AM
JeroenDeDauw updated the task description. (Show Details)Aug 7 2015, 7:09 AM
JeroenDeDauw updated the task description. (Show Details)Aug 7 2015, 8:32 AM
JeroenDeDauw updated the task description. (Show Details)Aug 7 2015, 8:39 AM
JeroenDeDauw updated the task description. (Show Details)Aug 7 2015, 8:41 AM
Addshore updated the task description. (Show Details)Aug 7 2015, 10:58 AM

Well, I just went thought them all and agree with them all, so I have moved them all to the +1 section in the description!
I may throw some patches up for these today!

Many of these are now merged into the Services repo.

The following PRs are still open:

The following services still need to be moved / have PRs made for them out of the list above:

  • Wikibase\Lib\Store\EntityRetrievingTermLookup (depends on TermLookup and EntityLookup)
  • Wikibase\Lib\Store\LanguageLabelDescriptionLookup (depends on LabelDescriptionLookup and TermLookup)
  • Wikibase\Lib\EntityRetrievingDataTypeLookup (depends on EntityLookup)
  • Wikibase\Lib\Store\RedirectResolvingEntityLookup (depends on EntityLookup and EntityRedirectResolvingDecorator)
  • Wikibase\Lib\Store\EntityRedirectResolvingDecorator (depends on UnresolvedRedirectException which derives from StorageException and MWException. That would need changing)
  • Agree on all EntityId formatter and parsers.
  • Agree on all lookups, prefetchers and resolvers.
  • Disagree on EntityRedirect. DataModel please. This is not even a "service", only a very simple value object.
  • Disagree on TypedValueFormatter. This belongs to DataValues.
  • Mixed feelings about ValuesFinder. Could also go in DataValues.
JeroenDeDauw added a comment.EditedAug 8 2015, 5:19 AM

Thanks for the feedback Thiemo. Unfortunately I've already merged TypedValueFormatter and ValuesFinder now, as there where no objections on the PRs and I had not seen this. No real damage done as no release has happened yet. Let's discuss them and revert if needed.

Mixed feelings about ValuesFinder. Could also go in DataValues.

It can't - it uses PropertyDataTypeLookup which is in DM Services and PropertyId which is in DM

Disagree on TypedValueFormatter. This belongs to DataValues.

When I looked at it at first, I had a similar reaction. After all, this class does not strictly speaking depend on DM, so it seems to not match the contract of DM Services. .... #fail. Ok, I agree now this does not belong in this component. I mistakingly identified $dataTypeId as DM concept. And since it is not, this class indeed does not match the components contract of residence :)

Removal submitted in https://github.com/wmde/WikibaseDataModelServices/pull/25

I suggest we try to round this up quickly, as otherwise modifications we need to mirror might happen to the old copies. This task does not need t be 100% complete before we do the first release with moved stuff.

This comment was removed by JeroenDeDauw.

Moved over EntityRetrievingTermLookup, LanguageLabelDescriptionLookup and EntityRetrievingDataTypeLookup.

https://github.com/wmde/WikibaseDataModelServices/pull/27
https://github.com/wmde/WikibaseDataModelServices/pull/30
https://github.com/wmde/WikibaseDataModelServices/pull/32

Also started with EntityRedirectResolvingDecorator, though that needs more work. Feel free to pick up where I left it so far: https://github.com/wmde/WikibaseDataModelServices/pull/33

Now also moved RedirectResolvingEntityLookup: https://github.com/wmde/WikibaseDataModelServices/pull/40

Which are all the classes in the list if I'm not mistaken... still need to remove the old copies though.

I will make a new release of the components (DMS) once I have made a PR for sorting the EntityLookup stuff & exceptions etc

What's the status of this? Still in limbo? The code is going to diverge...

daniel triaged this task as High priority.Sep 10 2015, 1:35 PM

Bumping to high: The component split was started, and now we have several classes in the old and new component. If we want to go forward with this, we need to get a move on. Otherwise, the code is just going to diverge, and we have to start over.

I checked all classes in the current Wikibase-DataModel-Services 2.0.1 and found these duplicates:

  • EntityRedirectResolvingDecorator
  • UnresolvedRedirectException
  • RedirectResolvingEntityLookup
  • RestrictedEntityLookup
  • PropertyLabelResolver
  • TermBuffer

All these should be removed from lib fast. Do not forget to check if they are identical!

thiemowmde renamed this task from Move Wikibase Lib code that belongs in DataModel Services to this new component to [Task] Move Wikibase Lib code that belongs in DataModel Services to this new component.Sep 10 2015, 2:14 PM
thiemowmde removed a project: Patch-For-Review.

Thanks for providing the list Thiemo, this is very helpful. I was under the apparently wrong impression we already removed all the old copies. Will have a look at removing the remaining ones.

JeroenDeDauw added a comment.EditedSep 14 2015, 12:32 PM

There appears to be a design and/or problem with EntityRedirectResolvingDecorator, RedirectResolvingEntityLookup and UnresolvedRedirectException that is preventing ditching the old versions. Needs more investigation. Too tired to untangle that now.

The final removal (EntityRedirectResolvingDecorator and RedirectResolvingEntityLookup) is done with https://gerrit.wikimedia.org/r/#/c/239085/

So all of the things are now removed (as the above was merged) and this can now be closed? @JeroenDeDauw?

JeroenDeDauw closed this task as Resolved.Dec 8 2015, 10:36 AM
JeroenDeDauw claimed this task.

Yeah, we can always open a new ticket if we find something more