Page MenuHomePhabricator

Figure out what to do with Entity::isEmpty
Closed, ResolvedPublic

Description

Current situation

Entity::isEmpty is deprecated (because Entity is). isEmpty is not part of any other interface implemented by our concrete entities (ie Item and Property), most notably EntityDocument.

We have code that is using the isEmpty method while getting an Entity or EntityDocument. That code either uses a deprecated method or one that is not guaranteed to be there. This is currently dealt with by ignoring the deprecation, ignoring the call to the method that is not guaranteed to be there, or by adding a method_exists( $entity, 'isEmpty' ) check.

There are only a handful places where the isEmpty check is needed on an entity of unknown type. (One example is here, though its not clear the problem will need to be solved there when the OCP violations in that code are fixed.)

Possible ways to improve

  1. Use method_exists checks everywhere (not so clean)
  2. Add isEmpty to EntityDocument (>99% of the users of the interface do not need the method)
  3. Create a new CheckableForEmptiness interface (is having an instanceof check sufficiently better than the method_exists?)
  4. Create a new CheckableForEmptinessEntity interface (will still need instanceof check)

Related Objects

View Standalone Graph
This task is connected to more than 200 other tasks. Only direct parents and subtasks are shown here. Use View Standalone Graph to show more of the graph.
StatusAssignedTask
Resolveddaniel
ResolvedBene

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 19 2015, 1:27 PM
JeroenDeDauw updated the task description. (Show Details)Aug 19 2015, 1:45 PM
JeroenDeDauw set Security to None.

Feel free to add additional approaches.

Approaches 1 and 2 seem best to me, though neither is without downsides (I'd like someone to suggest something better). 3 and 4 do not seem better and require an additional interface. I don't see any serious problems with approaches 1 and 2. 2 seems cleaner assuming going with it does not cause problems. That most users of the interface do not need the method makes me quite uncomfortable with it though, even though I can't think of concrete issues this will cause us in this case.

JeroenDeDauw updated the task description. (Show Details)Aug 19 2015, 1:51 PM

CheckableForEmptiness !

Can you motivate that? Clearly the entities could easily implement it, and several other classes could as well. However, that is looking at the interface from the implementers side, rather than the intended users side (which is what I think one needs to do for good interface design). I'd basically be inclined to add such an interface as soon as I'd be able to nicely type hint against it. Does your reasoning conflict with that, or do you think doing an instanceof check is sufficiently better than a method_exists one to warrant the added interface?

I prefer instance of over method_exists when reading code, I have no idea how the php internals actually handle this.

As for speed http://stackoverflow.com/questions/28767294/instanceof-or-method-exist-which-one-should-use says instanceof is marginally faster.

As for having something called CheckableForEmptiness, the use of this in typehints for method params is probably rather limited when thinking directly.
But Item & Property could probably implement this CheckableForEmptiness thing.

But then we may as well just add this thing to EntityDocument and be done with it.

thiemowmde added a subscriber: thiemowmde.EditedAug 26 2015, 3:45 PM

I just wrote this on https://github.com/wmde/WikibaseDataModel/pull/547#issuecomment-134935972:

I can not think of a reason why an entity type should not have an isEmpty method. Basically: Can you think of an entity type where it's not possible to tell if it's empty or not? In a worst case scenario it can simply return false all the time. So +2 from my side, but I want to wait for opinions of other team members.

Bene added a subscriber: Bene.Aug 31 2015, 10:40 AM

Any objections to adding isEmpty to EntityDocument? The comments on T110177 and on github don't show any objections. I will close this then as resolved.

Bene closed this task as Resolved.Aug 31 2015, 1:46 PM
Bene claimed this task.