Talk:Best practices for extensions

Jump to navigation Jump to search

About this board

Don't: directly construct service objects

13
DKinzler (WMF) (talkcontribs)

In a way similar to protected methods, constructor signatures of service-like objects are generally not stable. They are intended for use by wiring code, and can be expected to change without notice. Extensions (and all application logic) should ideally only ever directly construct plain value objects. However, we still have quite a number of things in core that are neither service objects nor value objects. So I propose to add the following to the "Don't" section:

  • MUST NOT: directly instantiate a service class defined by core, instead of obtaining an instance via a service container.
  • SHOULD NOT: directly instantiate anything that is not a plain value object.
Jdforrester (WMF) (talkcontribs)

Sounds good.

Tgr (WMF) (talkcontribs)

Sometimes you need a service with non-default configuration, most commonly when doing cross-wiki things. (Random recent example: )

DKinzler (WMF) (talkcontribs)

As long as that is in core, it could be acceptable. Extensions should never do that.

The Right Way (tm) is to introduce a factory service that you can ask for a service instance for the target wiki.

Eventually (tm) we'd want a full MediaWikiServices instance for the target wiki. That will be possible as soon as no service uses global state.

Tgr (WMF) (talkcontribs)

We won't realistically introduce factories for all the services which depend on the wiki (which is basically all the services), MediaWikiServices doesn't support using foreign wiki configuration yet (even if it will, that will only cover wikis in the same farm, which is not the only use case), and expecting extension authors to make core changes instead of doing simple workarounds in their extensions is not realistic or reasonable IMO (especially as long as we don't get our code review problems under control).

DKinzler (WMF) (talkcontribs)

> MediaWikiServices doesn't support using foreign wiki configuration yet

Actually, it does. But not all services are completely isolated from global state.

> expecting extension authors to make core changes instead of doing simple workarounds in their extensions is not realistic or reasonable IMO

These workarounds are what is preventing core changes, because we break them when we change core. I would expect extension authors to request or propose core changes when they need them, yes.

DKinzler (WMF) (talkcontribs)

Reconsidering what I just said: extensions may have a need to call constructors of services if they extend the service class, or if the replace the service instance. That is, they may need to call service constructors in wiring code.

This is fine as long as the service is declared to be extensible - which implies that the constructor signature is treated as public and stable. Constructor signatures of other services should not be considered stable.

The rationale for this restriction is to support better dependency injection: with DI, there is often a need to change constructor signatures. Which is generally no problem since DI says that the constructor is only called in the wiring code, so there is only one place that needs to be changed to accommodate the new signature. Making such changes backwards compatible is a burden that should be avoided when possible - that is, we should only pay that cost in cases where the service is intended as an extension interface.

Tgr (WMF) (talkcontribs)

> Actually, it does.

Actually, it doesn't :) There is no way for me to obtain a DI container for a foreign wiki, short of constructing that container myself using a fake configuration object that I also construct myself, which is a lot more nasty than constructing a specific service manually using a specific wiki ID and language object.

> I would expect extension authors to request or propose core changes when they need them, yes.

That's a reasonable expectations for WMF staff maintaining extensions used in Wikimedia production. It's a completely unreasonable expectation for people running their own wikis, possibly in their free time, their bug reports get mostly ignored and their patches spend months if not forever in Gerrit without anyone ever looking at them. The MediaWiki community just does not provide the level of volunteer support currently where we could in good conscience ask people to have their problems fixed in core.

More specifically to the foreign wiki issue, what would the core change be? Do we really want to create a factory class for everything that takes a wiki ID, when those classes won't be necessary once MediaWikiServices supports foreign wikis?

DKinzler (WMF) (talkcontribs)

> There is no way for me to obtain a DI container for a foreign wiki, short of constructing that container myself using a fake configuration object

Right - the problem is that there is no clean way to get the config settings for another wiki. For this to be complete, it would have to include the enabled extensions as well, so we'll need injecteble hook runners.

> It's a completely unreasonable expectation for people running their own wikis, possibly in their free time, their bug reports get mostly ignored and their patches spend months if not forever in Gerrit without anyone ever looking at them.

This policy doesn't apply to people writing their own extension for their own use. But yes, their extension might break without warning.

> More specifically to the foreign wiki issue, what would the core change be? Do we really want to create a factory class for everything that takes a wiki ID, when those classes won't be necessary once MediaWikiServices supports foreign wikis?

I'm not particularly keen on doing that, but the code is trivial enough. We could have a helper class or a trait for the boiler plate, even.

Tgr (WMF) (talkcontribs)

This page is definitely meant for people writing their own extensions, so that they end up with maintainable code that others can reuse. Although I guess it would make sense to treat it as a policy (which it currently isn't) for Wikimedia-deployed extensions and merely as guidance for others.

DKinzler (WMF) (talkcontribs)

You are right, of course - it is meant for them, but thy are not bound by it. Sorry for being imprecise.

Tgr (WMF) (talkcontribs)

The point is, don't add MUST NOTs that are not reasonable expectations for most of the intended audience (even if they are not bound by it).

DKinzler (WMF) (talkcontribs)

I think they are reasonable expectations. The only way to get out of the we-can't-change-core-without-breaking-extensions dead end is to limit what extensions can do (or what they can expect to continue working with the next release).

But you are right about my original proposal being too restrictive. It should be more along the lines of:

  • MUST NOT: directly instantiate a service class defined by core, instead of obtaining an instance via a service container, except when wrapping, replacing or defining a service that is documented to be an extension point.

Another way of phrasing this would be

  • MUST NOT rely on the stability of a constructor signature of a class that is not defined to be a pure value object, or documented to be an extension point suitable for subclassing or direct construction by extensions.

The second option is not less restrictive, but perhaps more to the point.


Reply to "Don't: directly construct service objects"
Smalyshev (WMF) (talkcontribs)

One thing that should be avoided even more than global state (e.g. global variables/public static properties in classes) is local static vars and similar instances of hidden local state, especially when used for caching. It looks like a very good idea when writing it, and is absolute nightmare when debugging/testing, because it creates hidden un-clearable state which could be in any random configuration when you get to it.

Reply to "Avoid global state"
Tgr (WMF) (talkcontribs)
BDavis (WMF) (talkcontribs)

+1 for src/ in new projects. includes/ is a very PHP3/4 convention.

Reply to "Deprecate /includes"

Namespacing extensions

6
Summary by Legoktm

Clarified acceptable namespaces in the document.

SamanthaNguyen (talkcontribs)

I see that the best practice for namespacing extensions would be MediaWiki\Extensions\ExtensionName, but I'm aware that there's also extensions that follow this pattern: MediaWiki\ExtensionName (skips the Extensions namespace prefix).

Examples include Linter (MediaWiki\Linter) and GlobalUserPage (MediaWiki\GlobalUserPage).

Which convention out of the two should people follow (or is it simply a recommendation/suggestion?)

Samwilson (talkcontribs)

I thought the convention was MediaWiki\Extension\ExtensionName (i.e. singular), and that's what I've been following (it seems least likely to result in collisions or confusion). There's some discussion in phab:T166010 of the options.

SamanthaNguyen (talkcontribs)

Can’t believe I’ve actually been misreading it this whole time, so thanks. :P Thanks for the link too, that seems to clear it up a bit for me :) I’ll go with MediaWiki\Extension\ExtensionName too.

Legoktm (talkcontribs)

The current consensus is that if the extension name is something unique and unlikely to be ever used in core (e.g. GlobalUserPage), then it's ok to be directly under MediaWiki. But if it's a generic-ish word like "Translate" then it should be disambiguated with MediaWiki\Extension\Translate.

Nikerabbit (talkcontribs)

Hint taken :) Apropos, hopefully not too much off-topic, are there any recommendations for how to preserve some backwards compatibility (or not) when moving classes inside a namespace? I know that at least TranslateSVG and CentralNotice use some Translate classes.

