Page MenuHomePhabricator

Strategy for handling PHP interface changes
Open, NormalPublic

Description

Historically, the main contact surface between MediaWiki and extensions were base classes (to be subclassed by extensions) and hook signatures. This made non-breaking changes (with support for outdated code during some deprecation period) relatively easy: it was possible to add more parameters to a hook signature, add more parameters (with default values) to a base class method, and add new methods to a base class, without breaking anything, since all of these are valid in PHP:

function hook1( $a ) {}
call_user_func( 'hook1', 1, 2 );
class Core1 {
    public function f( $a, $b = null ) {}
}
class Extension1 extends Core1 {
    public function f( $a ) {}
}
// will raise a warning but still work
class Core2 {
    public function f( $a ) {}
    public function f2( $b ) {}
}
class Extension2 extends Core2 {
    public function f( $a ) {}
}

With dependency injection and generic composition over inheritance practices, we are increasingly moving towards asking extensions to implement an interface instead of subclassing a base class (and there are plans to change the hook system in similar ways). The above change mitigation strategies fail for interfaces:

interface Core3 {
    public function f( $a, $b = null );
}
class Extension3 implements Core3 {
    public function f( $a ) {}
}
// fatal error
interface Core4 {
    public function f( $a );
    public function f2( $b );
}
class Extension4 implements Core4 {
    public function f( $a ) {}
}
// fatal error

Replacing the old interface with a new one is not much better as it will break type hints. If we want to avoid inflicting a lot of pain on wiki owners with in-house or not fully up to date extensions, we need to come up with new change mechanisms.


TODO:

  • Articulate why we want to use interfaces in the first place
  • Survey how interface changes are handled in major PHP frameworks
  • Collect past examples of changing a hook or base class and see how those would work with an interface

Event Timeline

Tgr created this task.May 2 2018, 10:18 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 2 2018, 10:18 AM
daniel added a subscriber: daniel.May 2 2018, 10:34 AM

...and it's not just adding functions, which can be done by having the new interface extend the old interface. Still painful for calling code, but doable.

what is worse is changing method signature - that becomes virtually impossible.

Perhaps we just should not ask extensions to implement interfaces at all. Perhaps we should always provide a base class. We could declare an interface and type-hint against that, if that seems useful. This strategy seems to work reasonable well for Java: for the List interface, there is an AbstractList base class that can be used by code that wants to implement the List interface; this makes it easy to introduce a new ListNG interface with default/compat implementations in the AbstractList base class.

This problem checks the boxes for an RFC: it's strategic, cross-cutting and hard to undo. The RFC could consist just of the problem statement, to explore the topic and narrow the field of solutions. Or it could propose a concrete policy, and discuss that proposition.

daniel added a comment.May 4 2018, 9:33 AM

@Tgr TechCom would like to discuss this topic on IRC next Wednesday, at 23:00 CEST / 2pm PDT. The purpose of the discussion would be to explore the problem and possible solutions, not approving a concrete proposal. Do you think such a public conversation would be useful? Does that time work for you?

kchapman added a subscriber: kchapman.

TechCom is going to host a RFC IRC Meeting 2018-05-09 in the #wikimedia-office channel at 2pm PST(22:00 UTC, 23:00 CET)

Osnard added a subscriber: Osnard.May 8 2018, 5:48 AM
Tgr added a comment.May 9 2018, 8:57 PM

Some of the options that come to mind:

  • Just don't use interfaces (use abstract base classes instead). Easy enough but loses the two benefits interfaces provide: multiple inheritance and the ability of implementers to use their own base classes.
  • Use interfaces but require/recommend implementers to extend a standard base class (which can provide fallback behavior / null implementations). Has the same problems, and does not help with adding new parameters to an existing method.
  • Like above but use a trait instead of a base class. Does not help with adding new parameters to an existing method. Also, multiple inheritance with traits is kinda messy.
  • Version interfaces; new versions of classes can implement both the new and the old version. Works but eww.
Tgr added a comment.May 9 2018, 10:28 PM

IRC log of today's RfC meeting:

