Add ReferencedEntityIdLookup interface

[mariushoch]

I chose a more general name here, as this can be used to lookup several kinds of relations (not only "parent" relations).

This is supposed to have an implementation similar to TypeCheckerHelper::isSubclassOf (in WikibaseQualityConstraints).

Needed in the context of T179155, but can probably also be re-used in WikibaseQualityConstraints.

[lucaswerkmeister]

How will this distinguished between “no target entity found” and “search limit exhausted”? (I assume the limit itself will be implementation-specific, but the interface should still provide an appropriate return type, right?)

[mariushoch]

I thought it's probably best to throw an exception in that case?! That would make the interface agnostic from that.

[lucaswerkmeister]

Okay, an exception makes sense. And I agree that the limit itself doesn’t belong in the interface.

[lucaswerkmeister]

GPL-2.0-or-later ?

[mariushoch]

Good catch (although this component uses the old naming scheme consistently)

[mariushoch]

Another question is: Do we want to have the recursion/ depth limits in the interface? I think we don't, given when for example using a SPARQL backend (instead of relying on looking up entities), this suddenly doesn't make much sense anymore.

[thiemowmde]

I'm pretty happy with what I see here! :-) The only detail I'm missing is that it's not entirely clear what "linked" exactly means. It could be linked both ways: Either the $fromId contains a property linking to one of the $toIds (this is what we want to do here), or the other way around.

I think we can add a short sentence like "… linked by statements with the given property ID, pointing towards the target entities."

This relationship might become a little more obvious when we order the function arguments like this: ( EntityId $fromId, PropertyId $propertyId, array $toIds ).

Please capitalize "ID" in the text (not in the variable names).

[mariushoch]

Changed.

[mariushoch]

Hm, should ClosestReferencedEntityIdLookup::getClosestEntityId take one entity id as starting point or an array of entity ids (see also T179155#4110697?

[thiemowmde]

Do we want to have the recursion/ depth limits in the interface? I think we don't […]

I agree. Sure, we know we need a limit. But I think it should not be in this interface, but in a configuration variable and in the constructor of the implementation that needs it.

should ClosestReferencedEntityIdLookup::getClosestEntityId take one entity id as starting point or an array of entity ids

In my opinion an array does not make much sense. The method would need to return two entity IDs then: the closest end point that was found, as well as the starting point that lead to this result.

Let's please stick to a single root node for this tree.

[mariushoch]

One thing that came to my mind yesterday is that while in a PHP implementation we can easily make sure that we actually find the closest referenced entity, this is not necessarily true for a SPARQL implementation for example.

Do we want to reflect this in the name of the interface?

[thiemowmde]

[…] find the closest referenced entity, this is not necessarily true for a SPARQL implementation for example.

I agree this should be rephrased then. Maybe instead of talking about "closest", talk about "whichever is found first, but not necessarily the closest in terms of graph distance"?

[JeroenDeDauw]

No objection to this, but I do wonder if it is really needed to construct such an elaborate message. Can't the info be kept in a more structured form? Perhaps when the message construction is needed it can be done on demand in the accessing method (not sure how this usually works for exceptions). That way you avoid the work in the constructor.

---

I'd extract a buildMessage and perhaps a buildTargetsString methods.

[JeroenDeDauw]

Looks good to me +1 (no +2 since I do not know the context)

---

Another question is: Do we want to have the recursion/ depth limits in the interface?

Without knowing the context and domain, my bet is on those being in the constructor of implementations that care about these being the better approach. I'd go with putting them in the interface if you don't have this info yet when the services are constructed and these arguments are applicable to all "normal" implementations.

[lucaswerkmeister]

One thing that came to my mind yesterday is that while in a PHP implementation we can easily make sure that we actually find the closest referenced entity, this is not necessarily true for a SPARQL implementation for example.

Do we want to reflect this in the name of the interface?

I just realized that this might have implications for usage tracking too. Consider a situation like the following:

      bar – baz – bat
foo ⧼
      qux

If I ask for getClosestEntityId( foo, property, [ bat, quq ] ), I will get bat returned. What happens now if a relation between qux and quq is added? If we guarantee the closest entity, the function should now return quq, and we might need to purge pages that used the bat result. If we don’t make that guarantee, that’s unnecessary. In other words, I feel like dropping this guarantee means we only need to track usages on the successful chain that led to the result, not on the whole tree we explored.

Does that make any sense? :)

[mariushoch]

I feel like dropping this guarantee means we only need to track usages on the successful chain that led to the result, not on the whole tree we explored.

Oh, that's true… the weak guarantee that it's just going to be some referenced entity allows us to do that :) So even more reasons to change to that.

[mariushoch]

Renamed the interface (and exception) and clarified in the documentation that this might not return the closest referenced entity.