Page MenuHomePhabricator

20% of hooks appear to be unused
Closed, InvalidPublic

Description

By my count, 111 hooks listed in hooks.txt have no registered handler[1]

A full list is available at P10792

I propose that they all be deprecated and removed[2]

Does it make sense to do this before or after T247088: New Hook System?

  • If the hook system will be implemented before 1.35 is cut, I suggest deprecating after the hook system is implemented, but before 1.35 is cut, to remove them all in 1.36
  • If the hook system will not be implemented before 1.35 is cut, I suggest deprecating before the hook system is implemented and before 1.35 is cut, and then removing them all in 1.36 before the hook system is implemented

[1] Checked using codesearch, lists all known repositories but not offline extensions, etc
[2] Some are already listed as deprecated, so in that case just remove

Event Timeline

Restricted Application added a project: User-DannyS712. · View Herald TranscriptMar 27 2020, 5:46 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
DannyS712 moved this task from Unsorted to Next on the User-DannyS712 board.Mar 27 2020, 5:46 AM
DannyS712 moved this task from Unsorted to Needs removal on the Technical-Debt board.
DannyS712 added subscribers: nnikkhoui, tstarling.

CC those working on implementing the new hook system - @nnikkhoui @tstarling

DannyS712 updated the task description. (Show Details)Mar 27 2020, 5:51 AM
ashley added a subscriber: ashley.EditedMar 30 2020, 9:04 PM

I'd like to propose keeping some of these hooks, at least:

  • AbortDiffCache — used by SiteWideMessages (originally a Wikia extension, but there's a private ShoutWiki version of it as well)
  • ArticlePageDataAfter — used by wikiHow code
  • ArticleShowPatrolFooter — used by wikiHow code (see gerrit patchset where it was added)
  • most of the DifferenceEngine* hooks — used by wikiHow code (see gerrit patchset where they were added) and some of these are also used by SocialProfile to implement the "avatars in diffs" feature (which probably needs some modernization due to the MCR stuff and whatnot, but I don't know how to do that and a while back core only seemed to accept Revisions and not RevisionRecords in some places; see this patch for more details about that); some are also used by the aforementioned SiteWideMessages extension
  • GetLangPreferredVariant — used by wikiHow code (see gerrit patchset where it was added)
  • MessageCacheReplace — was originally added by Emil Podlaszewski for Wikia back in 2009; is still used by three Wikia extensions and a ShoutWiki skin (Quartz, which is an ex-Wikia skin)
  • OutputPageAfterGetHeadLinksArray — used by wikiHow code (see gerrit patchset where it was added)
  • SpecialRandomGetRandomTitle — current replacement for the deprecated $wgExtraRandompageSQL configuration option; hence seems like something that may well have users not showing up in codesearch
  • UserRequiresHTTPS — used by wikiHow code
  • UserResetAllOptions — used by wikiHow code (see gerrit patchset where it was added)
  • UserRetrieveNewTalks — used by ShoutWiki's SharedNewtalk (global notification of new User_talk: messages) extension; also used by the aforementioned SiteWideMessages extension

These are used by HydraWiki extensions:

While codesearch is immensely useful and handy, it does not catch everything, obviously. Let's make sure we don't throw out the baby with the bathwater here and that third-party users don't have to resort to awful local core hacks.

edwardspec added a subscriber: edwardspec.EditedApr 2 2020, 1:28 AM

The following hooks (although unused now) have valid usecases where they can be needed (private corporate wikis, etc.):

GetCanonicalURL
GetIP
UserLoadDefaults

GetSlotDiffRenderer is a recently added MCR-related hook (it's not used yet, but it makes sense that an extension should be able to replace SlotDiffRenderer):.

One of the private extensions I know uses PageHistoryPager::doBatchLookups hook (when viewing History, it displays additional yes/no icon for every revision, and uses this hook to batch-fetch "yes or no?" flag for all revisions in 1 SQL query).

Krinkle closed this task as Invalid.Apr 11 2020, 5:40 PM
Krinkle added a subscriber: Krinkle.

Being unused is not a problem. Please create a task about a specific hook instead, but only if there is a problem. For example, if they are slow, or if there is a maintenance cost for compatibility. In general, hooks are cheap and we should not try to reduce them. That creates a lot of work and discussion without helping ourselves or users.

I am closing this now, but please if there is a compatiblity cost or performance cost with any of these, feel free to create a separate task. Be sure to also involve the maintainers of that area (for example Performance Team for hooks in ResourceLoader) and ask them if it helps them to not have this hook.