Page MenuHomePhabricator
Paste P7111

TechCom RfC meeting 2018-05-10

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