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.
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).
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.
Change 501648 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[oojs/core@master] Factory: Use OO.Registry internally instead of sub class
Change 501648 merged by jenkins-bot:
[oojs/core@master] Factory: Use OO.Registry internally instead of sub class
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"
@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"
From my comments on the patch:
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.
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.