117:00 < kchapman> #startmeeting RFC meeting
217:00 < kchapman> #topic Come up with a strategy for handling interface changes https://phabricator.wikimedia.org/T193613
317:00 -!- dmaza [uid298049@gateway/web/irccloud.com/x-lnicfitvzvwcmtgg] has quit [Client Quit]
417:01 -!- ccogdill [~ccogdill@tan241.corp.wikimedia.org] has quit [Quit: ccogdill]
517:01 < kchapman> does the bot needed to be kicked? For some reason I remember it responding after the telling it to start the meeting (I may have remembered wrong)
617:02 < legoktm> the bot isn't even in the channel :(
717:02 < legoktm> it is supposed to say things, yes
817:02 < DanielK_WMDE> yea, that'S the problem
917:02 < DanielK_WMDE> we can just use the bot commands anyway, and then laterr grep for them by hand
1017:03 < TimStarling> there's no meetbot
1117:03 < DanielK_WMDE> just spottinng #info as a marker
1217:03 < DanielK_WMDE> tgr: you here?
1317:03 < tgr> o/
1417:03 < DanielK_WMDE> \o/
1517:03 -!- ccogdill [~ccogdill@tan241.corp.wikimedia.org] has joined #wikimedia-office
1617:03 < kchapman> who else is here to discuss interface changes?
1717:04 -!- heatherw [~administr@wikimedia/heatherawalls] has joined #wikimedia-office
1817:04 * legoktm waves
1917:04 -!- dannyh [~textual@wikimedia/DannyH-WMF] has joined #wikimedia-office
2017:05 < TimStarling> QCI is finished so yes I can do this now
2117:05 -!- dannyh [~textual@wikimedia/DannyH-WMF] has quit [Read error: Connection reset by peer]
2217:05 < kchapman> tgr: do you want to provide a brief summary since you filed the ticket?
2317:05 -!- dannyh [~textual@wikimedia/DannyH-WMF] has joined #wikimedia-office
2417:05 < tgr> sure
2517:06 < tgr> we are relying on interfaces increasingly often as the boundary between core and extensions
2617:07 < tgr> in the past we said "extend class Foo if you want to customize this functionality", these days we say "implement interface Foo"
2717:07 < tgr> that's generally a good thing as it doesn't prevent implementers from extending their own base classes, implementing multiple interfaces in one class etc
2817:08 < tgr> but changing the boundary becomes harder
2917:08 < tgr> e.g. adding a new optional parameter to a method is not a problem when we do it in a base class
3017:09 < tgr> for an interface, that will cause a fatal error for implementers who did not update accordingly
3117:09 < tgr> (see the task for examples)
3217:10 < tgr> so what do we intend to do to avoid inflicting that kind of unpleasantness on extension developers?
3317:11 < TimStarling> just recommending abstract classes over interfaces would solve some of it
3417:12 < DanielK_WMDE> I'd still want to define and type-hint against interfaces
3517:12 < tgr> that's certainly an option
3617:12 < DanielK_WMDE> but offering base classes would ease the pain
3717:12 < tgr> maybe the least bad
3817:12 -!- jgleeson [~jgleeson@wikimedia/jgleeson-wmf] has quit [Remote host closed the connection]
3917:13 -!- jgleeson [~jgleeson@wikimedia/jgleeson-wmf] has joined #wikimedia-office
4017:13 < DanielK_WMDE> side note: "e.g. adding a new optional parameter to a method is not a problem when we do it in a base class" isn't really true. this triggers "Declaration of B::f($a) should be
41 compatible with A::f($a, $b = '')"...
4217:13 < tgr> abstract base classes are annoying as they prevent you from using your own class hierarchies
4317:13 < DanielK_WMDE> indeed.
4417:13 < DanielK_WMDE> an alternative is to never modify interfaces, and replace them with different interfaces instead.
4517:14 < legoktm> versioned interfaces?
4617:14 < DanielK_WMDE> this is easier with small interfaces. and we could put version numbers in (*not* pretty, but effective)
4717:14 < Krinkle> The danger with base classes is also, that it is easy to blur the line of what a subclass is meant to add or change. E.g. where the end result can become incompatible with itself.
4817:14 < DanielK_WMDE> the problem is - this doesn't work well with type hints
4917:14 < Krinkle> If we encourage base classes to solve the interface issue, I think we'd need to be very strict and proactively mark things as final to avoid this problem, e.g. "if overriding method x,
50 other assumptions may differ and must require subclass to implement interface directly"
5117:15 < DanielK_WMDE> legoktm: we can do versioned interfaces, but what would code hint against? code would often have to accept both, and then have some utility method for mapping one to the other
5217:15 < DanielK_WMDE> also not totally horrible
5317:15 < DanielK_WMDE> but i like type hints...
5417:15 < Krinkle> Whilst still allowing simple overrides and extensions, where the base class can solve the problem of an additional utility being added to the interface.
5517:16 < DanielK_WMDE> Krinkle: oh yes. it should be very clear which methods are expected to be overwritten, and which must not
5617:16 < Krinkle> If our main example is "core concept X, add a method that uses other existing methods" e.g. convenience methods, another way to solve that is to use composition.
5717:16 < tgr> DanielK_WMDE: could do interface Foo_v1 extends Foo, interface Foo_v2 extends Foo
5817:16 < DanielK_WMDE> Krinkle: do you agree that if we go with base classes, we should still define interfaces?
5917:16 < tgr> but it makes the learning curve steeper
6017:17 < Krinkle> DanielK_WMDE: Yes, we should keep interfaces either way, especially if we're going to be strict about adding 'final' to our base classes where needed (e.g. for convenience methods).
6117:17 < DanielK_WMDE> tgr: ok - and calling code then type hints against Foo, and then calls makeFooV2, which looks at the concrete class and provides mapping?
6217:17 < tgr> interfaces + base classes don't really improve things IMO, you have all the problems you'd have with just base classes (forced hierarchy)
6317:18 < DanielK_WMDE> we have that kind of thing with Title::newFromLinkTarget (though that actually maps to new to thhe old interface, nto the other way around)
6417:18 < Krinkle> I'm not entirely sure though, what that would mean if we have both. Does that mean the interface will not change in breaking ways and remain a not-so-useful subset? (e.g. can't type-hint
65 against it because methods may be missing), or does that mean that we will break the interface frequently and only provide deprecation/removal notices for the base class (e.g. interface
66 is allowed but requires higher maintenance burden)
6717:19 < tgr> DanielK_WMDE: yeah, if it wants to be compatible with multiple core versions, it hints against Foo and then does if ($foo instanceof Foo_v1) or something
6817:19 < DanielK_WMDE> maybe we can do an example. We have an interface Foo with method frob($x) which we want to turn into frob($x, $y). And we have method sizzle( Foo $foo ) in some other class.
6917:19 < DanielK_WMDE> what do we do?
7017:20 * DanielK_WMDE sets up an therpad
7117:20 < DanielK_WMDE> https://etherpad.wikimedia.org/p/FrobSizzle
7217:22 < tgr> actually, base classes don't work for that use case
7317:22 < tgr> they would for adding a new method
7417:23 < DanielK_WMDE> well, one option is to add frob2()
7517:23 < Krinkle> Adding a non-optional parameter seems odd. Is there a past/real example where that happened?
7617:23 < DanielK_WMDE> the "add parameter" case can easily be mapped to "add method"
7717:23 < DanielK_WMDE> Krinkle: we can make it optional. same problem. php still complains
7817:23 < Krinkle> Seems like this makes the method do something entirely different, so yeah, new method would make more sense.
7917:24 < Krinkle> Sure, but it being optional makes the base class and other interaction easier to deal with.
8017:24 < Krinkle> In that it reduces the scope to developers providing the classes, as opposed to existing callers not working any more.
8117:24 < Krinkle> I don't think we should include changes in this conversation that would require callers to methods to change.
8217:25 < tgr> there are two scenarios where change is a problem
8317:25 -!- drewmutt is now known as California
8417:25 < tgr> one where the extension receives an instance of Foo and does something with it
8517:25 < DanielK_WMDE> Krinkle: i have changed the etherpad accordingly
8617:26 < tgr> I don't think it matters in that case whether Foo is a class or an interface
8717:26 < tgr> the other is where the extension implements/extends Foo
8817:26 < tgr> think Content, AuthPlugin etc
8917:26 < tgr> that's where interfaces become problematic
9017:27 -!- California is now known as drewmutt
9117:29 < TimStarling> if we have versioned interfaces, how would we name them? most of the class names that originally had underscores have been renamed now to take the underscores out
9217:30 < TimStarling> should we have a versioned namespace which interfaces are inside?
9317:31 -!- Mneisler [~textual@108-69-129-119.lightspeed.sntcca.sbcglobal.net] has quit [Quit: Computer has gone to sleep.]
9417:31 < legoktm> MediaWiki\Something\AnotherThingV1
9517:31 < DanielK_WMDE> TimStarling: i think having the same interface names in version namespaces would be very confusing, since all code that provides some legacy compatibility would have to juggle
96 multiple interfaces with the same name
9717:32 -!- Mneisler [~textual@108-69-129-119.lightspeed.sntcca.sbcglobal.net] has joined #wikimedia-office
9817:33 < tgr> if you end up with V1\Foo and V2\Foo that's actually not too bad
9917:33 -!- Jianhui67 [uid54581@wikimedia/Jianhui67] has quit [Quit: Connection closed for inactivity]
10017:33 < Krinkle> There is use..as...; which could be used for that if multiple are needed at the same time
10117:33 < tgr> but could just Foo and Foo2 (or FooV2)
10217:34 < DanielK_WMDE> I have updated https://etherpad.wikimedia.org/p/FrobSizzle
10317:34 < DanielK_WMDE> sorry for the silly names :)
10417:34 < DanielK_WMDE> i think this approach can work for adding methods (and parameters)
10517:34 < tgr> it's kind of like API versioning: ugly, but might be less confusing to reusers than continuous deprecation of things
10617:34 < DanielK_WMDE> but it's not... pretty.
10717:36 < tgr> one problem with the etherpad example is, how do you get rid of the deprecated method?
10817:37 < DanielK_WMDE> tgr: you stop using the old interface
10917:37 < DanielK_WMDE> finally, the new interface stops extending the old interface
11017:37 < tgr> doesn't help, still has to be implemented
11117:38 < tgr> uh, nvm, you are right
11217:38 < DanielK_WMDE> yea - stop hinting for / calling it. then, make sure nothing implements it diectly. after that, remove the itnerface. then remove the stub methods that were used to implement it.
11317:38 < tgr> that will work as long as all type hints are gone
11417:38 < Krinkle> Sorry about adding the Line and Square thingies, but I realised something odd.
11517:38 < DanielK_WMDE> maybe i'll add all that to the etherpad. is that useful, or a distraction?
11617:38 < Krinkle> Foo2Adapter
11717:39 < Krinkle> How can it implement the new interface for an object that only has the old interface?
11817:39 < Krinkle> It seems like the old one isn't capable of taking $y
11917:39 < Krinkle> ignoring it seems odd.
12017:39 < Krinkle> An optional parameter is backward compatible, but not forward compatible.
12117:39 < Krinkle> If it is given, it is significant and would break the contract if it is ignored.
12217:40 < DanielK_WMDE> it may. depends on the contract
12317:40 < DanielK_WMDE> for MCR, i'm introducing slot role parameters in some places
12417:40 < DanielK_WMDE> in that case, I would just check that the requested role is "main", and fail if it's not
12517:41 < tgr> yeah, just use some sensible default
12617:41 < tgr> that problem is not specific to interfaces
12717:41 < Krinkle> Yeah, but then if it is called with a non-main slot, it can't be ignored.
12817:42 < DanielK_WMDE> no, it would just behave as if that slot did not exist
12917:42 < Krinkle> I haven't looked at all the angles, but seems to me like the code should be designed in a way where (talking about MCR) a class for main-slot only can't be given a non-main slot. E.g.
130 isn't about failing at run-time, but about making it impossible to reach without type errors.
13117:42 < DanielK_WMDE> of course, this restriction has to be built into the new methods contract
13217:44 < Krinkle> When the new interface is added, nothing expects it yet. Newer consumer code that wants to use it can type for it, but that would be its own breaking change of the consumers own class,
133 which would require deprecation and/or avoidance.
13417:45 < Krinkle> I'm not sure adapters are feasible as a general solution to the issue.
13517:45 < kchapman> Reminder: 15 minutes left. Also if you use the #info tag it will help the katebot grep to create the minutes
13617:47 -!- dmaza [uid298049@wikimedia/dmaza-wmf] has joined #wikimedia-office
13717:47 < DanielK_WMDE> Krinkle: i think it's a working option, but it's not a very nice and easy one.
13817:47 < DanielK_WMDE> but maybe we want to circle back: do we want to go back to base classes, because of all of this?
13917:48 < tgr> would be nice to have a survey of how other PHP frameworks deal with this
14017:48 < Krinkle> How do base classes solve the problem of being able to introduce a new primitive/requirement into the system in a non-breaking way for sub classes?
14117:48 < Krinkle> I don't think that's a solveable problem. New requirements require new implementation.
14217:49 -!- Stryn [~chatzilla@wikipedia/Stryn] has quit [Ping timeout: 255 seconds]
14317:50 -!- wm-labs-meetbot [tools.meet@wikimedia/bot/wm-labs-meetbot] has joined #wikimedia-office
14417:51 -!- Stryn [~chatzilla@wikipedia/Stryn] has joined #wikimedia-office
14517:51 < DanielK_WMDE> Krinkle: it's not a prooblem solvable in a general way. but often we have additional constraints or guarantees that allow this to be solved.
14617:51 < DanielK_WMDE> e.g. the "main" slot.
14717:51 -!- ccogdill [~ccogdill@tan241.corp.wikimedia.org] has quit [Quit: ccogdill]
14817:52 < Krinkle> That means if we make use of a non-main slot in core (e.g. for categories) than any wiki using their own storage engine would just fail at run-time with the check we add.
14917:52 < kchapman> Just 8 minutes left.
15017:52 < Krinkle> That would trade a static error for a run-time exception.
15117:53 < DanielK_WMDE> tgr: is this conversation useful? is there any concrete question we should try to answer in the remaining minutes?
15217:53 < tgr> no, it just means they couldn't use categories
15317:53 < Krinkle> if we don't make use of the added requirements by default, then that would be easier, but that sounds more like a feature flag.
15417:53 -!- jgleeson [~jgleeson@wikimedia/jgleeson-wmf] has quit [Ping timeout: 250 seconds]
15517:54 -!- Nudin_WMDE [~nudin@2001:a61:2bb9:3500:4410:b996:e39e:55fa] has quit [Quit: Verlassend]
15617:54 -!- ccogdill [~ccogdill@tan241.corp.wikimedia.org] has joined #wikimedia-office
15717:54 < tgr> DanielK_WMDE: it helped to make the problem less vague, yeah
15817:55 < kchapman> 5 more minutes. tgr anything else for the remaining time?
15917:55 < DanielK_WMDE> tgr: we don't have meetbot here... can you put a summary on the ticket?
16017:55 < tgr> it sounds like we could use some real-life examples (past changes which would have been problematic with an interface) to see how does Krinkle's objection pan put there
16117:56 < Krinkle> Just to clarify, I don't object any current proposal, but I have certain doubts about whether it can solve the problem we want to solve and/or whether it solves a subset only, and
162 whether we're fine with that, or alternatively, I may've misunderstood the problem, for which a real-time example would help.
16317:57 < tgr> yeah, I should have said "Krinkle's doubts", my bad
16417:57 < tgr> it was a good point
16517:58 < DanielK_WMDE> Krinkle: I think you are correct - the proposal, in the general case, is a violation of LSP.
16617:58 < DanielK_WMDE> but I don't see a good alternative
16717:58 < DanielK_WMDE> (also, it's hard to not violate LSP if you don't have type parameters)
16817:59 < bawolff> As an aside, when I saw the subject line announcing this meeting, I assumed everyone was talking about how to make UI changes more palatable to the community. Email subjects might want
169 to be more clear on what this is about in the future (As this topic is much more interesting than the one i thought it was)
17017:59 < DanielK_WMDE> ah. "interface changes". I see :)
17117:59 < kchapman> sorry bawolff I just took the title of the phab task for the email. I'll keep that in mind in the future
17218:00 < tgr> uh, sorry, that reading never occurred to me
17318:00 < bawolff> kchapman: no worries, its totally understandable that people wouldn't read it the way I was reading it
17418:00 < tgr> might be spending too much time in PHP land
17518:00 -!- dannyh [~textual@wikimedia/DannyH-WMF] has quit [Quit: Computer has gone to sleep.]
17618:01 < kchapman> tgr so we are at the hour. Can you put a summary in the phab ticket?
17718:01 < tgr> will post a summary to the task
17818:01 < kchapman> Excellent. Thanks all, the meeting is closed

