Topic on Talk:Best practices for extensions

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.

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.

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