Page MenuHomePhabricator

ChangeTools should be a service
Open, Needs TriagePublicBUG REPORT

Description

In T51541 we added the thanks link to various special pages.
In https://gerrit.wikimedia.org/r/c/mediawiki/core/+/867732 @Tgr suggested the new functionality should be pulled out into a service.

Best practice is to not do non-trivial work in the constructor. Moreover, I don't think the usage pattern of create object - call toHTML - discard object makes that much sense. The object has some service dependencies. Injecting those dependencies shouldn't be the concern of the pagers (otherwise when the dependencies change for some reasons, all the pagers need to be updated), it should be handled by the service wiring.

So IMO this should be a service, LinkRenderer and HookRunner should be passed to the constructor, the other arguments should be passed to its main method, and the object should be reused between lines. The clickjacking flag will need a different handling (since services should not have externally-visible state) - either an output parameter, or maybe pass in OutputPage and have this class call setPreventClickjacking() on it.