Daniels's example from the etherpad:
1interface Foo {
2 function frob($x); // should become frob($x, $y = '')
3}
4
5class MyFoo implements Foo { // in an extension
6 function frob($x) { // must not break
7 ...
8 }
9}
10
11class Xyzzy {
12 function sizzle( Foo $foo ) {
13 $foo->frob( "an X" ); // wants to call $foo->frob( "an X", "some Y" );
14 }
15}
16
17class Whammo {
18 function drizzle( Foo $foo ) {
19 $foo->frob( "another x" ); // should still work
20 }
21}
22
23------------------------------------------------------------------------------------------------
24
25// @deprecated use Foo2 instead
26interface Foo {
27 // Render a dot $x from the left ($y is always 0, not configurable)
28 function frobDot($x); // @deprecated use Foo2::foo2 instead!
29}
30
31interface Foo2 extends Foo {
32 // Render a dot at $x/$y coord
33 // Document that some implementations may not support $y !== 0
34 function frobDotAt($x, $y = 0);
35}
36
37class MyFoo implements Foo2 { // in an extension
38 function frobDot($x) { // implement old interface based on new interface
39 $this->frobDotAt($x, 0);
40 }
41 function frobDotAt($x, $y = 0) {
42 ...
43 }
44}
45
46class Foo2Adapter implements Foo2 {
47 private $foo;
48 public funtion __construct( Foo $foo ) {
49 $this->foo = $foo;
50 }
51
52 function frobDot($x, $y = 0) { // implement new interface based on old interface
53 if ( $y !== 0) throw ooops! // make sure we can ignore the new parameter
54 $this->foo->frobDotAt($x); // ignore $y ?
55
56 }
57}
58
59class Xyzzy {
60 function sizzle( Foo $foo ) { // wants to hint against Foo2 eventually
61 $foo2 = Foo2Adapter::newFromFoo( $foo ); // this mapping get's pushed up to callers more and more
62 $foo2->from2( "an X", "some Y" );
63 }
64}
65
66// stop calling Foo::frob ///////////////////////////////
67
68class Xyzzy {
69 function sizzle( Foo2 $foo ) { // callers must now do the Foo2Adapter thing
70 $foo->frob2( "an X", "some Y" );
71 }
72}
73
74class Whammo {
75 function drizzle( Foo2 $foo ) {
76 $foo->frob2( "another x", "more y" ); // should still work
77 }
78}
79
80// drop Foo ///////////////////////////////
81
82interface Foo2 {
83 // Render a dot at $x/$y coord
84 function frobDotAt($x, $y = 0);
85}
86
87// remove B/C methods
88class MyFoo implements Foo2 { // in an extension
89 function frobDotAt($x, $y = 0) {
90 ...
91 }
92}

