Page MenuHomePhabricator

[Task] Remove deprecated methods from Entity and subtypes
Closed, DeclinedPublic

Description

There are lots of deprecated methods in Entity and its subtypes which are deprecated since 0.7.3. These methods should be removed in the next major release (4.0) as they are now deprecated for quite a long time and can be easily replaced by other methods as described in the deprecation notice.

This removes usage of Entity in the sense that it replaces methods which are defined in Entity by methods which are defined in dedicated interfaces (like FingerprintProvider).

Event Timeline

Bene created this task.May 6 2015, 7:29 AM
Bene raised the priority of this task from to Needs Triage.
Bene updated the task description. (Show Details)
Bene added projects: Wikidata, Wikibase-DataModel.
Bene added a subscriber: Bene.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 6 2015, 7:29 AM
Bene claimed this task.May 6 2015, 9:11 AM
Bene moved this task from Backlog to Review on the Wikidata-Sprint-2015-04-21 board.
thiemowmde added a subscriber: thiemowmde.EditedMay 6 2015, 12:20 PM

can be easily replaced [...]

Easily? Yes.

-$item->getAllAliases();
+$item->getFingerprint()->getAliasGroups()->toTextArray();

Nice, readable and maintainable? No.

Deprecated methods like these should be phased out some day, but I disagree that this should be done in the planed DataModel 3.0. DataModel 3.0 should, in my opinion, focus on phasing out Claim, and avoid Fingerprint related stuff until the discussion is finished.

T75496 does not depend on the removal of methods. This just doesn't make sense. Registries and strategies that allow new entity types can easily be introduced without phasing out stuff in Entity first.

So please remove both blockers.

Relevant patches, both blocked because of my first argument:

thiemowmde triaged this task as Low priority.May 6 2015, 12:21 PM
thiemowmde set Security to None.
thiemowmde added subscribers: JeroenDeDauw, daniel.
Bene raised the priority of this task from Low to Normal.May 6 2015, 1:25 PM

The state you posted is of course only an interstage. After some more refactoring there will be a FingerprintDiffer and so on so that we don't rely on Entity in that place either. I just did not want to put too many changes into one pull request.

So if you think in the long terms a bit you will understand how this improves the code as a whole and makes it finally more readable and maintainable.

Bene updated the task description. (Show Details)May 6 2015, 3:43 PM
thiemowmde added a comment.EditedMay 6 2015, 3:49 PM

Introduction of new code is not blocked on deprecated code.

So if you think in the long terms a bit you will understand [...]

I'm not going to continue a discussion on this level.

Can we please not create an issue per PR and in doing so duplicate the discussions? We are also not creating an issue per commit on Gerrit.

Most discussion happened here: https://github.com/wmde/WikibaseDataModel/pull/476

Bene closed this task as Declined.May 17 2015, 10:40 AM
JanZerebecki reopened this task as Open.May 18 2015, 1:47 PM
JanZerebecki added a subscriber: JanZerebecki.

Can we please not create an issue per PR and in doing so duplicate the discussions? We are also not creating an issue per commit on Gerrit.

Please do not prevent people from associating commits/PRs with a phabricator task. And thus please do not prevent people from working on a PR during a sprint (no phabricator ticket => can not be put into a sprint).

Had a chat with @Bene. This is what's left in Entity and not deprecated:

  • Entity::getFingerprint already has an interface, FingerprintProvider. In 3.1, the method should be deprecated in Entity and un-deprecated in Item and Property (via duplicate implementations, similar to Item::setLabel and such).
  • Entity::setFingerprint needs investigation. The most flexible solution is to create a FingerprintHolder with a getter and a setter, similar to StatementListHolder, make Item and Property implement this interface and deprecate the methods in Entity. But do we really need setFingerprint? If getFingerprint is guaranteed to not return a copy, all manipulations can be done on the returned Fingerprint object and setFingerprint never needs to be called. What we need to do:
    • Check if all usages of setFingerprint (especially the ones in Deserializers) are optional and can be removed.
    • The guarantee that getFingerprint does not return a copy must be added to the methods documentation.
  • Entity::isEmpty should be deprecated and un-deprecated in Item and Property.
  • Entity::clear should be deprecated and later die. Just create a new Item if you need a new item. Repurposing existing entities is ugly.
  • Entity::copy. Not sure about this. It used a lot, more than 100 times, mostly in tests obviously but also in production. But it's very, very trivial and should be avoided anyway. Better create a new object.

Had a chat with @Bene. This is what's left in Entity and not deprecated:

  • Check if all usages of setFingerprint (especially the ones in Deserializers) are optional and can be removed.
  • The guarantee that getFingerprint does not return a copy must be added to the methods documentation.

Just don't use Fingerprint at all. However, the question whether to return a clone from the getter exists for all mutable classes. This will be the same when dealing with TermList objects.

  • Entity::isEmpty should be deprecated and un-deprecated in Item and Property.

This should probably go into EntityDocument if it is needed. A generic method to check if an entity is empty seems sensible. It's also needed by EntityContent.

  • Entity::clear should be deprecated and later die. Just create a new Item if you need a new item. Repurposing existing entities is ugly.

Agreed

  • Entity::copy. Not sure about this. It used a lot, more than 100 times, mostly in tests obviously but also in production. But it's very, very trivial and should be avoided anyway. Better create a new object.

The implementation is nasty, but we do need a generic copying mechanism. Since EntityContent is mutable, it needs a deep clone mechanism. Creating a new object with the same content is exactly what is done here, the question is jut - how?

Change 222978 had a related patch set uploaded (by Bene):
Remove usage of Entity::copy

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

Bene removed Bene as the assignee of this task.Jul 6 2015, 7:00 PM
Bene updated the task description. (Show Details)
Bene removed a project: Patch-For-Review.
thiemowmde renamed this task from Remove deprecated methods from Entity and subtypes to [Task] Remove deprecated methods from Entity and subtypes.Sep 10 2015, 3:55 PM
thiemowmde removed a subscriber: gerritbot.
Bene closed this task as Declined.Feb 12 2016, 5:07 PM
Bene claimed this task.