[14:28:56] Technical Advice IRC meeting starting in 30 minutes in channel #wikimedia-tech, hosts: @CFisch_WMDE & @Tonina_WMDE - all questions welcome, more infos: https://www.mediawiki.org/wiki/Technical_Advice_IRC_Meeting [21:00:39] #startmeeting RFC meeting [21:00:58] #topic Come up with a strategy for handling interface changes https://phabricator.wikimedia.org/T193613 [21:01:57] 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) [21:02:50] the bot isn't even in the channel :( [21:02:55] it is supposed to say things, yes [21:02:57] yea, that'S the problem [21:03:12] we can just use the bot commands anyway, and then laterr grep for them by hand [21:03:20] there's no meetbot [21:03:23] just spottinng #info as a marker [21:03:45] tgr: you here? [21:03:54] o/ [21:03:58] \o/ [21:04:13] who else is here to discuss interface changes? [21:05:06] * legoktm waves [21:05:25] QCI is finished so yes I can do this now [21:05:47] tgr: do you want to provide a brief summary since you filed the ticket? [21:06:02] sure [21:06:38] we are relying on interfaces increasingly often as the boundary between core and extensions [21:07:20] in the past we said "extend class Foo if you want to customize this functionality", these days we say "implement interface Foo" [21:08:11] 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 [21:08:29] but changing the boundary becomes harder [21:08:59] e.g. adding a new optional parameter to a method is not a problem when we do it in a base class [21:09:27] for an interface, that will cause a fatal error for implementers who did not update accordingly [21:09:39] (see the task for examples) [21:10:18] so what do we intend to do to avoid inflicting that kind of unpleasantness on extension developers? [21:12:04] just recommending abstract classes over interfaces would solve some of it [21:12:37] I'd still want to define and type-hint against interfaces [21:12:42] that's certainly an option [21:12:48] but offering base classes would ease the pain [21:12:51] maybe the least bad [21:13:35] 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 compatible with A::f($a, $b = '')"... [21:13:36] abstract base classes are annoying as they prevent you from using your own class hierarchies [21:13:48] indeed. [21:14:04] an alternative is to never modify interfaces, and replace them with different interfaces instead. [21:14:20] versioned interfaces? [21:14:22] this is easier with small interfaces. and we could put version numbers in (*not* pretty, but effective) [21:14:25] 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. [21:14:34] the problem is - this doesn't work well with type hints [21:15:10] 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, other assumptions may differ and must require subclass to implement interface directly" [21:15:23] 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 [21:15:28] also not totally horrible [21:15:31] but i like type hints... [21:15:38] Whilst still allowing simple overrides and extensions, where the base class can solve the problem of an additional utility being added to the interface. [21:16:44] Krinkle: oh yes. it should be very clear which methods are expected to be overwritten, and which must not [21:16:49] 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. [21:17:02] DanielK_WMDE: could do interface Foo_v1 extends Foo, interface Foo_v2 extends Foo [21:17:05] Krinkle: do you agree that if we go with base classes, we should still define interfaces? [21:17:12] but it makes the learning curve steeper [21:17:41] 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). [21:18:03] tgr: ok - and calling code then type hints against Foo, and then calls makeFooV2, which looks at the concrete class and provides mapping? [21:18:10] interfaces + base classes don't really improve things IMO, you have all the problems you'd have with just base classes (forced hierarchy) [21:18:29] we have that kind of thing with Title::newFromLinkTarget (though that actually maps to new to thhe old interface, nto the other way around) [21:19:07] 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 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 is allowed but requires higher maintenance burden) [21:19:17] 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 [21:20:03] 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. [21:20:06] what do we do? [21:21:02] * DanielK_WMDE sets up an therpad [21:21:04] https://etherpad.wikimedia.org/p/FrobSizzle [21:23:03] actually, base classes don't work for that use case [21:23:11] they would for adding a new method [21:23:28] well, one option is to add frob2() [21:23:33] Adding a non-optional parameter seems odd. Is there a past/real example where that happened? [21:23:44] the "add parameter" case can easily be mapped to "add method" [21:24:00] Krinkle: we can make it optional. same problem. php still complains [21:24:00] Seems like this makes the method do something entirely different, so yeah, new method would make more sense. [21:24:17] Sure, but it being optional makes the base class and other interaction easier to deal with. [21:24:37] In that it reduces the scope to developers providing the classes, as opposed to existing callers not working any more. [21:24:58] I don't think we should include changes in this conversation that would require callers to methods to change. [21:25:40] there are two scenarios where change is a problem [21:25:59] one where the extension receives an instance of Foo and does something with it [21:26:09] Krinkle: i have changed the etherpad accordingly [21:26:19] I don't think it matters in that case whether Foo is a class or an interface [21:26:35] the other is where the extension implements/extends Foo [21:26:45] think Content, AuthPlugin etc [21:27:04] that's where interfaces become problematic [21:29:35] 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 [21:30:52] should we have a versioned namespace which interfaces are inside? [21:31:39] MediaWiki\Something\AnotherThingV1 [21:32:08] 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 multiple interfaces with the same name [21:33:24] if you end up with V1\Foo and V2\Foo that's actually not too bad [21:33:25] There is use..as...; which could be used for that if multiple are needed at the same time [21:33:59] but could just Foo and Foo2 (or FooV2) [21:34:39] I have updated https://etherpad.wikimedia.org/p/FrobSizzle [21:34:44] sorry for the silly names :) [21:34:57] i think this approach can work for adding methods (and parameters) [21:35:02] but it's not... pretty. [21:35:02] it's kind of like API versioning: ugly, but might be less confusing to reusers than continuous deprecation of things [21:37:02] one problem with the etherpad example is, how do you get rid of the deprecated method? [21:37:20] tgr: you stop using the old interface [21:37:45] finally, the new interface stops extending the old interface [21:37:46] doesn't help, still has to be implemented [21:38:36] uh, nvm, you are right [21:38:53] 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. [21:39:04] that will work as long as all type hints are gone [21:39:13] Sorry about adding the Line and Square thingies, but I realised something odd. [21:39:13] maybe i'll add all that to the etherpad. is that useful, or a distraction? [21:39:14] Foo2Adapter [21:39:33] How can it implement the new interface for an object that only has the old interface? [21:39:39] It seems like the old one isn't capable of taking $y [21:39:43] ignoring it seems odd. [21:39:55] An optional parameter is backward compatible, but not forward compatible. [21:40:07] If it is given, it is significant and would break the contract if it is ignored. [21:40:20] it may. depends on the contract [21:40:38] for MCR, i'm introducing slot role parameters in some places [21:41:11] in that case, I would just check that the requested role is "main", and fail if it's not [21:41:18] yeah, just use some sensible default [21:41:30] that problem is not specific to interfaces [21:41:58] Yeah, but then if it is called with a non-main slot, it can't be ignored. [21:42:37] no, it would just behave as if that slot did not exist [21:42:50] 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. isn't about failing at run-time, but about making it impossible to reach without type errors. [21:43:04] of course, this restriction has to be built into the new methods contract [21:45:06] 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, which would require deprecation and/or avoidance. [21:45:51] I'm not sure adapters are feasible as a general solution to the issue. [21:46:13] Reminder: 15 minutes left. Also if you use the #info tag it will help the katebot grep to create the minutes [21:47:31] Krinkle: i think it's a working option, but it's not a very nice and easy one. [21:47:53] but maybe we want to circle back: do we want to go back to base classes, because of all of this? [21:48:39] would be nice to have a survey of how other PHP frameworks deal with this [21:48:44] 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? [21:49:09] I don't think that's a solveable problem. New requirements require new implementation. [21:51:49] 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. [21:51:55] e.g. the "main" slot. [21:52:29] 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. [21:52:30] Just 8 minutes left. [21:52:37] That would trade a static error for a run-time exception. [21:53:18] tgr: is this conversation useful? is there any concrete question we should try to answer in the remaining minutes? [21:53:20] no, it just means they couldn't use categories [21:53:24] 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. [21:54:56] DanielK_WMDE: it helped to make the problem less vague, yeah [21:55:37] 5 more minutes. tgr anything else for the remaining time? [21:55:56] tgr: we don't have meetbot here... can you put a summary on the ticket? [21:55:57] 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 [21:57:15] 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 whether we're fine with that, or alternatively, I may've misunderstood the problem, for which a real-time example would help. [21:58:01] yeah, I should have said "Krinkle's doubts", my bad [21:58:14] it was a good point [21:58:21] Krinkle: I think you are correct - the proposal, in the general case, is a violation of LSP. [21:58:33] but I don't see a good alternative [21:59:00] (also, it's hard to not violate LSP if you don't have type parameters) [21:59:28] 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 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) [22:00:00] ah. "interface changes". I see :) [22:00:07] sorry bawolff I just took the title of the phab task for the email. I'll keep that in mind in the future [22:00:23] uh, sorry, that reading never occurred to me [22:00:49] kchapman: no worries, its totally understandable that people wouldn't read it the way I was reading it [22:00:50] might be spending too much time in PHP land [22:01:42] tgr so we are at the hour. Can you put a summary in the phab ticket? [22:01:48] will post a summary to the task [22:02:14] Excellent. Thanks all, the meeting is closed