Some of the main points raised:

  • Should we use interfaces at all? We should probably articulate the benefits we expect from them.
  • We should use realistic examples (e.g. from past refactorings) to avoid getting stuck on the programming language side of a task that does not make any sense on the business logic side anyway.
  • We should look at how other framworks deal with this.
  • The interface versioning option was the most popular (or at least the most discussed).
Tgr updated the task description. (Show Details)May 10 2018, 11:42 AM
Vvjjkkii renamed this task from Come up with a strategy for handling interface changes to 6sdaaaaaaa.Jul 1 2018, 1:13 AM
Vvjjkkii triaged this task as High priority.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed a subscriber: Aklapper.
CommunityTechBot renamed this task from 6sdaaaaaaa to Come up with a strategy for handling interface changes.Jul 1 2018, 9:14 AM
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added a subscriber: Aklapper.
Osnard added a comment.Jul 2 2018, 6:40 AM

Sorry, I did not have the time to read the whole discussion, so maybe this is misses the point. But as an extension developer I'd really love to see more (abstract) base classes that I can rely on. It helps me with the decision of how to structure my code.

For example, in BlueSpice we've created abstract base classes for a range of frequently used hooks [1][2]. When a developer wants to bind to a hook he only needs to extend the appropriate base class [3] and register the callback [4]. He does not need to care about the signature of the hook, as the hook base class provides him with the hooks' arguments in form of protected properties [5]. Some base classes even implement convenience methods [6]. If a hook signature changes, the base class can adapt to it and maybe provide a "compatibility layer/shim" to the subclasses (e.g. if the hook signature is not just extended, but a parameter actually changes; e.g. \Article -> \WikiPage ). If we'd made the fields private and only expose them by protected getters we could even emit deprecation warnings if a subclass accesses them, thus making migration easier.

