Page MenuHomePhabricator

Skin hooks should run after extension hooks
Open, Needs TriagePublicBUG REPORT

Description

Skins make use of hooks to allow extensions to integrate themselves into the skin and support the extension-ecosystem we have. However, often skins need to make last minute modifications based on the data collected via extensions - for example in some cases it may want to modify HTML to support icons, it may want to disable certain features under certain circumstances, or prefer certain options over others when available.

What happens?:

Right now all hooks are registered and run in ain first in-first out order of registration. This meant that when rolling out a new version of Vector we hit a bug T287533. We fixed this by creating a skin function that essentially wrapped the hook so the skin could register it's hook using inheritance instead of hooks.

What should have happened instead?:

Ideally all skin hooks would be run after extension hooks. Since skins are responsible for the output this makes sense from a product point of view.

  • Notes **

In CI we enforce that extensions are loaded before skins so this issue was not picked up in our CI or beta cluster. However, it did surface as an issue in production.

Related: T212482

Related slack thread
https://wikimedia.slack.com/archives/C01R06P8D1B/p1626381636166400
(Sorry this is a private, but basically says all of the above in a less coherent way :))

Event Timeline

Jdlrobson renamed this task from Allow skins to run hooks last to Skin hooks should run after extension hooks.Jul 28 2021, 6:27 PM

That does not seem like an elegant solution. I don't understand why skins even need hooks. Skins are subclasses. A skin especially shouldn't have to hook its own parent, which is apparently the subject of T287533. Just have SkinTemplate call an empty protected method that SkinVector overrides.

I agree. The method in question already exists. Skin have the protected buildContentNavigationUrls method for this purpose.

buildContentNavigationUrls method for this purpos

Not really. The skin hooks wanted to run /after/ the extension hooks, but not after buildContentNavigationUrls. To solve this issue we refactored the code and pulled out a new method runOnSkinTemplateNavigationHooks.

Skins can run hooks directly after PersonalUrls hook by extending buildPersonalUrls method, as the hook is run at the end of the function but if buildPersonalUrls method body changes in future, that might not be a solution.

If extending skins methods is the way to go in future, wouldn't it make sense to deprecate the registration of skin hooks from inside skins to avoid this confusion?

Change 720080 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/skins/Vector@master] Use buildContentNavigationUrls() override instead of odd hook calling

https://gerrit.wikimedia.org/r/720080

Change 720081 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] skins: Revert \"SkinTemplate: Factor out SkinTemplateNavigation hooks\"

https://gerrit.wikimedia.org/r/720081