Topic on User talk:DKinzler (WMF)/Software Design Practices

Classes from another component should never be extended (subclassed), unless this is explicitly allowed in the documentation of this class.

This seems to be very restrictive. Component author may not foresee that somebody would need to extend/amend the functionality of the class. Requiring this would generate either copypaste or needless proxying.

Static entry points that serve as hook handlers should contain minimal code, typically getting any needed services from the global service locator, setting up the handler object, and calling the non-static handler method on that. This way, the actual handler code is isolated from global state, so unit tests for it do not require global fixtures.

I presume, this is for hooks that actually do depend on global state? Because a lot of hook implementations are not, though they can mutate global state (e.g. change registries). In this case, not sure how handler objects would help...

> I presume, this is for hooks that actually do depend on global state? Because a lot of hook implementations are not, though they can mutate global state (e.g. change registries). In this case, not sure how handler objects would help...

"Pure" functions can be static, that's true. I can make that explicit if you like.

Handler objects help with exactly one thing: they make it easy to inject mocks for testing.

The idea is to consider the object separate from the wiring code in static methods (entry points). Pushing any access to globals out of the object makes it easier to reason about the object's behavior.

+1 - I've been using this approach, and like it

Very common pattern for hooks is passing something to the hook (e.g. registry, state object, plain array, HTML, etc.) and the hook modifying it. Not sure if it counts as "pure" function strictly speaking but probably close to one as it doesn't change state outside what is explicitly passed to the function. Another pattern is for the hook to register certain things with a certain registry without passing that registry in the arguments - usually because there's no single object that can be passed. That would be harder to deal with without refactoring the hooks.

> Very common pattern for hooks is passing something to the hook (e.g. registry, state object, plain array, HTML, etc.) and the hook modifying it. Not sure if it counts as "pure" function strictly speaking but probably close to one as it doesn't change state outside what is explicitly passed to the function.

This would be pure enough in this context, since the hook handler doesn't need to access ''global'' state, even though it modifies ''some'' state.

> Another pattern is for the hook to register certain things with a certain registry without passing that registry in the arguments - usually because there's no single object that can be passed. That would be harder to deal with without refactoring the hooks.

Well, according to this guideline, you could get it from MediaWikiServices::getInstance() and inject it into the handler object.

But a refactoring of (pretty much) all the hooks is in the pipeline anyway, see Platform Evolution/Roadmap/Projects/Reduce Extension Interface Surface Area and https://phabricator.wikimedia.org/T212482.

> This seems to be very restrictive. Component author may not foresee that somebody would need to extend/amend the functionality of the class. Requiring this would generate either copypaste or needless proxying.

The whole point is that we want to support only explicitly defined extension points. Things that are not explicitly defined extension points should not be used as such. Extensions modifying core behavior in "unforseen" ways is exactly the problem we are trying to avoid.

So yes, it's restrictive. This restriction imposed on extension developers provides freedom to modify core, because the following changes don't have to be treated as breaking changes: modification and removal of protected methods and fields, broadening of parameter types, narrowing of return types, addition of optional method parameters, addition of abstract methods.

Treating all these things as breaking changes for all classes is really annoying and slows us down. And in fact, we don't: the deprecation policy already says that protected methods are only stable on classes that are explicitly defined to support sublcassing by extensions. So, in practice, subclassing of random classes is already unsupported.

My concern here is that there's no way we can predict how extension authors - including ourselves in the future - would need to modify existing functionality. If the method is protected and not private, it is already usually intended to be a flexibility point at least inside the module - then it is conceivable that outside extension may have similar needs.

I certainly get that only extending in specific pre-declared points makes refactoring core easier. On the other hand, it makes extending harder. I would probably be fine with "if you extend in places that are not specific extension point, then you'd have to fix your code once it changes" but if we're making policy that no extension should go beyond these points ever (of course this would be the case only for WMF extensions, we can't control anything else) then it would come back to core as a flood of requests for more extension points because it's not possible to foresee all needs.

So I guess the ultimate question is what "unsupported" means. If it means "we'll be trying to avoid it, and be extra vigilant when it happens and ready for breakage when refactoring occurs" then it's fine. If it means "we won't accept code that does it" then it's probably too narrow.

> it would come back to core as a flood of requests for more extension points because it's not possible to foresee all needs.

I indeed want to see that flood, because it give me a lot of information about into what direction the platform should evolve.

Whether this is a SHOULD NOT or a MUST NOT can be discussed. The main point is really about the deprecation policy. I'd say that if an extensions messes with things that are not extension points in core, it should be expected that this extension breaks, since things that are not part of the public interface may change without notice.