[1] https://github.com/wikimedia/mediawiki-extensions-BlueSpiceFoundation/tree/bf63b4395efed92091db6dce25906ce196774d37/src/Hook
[2] https://github.com/wikimedia/mediawiki-extensions-BlueSpiceFoundation/blob/bf63b4395efed92091db6dce25906ce196774d37/src/Hook/SkinTemplateOutputPageBeforeExec.php
[3] https://github.com/wikimedia/mediawiki-extensions-BlueSpiceMultiUpload/blob/c588d9abdb70d87db101ecf130c65468e4889283/src/Hook/SkinTemplateOutputPageBeforeExec/AddDropZone.php
[4] https://github.com/wikimedia/mediawiki-extensions-BlueSpiceMultiUpload/blob/c588d9abdb70d87db101ecf130c65468e4889283/extension.json#L36
[5] https://github.com/wikimedia/mediawiki-extensions-BlueSpiceAuthors/blob/master/src/Hook/BeforePageDisplay/AddModules.php#L15
[6] https://github.com/wikimedia/mediawiki-extensions-BlueSpiceFoundation/blob/bf63b4395efed92091db6dce25906ce196774d37/src/Hook/SkinTemplateOutputPageBeforeExec.php#L81-L87

CommunityTechBot raised the priority of this task from High to Needs Triage.Jul 3 2018, 1:58 AM
Legoktm renamed this task from Come up with a strategy for handling interface changes to Come up with a strategy for handling PHP interface changes.Oct 2 2018, 4:28 PM

