Page MenuHomePhabricator
Create Task
Maniphest T286717

Decide how to handle PropertyId vs FederatedPropertyId in Property-specific services
Closed, ResolvedPublic
Actions

Description

We decided in T285453 that it does not make sense for FederatedPropertyId to extend PropertyId. In T286367 we stumbled upon the first service interface that type hints PropertyId (PropertyDataTypeLookup) which does not work with FederatedPropertyId as it is.

We identified the following options for now:

  1. Make FederatedPropertyId extend PropertyId after all
  2. Change all Property-specific service to instead type hint the common parent type, i.e.EntityId
  3. Make PropertyId an interface, rename the "old" PropertyId to LocalPropertyId (?) and make FederatedPropertyId and LocalPropertyId both implement that interface (and also cry because of all the files that would need changing)

Details

SubjectRepoBranchLines +/-
FP: make FederatedPropertyId extend PropertyIdmediawiki/extensions/Wikibasemaster+3 -3
Customize query in gerrit

Related Objects
Search...

Event Timeline

Jakob_WMDE created this task.Jul 15 2021, 12:58 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 15 2021, 12:58 PM
Jakob_WMDE claimed this task.Jul 15 2021, 3:30 PM
Jakob_WMDE moved this task from To do to Peer Review on the Wikibase - Federated Properties (Federation v2 - Sprint 2) board.
Jakob_WMDE edited subscribers, added: Rosalie_WMDE; removed: rosalieper.

I think we should go with option 3. We briefly talked about this already when we introduced FederatedPropertyId and I think for some reason we had instanceof PropertyId vs ->getEntityType() === 'property' in mind but totally forgot about services that type hint to PropertyId and should keep doing so.

I think the required steps would be

  1. release a new minor WikibaseDataModel version with LocalPropertyId subclassing PropertyId with no modification for forwards compatibility
  2. update the version everywhere to the newly released one and use LocalPropertyId where applicable (any place that instantiates a new PropertyId or uses getNumericId, newFromNumber or newFromRepositoryAndNumber)
  3. release a new major WikibaseDataModel version and make PropertyId an interface, rename the "old" PropertyId to LocalPropertyId
  4. update mediawiki/vendor (possibly also has to happen after step 1?)
  5. make FederatedPropertyId implement PropertyId
Rosalie_WMDE added a comment.Jul 16 2021, 7:46 AM

+1

• Tonina_Zhelyazkova_WMDE added a comment.Jul 16 2021, 8:40 AM
In T286717#7215378, @Jakob_WMDE wrote:

I think we should go with option 3. We briefly talked about this already when we introduced FederatedPropertyId and I think for some reason we had instanceof PropertyId vs ->getEntityType() === 'property' in mind but totally forgot about services that type hint to PropertyId and should keep doing so.

I think the required steps would be

  1. release a new minor WikibaseDataModel version with LocalPropertyId subclassing PropertyId with no modification for forwards compatibility
  2. update the version everywhere to the newly released one and use LocalPropertyId where applicable (any place that instantiates a new PropertyId or uses getNumericId, newFromNumber or newFromRepositoryAndNumber)
  3. release a new major WikibaseDataModel version and make PropertyId an interface, rename the "old" PropertyId to LocalPropertyId
  4. update mediawiki/vendor (possibly also has to happen after step 1?)
  5. make FederatedPropertyId implement PropertyId

We decided this approach makes the most sense but since it will introduce major changes, we are going to write an ADR first (see subtask) and implement the solution if/after the ADR is accepted.
In the meantime, to not block our ongoing work, we will make FederatedPropertyId extend PropertyId

Jakob_WMDE moved this task from Peer Review to Done on the Wikibase - Federated Properties (Federation v2 - Sprint 2) board.Jul 16 2021, 11:01 AM
gerritbot added a comment.Jul 16 2021, 1:36 PM

Change 704968 had a related patch set uploaded (by Jakob; author: Jakob):

[mediawiki/extensions/Wikibase@master] FP: make FederatedPropertyId extend PropertyId

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

gerritbot added a project: Patch-For-Review.Jul 16 2021, 1:36 PM
gerritbot added a comment.Jul 20 2021, 8:06 AM

Change 704968 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] FP: make FederatedPropertyId extend PropertyId

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

Maintenance_bot removed a project: Patch-For-Review.Jul 20 2021, 8:10 AM
ReleaseTaggerBot added a project: MW-1.37-notes (1.37.0-wmf.16; 2021-07-26).Jul 20 2021, 9:00 AM
Addshore closed this task as Resolved.Aug 24 2021, 9:33 AM
Addshore closed subtask T286762: Write an ADR to reflect our decision how to handle PropertyId vs FederatedPropertyId when it comes to property-specific services as Resolved.
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL