Page MenuHomePhabricator

OO.Factory inherits from OO.Registry but violates LSP
Open, Stalled, LowestPublic

Description

oo.Factory inheriting from oo.Registry is a good example for bad inheritance as it is a violaton of the Liskov substitution principle.

Instead this would be a good case to use object composition.

OOjs might be a nice approach to get some common way of doing inheritance in different MW extensions and core. OOjs doing the "OO" (Object Oriented) part wrong internally though does not really shine a bright light on it.

Event Timeline

Danwe raised the priority of this task from to Needs Triage.
Danwe updated the task description. (Show Details)
Danwe added a project: OOjs core.
Danwe added a subscriber: Danwe.

To elaborate: The bad thing here is that register() and unregister() have different function signatures than the ones stated in the "super class", resulting in a violation of the class' (or interface's) contract.

Apart from that, there is no gain from using inheritance here. Inherit from oo.EventEmitter instead if that is what is desired here (whether the event emitter inheritance approach is a good one or not could be an interesting discussion I guess).

(whether the event emitter inheritance approach is a good one or not could be an interesting discussion I guess).

Classes aren't supposed to inherit from EventEmitter, they're supposed to mix it in. Registry does in fact do this: it inherits nothing, and mixes in EventEmitter.

See also T107636: Allow explicit naming of constructors in OO.Factory , which proposes unifying the register() method. If we do that, that might make this issue go away.

About the mixin, you are right there, my mistake.

T107636 won't change this bug though, inheritance is still the wrong approach for oo.Factory.

T107636 won't change this bug though, inheritance is still the wrong approach for oo.Factory.

@Danwe I think Roan meant that if we fix T107636 by optionally allowing the name to be specified as first parameter to oo.Factory#register, it also becomes compatible with the parent class, which addresses this task.

Krinkle renamed this task from oo.Factory: inheriting from oo.Registry violates LSP - use composition instead to OO.Factory inherits from OO.Registry but violates LSP.Apr 5 2019, 5:34 PM
Krinkle claimed this task.
Krinkle moved this task from Accepted Enhancement to Done on the OOjs core board.

Change 501648 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[oojs/core@master] Factory: Use OO.Registry internally instead of sub class

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

Change 501648 merged by jenkins-bot:
[oojs/core@master] Factory: Use OO.Registry internally instead of sub class

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

Most sub-classes of OO.Registry expect to be able to access the "private" plain-object property registry (e.g. ve.dm.NodeFactory). Changing this would effectively be a breaking change, even though this isn't documented as a public API.

Change 502350 had a related patch set uploaded (by Esanders; owner: Esanders):
[oojs/core@master] Revert "Factory: Use OO.Registry internally instead of sub class"

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

@Esanders It wasn't a publicly documented property and as such, not a breaking change of the interface.

Anyway, technicalities aside, we can hold off on a release until we migrate VE. Or, if not easy to do, I'm also fine with a revert and re-landing in a later minor release.

At first glance, it seems like lookup would accomodate the use case that VE uses it for. Might even predate the existence of that method.

Change 502350 merged by jenkins-bot:
[oojs/core@master] Revert "Factory: Use OO.Registry internally instead of sub class"

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

From my comments on the patch:

  • Surely the sub-class *should* accept a narrower space of values? e.g. MyAnimalFactory should only accept Animal sub-classes as data. In this regard, Factory should be a sub-class of Registry, as it only accepts Functions, instead of any data.
  • The signatures being different could be fixed by changing Factory#register to (name,constructor), and special casing (constructor) when passed as a single argument.

Surely the sub-class *should* accept a narrower space of values?

The "surely" + "?" irritates me, is this a question or a statement?

e.g. MyAnimalFactory should only accept Animal sub-classes as data.

Surely you'd be violating LSP that way. Not saying that's necessarily a bad thing here (it's been years since I've looked at this), but just saying.

Citing commit message of a2bf54e90cb:

Two reasons:

  1. It wasn't a strictly substitutable implementation of Registry due to the signatures being different and it accepting a much narrower space of values. Per LSP, extra params and larger range of types is allowed, but not fewer.
  1. Composition in general creates APIs that are easier to learn, less work to maintain, and less fragile in practice. E.g. the API surface does not snowball through sub classes, and no clashes with instance properties.
Krinkle changed the task status from Open to Stalled.Apr 9 2019, 5:11 PM
Krinkle removed Krinkle as the assignee of this task.
Krinkle lowered the priority of this task from Low to Lowest.
Krinkle removed a project: Patch-For-Review.

Marking as stalled until VE refs to hasOwn(this.registry,…) and this.registry[key] are updated to use lookup() instead. Not a priority in any way.