This came up a while back in an internal meeting to revisit at some point. I'm not 100% sure whether the above is result of that already happening or whether there is something specific Daniel wanted us to do as a group.

Tagging on the committee workboard for now, instead of tracking as TODO on a document somewhere.

daniel added a comment.EditedMar 14 2019, 7:15 AM

This came up a while back in an internal meeting to revisit at some point. I'm not 100% sure whether the above is result of that already happening or whether there is something specific Daniel wanted us to do as a group.

Thanks for bringing this up again @Krinkle!

Since we had the discussion, I was in the position to create new "handler" type extension points again, and went for the abstract base class approach, following the thoughts we discussed during the meeting on this topic.

I think abstract base classes should be the recommendation for "handler" type extension points, and perhaps also for "service" type handler points. Providing base classes for hook handlers, as @Osnard suggests, seems a bit heavy weight for the general case, though I can see the appeal. I'd leave that aspect to be discussed on T212482: Evolve hook system to support "filters" and "actions" only.

The next could be drafting a best practices document for creating "handler" and "service" extension points (as opposed to hooks, which are covered by T212482). This is something TechCom can take on. That document should then be made official via an RFC.

(terminology for reference: "service" extension points are all services that can be replaced/redefined in the main service container (or any service container, really). "handler" extension points register code for handling a new "flavor" of some concept in a registry service - examples are ContentHandlers, SlotRoleHandlers, ApiModules, SpecialPages, etc).