Legoktm (talkcontribs)

For the record, I was just quoting Tim about "Translate" :)

I'm not sure if we've documented this anywhere, but you should add a class_alias to the bottom of the file containing the new class (e.g. https://phabricator.wikimedia.org/source/mediawiki/browse/master/includes/libs/rdbms/database/Database.php;e1aabf2f24aef20adc72db8a750704cbb33236c6$3786) and then make sure the autoloader mapping still has the old class name pointing to the new file location.

Also, Tim is currently working on a tool to automatically namespaceize core that should also be useable for extensions, so it might make sense to wait for that instead of doing it manually.

Don't overuse private visibility in services

4
Summary by Legoktm

Removed the section.

Tgr (WMF) (talkcontribs)

I have difficulty interpreting this point. Classes should not be injected with many services? Services should not have many properties? Their visibility should be something other than private?

Thiemo Kreuz (WMDE) (talkcontribs)

The same idea is repeated further down in the document. I support the majority of this document, but this makes me sad. And I believe I can have an opinion on that because I still remember the time when I had the same idea. I was wrong: Services, classes, basically every code should only expose what it intentionally wants to expose. All kinds of interfaces (this includes class and function signatures) should be as narrow as possible, and only be opened up if an actual need to do so arises. Everything else should be private. Not protected, as protected is essentially "public to subclasses". Private by default.

Can we change the point to "Use private by default" or something like "Function, class and interface signatures should be as narrow as possible"? Or, if we can not agree on this, at least remove the point for now?

Legoktm (talkcontribs)

IIRC the idea was to make things protected rather than private so extensions have a chance to modify/extend things. I think it was a direct response to the "keep things as narrow as possible" concept because it makes external customization harder.

Thiemo Kreuz (WMDE) (talkcontribs)

To me, this sounds like a misconception. When I introduce private properties or methods while working on the implementation details of a class, I don't do this to somehow make it easier for future subclasses to "modify/extend things". What "things" would that be, anyway? I typically have no idea if a class will ever have subclasses, and what needs these will have. I design an interface for what I know needs to be shared with the world. But the implementation I write is just that, implementation. Making things "protected by default" is literally exposing implementation details.

Extension developers can easily submit a patch to make a method protected, if this is really what they need. Such a patch will typically (as I have seen it happen many times) start a brief discussion that will very quickly lead to a much better, much more sustainable architecture.

Don't: subclass anything not intended to be subclassed by extensions

2
DKinzler (WMF) (talkcontribs)

I propose to add the following to the "Don't" section:

  • MUST NOT: subclass (extend) any class defined by MediaWiki core, unless that class is explicitly documented to allow subclassing by extensions.

The reason for this is that protected methods are not part of our stable interface per the Deprecation policy, so such subclasses may break without warning. The relevant section of the deprecation policy reads:

[...] the stable part of the PHP API is comprised of all code that is explicitly marked public, and has been included in at least one stable release. In addition, some classes expect to be subclassed in extensions; in those cases protected functions also are included in the API. These classes should have a note in their documentation comment that they expect subclassing. If no note is present, it SHOULD be assumed that the class is not expected to be subclassed.

I'm tempted to even extend this to implementing interfaces. Not all interface declarations are intended as extension points, and not all of them should be considered stable. If we go that far, we should also add:

  • MUST NOT: implement any interface declared by MediaWiki core, unless that interface is explicitly documented to allow implementation by extensions.
Thiemo Kreuz (WMDE) (talkcontribs)

+1 to both suggestions from my side.

I, personally, find it a really helpful rule-of-thumb to consider all classes to be final, except otherwise stated. The effect of this restriction is pretty much the same as you suggest: one can not extend anything, except it is allowed. The only reason we are not literally marking all our code as final is that it would be cumbersome and error-prone. But we should still threat it like it is.

Reply to "Don't: subclass anything not intended to be subclassed by extensions"

Injecting a logging channel for a class: Through constructor injection or setter injection?

3
SamanthaNguyen (talkcontribs)

This manual is useful, however, it does not specify whether it's recommended to inject the logging channel through a public setter or through the constructor for your object. Currently in MediaWiki core this seems to be inconsistent, which makes it confusing. Here is to note how all core services defined here ( https://github.com/wikimedia/mediawiki/blob/master/includes/ServiceWiring.php ) currently do it:

  • \MimeAnalyzer: Constructor
  • \MediaWiki\Storage\NameTableStoreFactory: Constructor
  • \OldRevisionImporter: Constructor
  • \MediaWiki\Preferences\PreferencesFactory: Setter
  • \MediaWiki\Revision\RevisionRenderer: Setter
  • \MediaWiki\Revision\RevisionStoreFactory: Constructor
  • \MediaWiki\Shell\CommandFactory: Setter
  • \UploadRevisionImporter: Constructor
  • \ImportableOldRevisionImporter: Constructor

From my knowledge, a service **must** be immutable. By providing the logger to the object through a setter means it's can be changed even after instantiation, which goes against practice of dependency injection. Plus, with the fact that it can still be changed, it means it's harder to debug which seems to be counter-intuitive. in the document, it mentions SHOULD: Use dependency injection under the Architecture section. Personally, I prefer to use constructor injection in general.

I believe we should have a standard on this, and this document should be suggesting which way. What is the benefit of having a class implement the \Psr\Log\LoggingAwareInterface and use the \Psr\Log\LoggerAwareTrait to satisfy the interface instead? Shouldn't it always be done through the constructor? I feel like this is a discussion worth having; please let me know if I've misunderstood. thank you.

Thiemo Kreuz (WMDE) (talkcontribs)

The one problem with constructor injection is – even if clearly to be preferred – that one might end with a constructor with a dozen or more parameters. The constructor becomes harder to use. Parameters might get confused (if they don't have strict type hints, or adjacent types are the same). Setting up additional test cases is a pain because of all the services that need to be mocked and injected, but don't matter for the particular test case. A situation like this hints at a higher-level design issue: A service that needs so much stuff to be injected probably does way to many unrelated things, and should be broken up.

With this out of the way my personal rule of thumb is a pretty strict "dependency injection at construction time". But there is one exception: loggers. See, a logger does not really change the behavior of a service. It is merely a side-effect if what the service does additionally creates log entries – or not. Even if the logger is changed mid-way, nothing happens.

I still prefer constructor injection, even for loggers. But if the constructor is already quite big (let's say 4 or more arguments), or I want to temporarily add logging to an existing class, I avoid making the constructor more complicated. A setLogger method that defaults to a NullLogger if not called (super useful in test situations) is easy to add – and to remove.

SamanthaNguyen (talkcontribs)

Thiemo: Thank you for the elaborate answer! I believe this answers my question.

Samwilson (talkcontribs)

Current wording: "Prominent version-number indication in the README and main PHP files (ideally all files)".

I take it this is for the version a file (or method, etc.) was introduced? Not that the current version number should be added to every file (apart from the readme and extension.json).

And also: do version numbers for extensions actually mean anything? What are they used for (other than for display in Special:Version)?

Thiemo Kreuz (WMDE) (talkcontribs)

If a version number is not actively used as a tool for communication, it is indeed pretty worthless. More and more MediaWiki extensions just remove the version number because of this.

This is different for smaller code libraries that – ideally – follow semantic versioning. These can also use @since tags – which is the only situation where I find version numbers helpful.

I would not copy-paste a version number around, even if an extension still uses one. What would be the benefit of that? The biggest problem with having the same version number in multiple "prominent" places is that they must all be updated. Every time. No, please don't do this. Have it in extension.json if it still means something for the main developers of an extension. Otherwise, just remove it.

Reply to "Version numbers in all files?"

Rating for "uninstallation maintenance scripts"

1
SamanthaNguyen (talkcontribs)

There's no rating for this item under the Database section. Is it something that must be done, should be, or recommended?

Reply to "Rating for "uninstallation maintenance scripts""

Place for maintenance scripts

2
Summary by MGChecker

Maintenance scripts must be put into a maintenance/ directory.

Nikerabbit (talkcontribs)

In the file layout, I think it should mention that maintenance scripts should be placed into directory named maintenance. Name scripts is also used a lot, but I'd rather not give options here like there is currently for src/includes modules/resources.

Legoktm (talkcontribs)