@daniel Sounds good. Do you think this document should be an RFC-approved policy, or a guideline that you write/publish as-is and maintain virally as other guidelines?

Krinkle assigned this task to daniel.Mar 20 2019, 7:04 PM
Krinkle moved this task from Inbox to In progress on the TechCom board.

@daniel Sounds good. Do you think this document should be an RFC-approved policy, or a guideline that you write/publish as-is and maintain virally as other guidelines?

Something in between would be ideal. A TechCom endorsed guideline? Go go through an RFC, with the goal of improving and buy-in. I wouldn't shoot for approval of a fixed policy, though. Amending it should be a matter of talk page discussion.

Tgr added a comment.Mar 21 2019, 12:53 AM

At this point I'd say, any time you want to use an interface, you should probably use an abstract class instead. (Except maybe if that class is not going to be exposed externally, but then what's the point of an interface in the first place?)

Whether something should have an interface/abstract class in the first place is an orthogonal issue; I'd like to see something for hooks that provides type safety and change management, whether that's a versioned interface or an abstract trait or an event object. (I'd like to have some discussion on those options when I have the time, which might not be soon.) The extension interfaces RFC seems to be mostly about hook contracts (performance and caching and whatnot) which is an orthogonal concern from both.

At this point I'd say, any time you want to use an interface, you should probably use an abstract class instead. (Except maybe if that class is not going to be exposed externally, but then what's the point of an interface in the first place?)

I still see valid use cases for interfaces. I'd phrase it like this: Interfaces should be treated as internal to the module that contains them. Abstract base classes should be used to define extension points that can be implemented outside the module.

Exposing an interface for consumption can still be useful - e.g. by declaring a hook parameter to implement a specific interface, thereby limiting the hook handler's access to the "safe" part of the underlying implementation. Interfaces can also still be useful within the module, e.g. to facilitate transitioning from "smart" records to value objects. For instance, Title implementing LinkTarget, so code that accepts a LinkTarget can still take a Title - a subclass wouldn't work as soon as you try to have Title implement another new interface, such as PageIdentity.

Whether something should have an interface/abstract class in the first place is an orthogonal issue; I'd like to see something for hooks that provides type safety and change management, whether that's a versioned interface or an abstract trait or an event object. (I'd like to have some discussion on those options when I have the time, which might not be soon.)

@BPirkle recently proposed using anonymous classes as one-off interfaces for use in hook parameters. I like the idea, it creates a well defined narrow interface that is bound to the specific use case (i.e. the hook).

The extension interfaces RFC seems to be mostly about hook contracts (performance and caching and whatnot) which is an orthogonal concern from both.

That RFC has a very different focus, but it's not entirely optional: it calls for the hook parameters to be (mostly) immutable (or at least, to not allow mutation by the hook handler). This can be achieved by making some modifiable object implement a read-only interface, and using that interface (or anon class, see above) in the hook signature. So we are back to the topic of exposing interfaces, but only for consumption.

Michael added a subscriber: Michael.May 6 2019, 3:58 PM
Simetrical removed a subscriber: Simetrical.
Simetrical added a subscriber: Simetrical.

Is the sole purpose of using abstract classes to give some time for the deprecation process to happen?

My main issue with abstract classes is that it is very easy to drift from a general contract of methods and there are no language constraints to avoid:

  • adding properties to the abstract class because this is convenient
  • adding a constructor
  • having protected/private methods

I don't think that interfaces and abstract classes are mutually exclusive, I've often seen them being used for the same component:

  • provide an interface for consumption
  • provide an abstract base class usable by implementations for convenience

There are no guarantee that the extension will use your base class but this provides a cleaner a more flexible way to let extensions adhere to a contract:

  • the interface will always remain the "official" contract
  • if the abstract class starts to drift it's easier to ditch it by deprecating it and providing another one since it's only referenced by implementations
dcausse added a comment.EditedJul 23 2019, 5:03 PM

Sorry I came to this task because I face the problem where having an interface used for consumption would have helped a lot. My example is the SearchResult/SearchResultSet objects that a SearchEngine must produce. My problem is:

  • SearchResult and SearchResultSet are concrete classes used as type hint all over the place
  • I have to extend this type hierarchy but the constructor parameters expected no longer make sense

My plan so far:

  • Introduce an interface defining what is an SearchResult[Set] and use it as type hint everywhere (https://gerrit.wikimedia.org/r/c/mediawiki/core/+/525101)
  • Add a minimal BaseSearchResult[Set] abstract base class based on my experience of what has been useful so far that SearchEngine implementation can extend for convenience
  • Eventually ditch the old SearchResult[Set] concrete classes
Tgr added a comment.Jul 23 2019, 5:08 PM

I have to extend this type hierarchy but the constructor parameters expected no longer make sense

If you extend a class, it shouldn't matter what the constructor parameters of the original class were. (Unless the class is constructed in code not controlled by you via some non-extensible mechanism, in which case that's your fundamental problem.)

I have to extend this type hierarchy but the constructor parameters expected no longer make sense

If you extend a class, it shouldn't matter what the constructor parameters of the original class were. (Unless the class is constructed in code not controlled by you via some non-extensible mechanism, in which case that's your fundamental problem.)

Do you mean avoiding to call the parent constructor?
While not calling the parent constructor is something allowed in PHP and may allow you to unlock some tricky situations I would not consider this to be a good practice.

Would a solution using a trait as an adapter to ease the transition be helpful to mitigate the constraints involved by only having a Base class?
Please see: P8791 for a short example
cons:

  • more code files

pros:

  • BC code isolated to a dedicated trait
  • can still implement the interface (and thus multiple ones) as long as you adhere to the BCTrait
  • call sites are type hinting with the interface
  • the interface remains a clear contract as opposed to a Base class that could rapidly become hard to read.

The constructor signature of the abstract base class would have to be treated as a public interface. Signature changes must be done in a backwards compatible way. PHP allows for polymorphic parameter lists via func_get_args. Not pretty, but doable.

Just like for the old methods that are kept for backwards compat and map to the new methods, the old constructor signature would be supported. Calls to deprecated methods and use of deprecated constructor signatures would trigger a deprecation warning. The B/C code would still be local to the abstract base class - which is exactly why we have it in the first place.

That being said: a base class that exists in lieu of a proper interface should probably have no members, and thus no need for a constructor. It's only once you start putting "shared utility code" into the base class that this need arises. Code sharing via subclassing is generally an anti-pattern anyway, and we should use composition instead (using traits or helper classes).

I feel that this guideline is very limiting esp the fact that it prevents to introduce short and isolated contract (e.g. things like LoggerAwareInterface).
For instance how this guideline would apply for things like DestructibleService?

daniel added a comment.EditedJul 26 2019, 11:39 AM

I feel that this guideline is very limiting esp the fact that it prevents to introduce short and isolated contract (e.g. things like ).
For instance how this guideline would apply for things like DestructibleService?

You are raising a good point, namely that fact that interfaces are indeed useful and should not be totally abandoned. That'as not the intention. The intention is that extension points should not be PHP interfaces, but rather be base classes.

E.g. an extension should not directly implement BlobStore, and perhaps BlobStore shouldn't be an interface but a base class. Or we should have AbstractBlobStore or BlobStoreBase for extensions to base their implementation on. That way, we could for instance as a bulk load interface to BlobStore, with a fallback implementation that just calls the regular getBlob() method for each blob, without breaking implementations.

A custom implementation of BlobStore could of course also implement DestructibleService and LoggerAwareInterface. These are not extension points in the sense that an extension would "register their DestructibleService". An extension would register their BlobStore, which might also be a DestructibleService.

In the context of refining our extension interface, extension points are either "hooks", or "services", or "handlers". This is about services and handlers (ContentHandlers, API modules, REST routes, etc).

I guess that line is blurry though, and I'm not quite sure what to call interfaces like DestructibleService and LoggerAwareInterface. They are indeed public and should be stable. They are not really extension points. Any good ideas what to call these kind of interfaces, so we can discuss them explicitly as a group?

debt triaged this task as Normal priority.Jul 30 2019, 5:26 PM
debt edited projects, added Discovery-Search; removed Discovery-Search (Current work).
debt moved this task from needs triage to watching / waiting on the Discovery-Search board.

I created a draft of a guideline for extension points, see https://www.mediawiki.org/wiki/Wikimedia_Technical_Committee/Extension_point_guidelines. It proposes to always use base classes as extension points. please comment there.

Krinkle renamed this task from Come up with a strategy for handling PHP interface changes to Strategy for handling PHP interface changes.Wed, Aug 21, 7:42 PM
daniel moved this task from Under discussion to Inbox on the TechCom-RFC board.Tue, Aug 27, 2:34 PM
Izno added a subscriber: Izno.Sun, Sep 1, 3:37 PM

After some discussion, it seems to me that this is primarily about defining what we consider a stable interface. I thnk we should amend the deprecation policy to be explicit about this. Draft here:
https://www.mediawiki.org/wiki/User:DKinzler_(WMF)/Stable_Interface